Open Stack

Hi Deva,

I haven't actually touched Ironic db migrations tests code yet, but
your feedback is very valuable for oslo.db maintainers, thank you!

So currently, there are two ways to run migrations tests:
1. Opportunistically (using openstack_citest user credentials; this is
how we test migrations in the gates in Nova/Glance/Cinder/etc). I'm
surprised we don't provide this out-of-box in common db code.
2. By providing database credentials in test_migrations.conf
(Nova/Glance/Cinder/etc have test_migrations.conf, though I haven't
ever tried to put mysql/postgresql credentials there and run unit
tests).

The latter came to common db code directly from Nova and I'm not sure
if anyone is using the incubator version of it in the consuming
projects. Actually, I'd really like us to drop this feature and stick
to the opportunistic tests of migrations (fyi, there is a patch on
review to oslo.db [1]) to ensure there is only one way to run the
migrations tests and this the way we run the tests in the gates.

[1] uses opportunistic DB test cases provided by oslo.db to prevent
race conditions: a db is created on demand per test (which is
obviously not fast, but safe and easy). And it's perfectly normal to
use a separate db per migrations test case, as this is a kind of a
test that needs total control on the database, which can not be
provided even by using of high transaction isolation levels (so
unfortunately we can't use the solution proposed by Mike here).

Migration tests using test_migrations.conf, on the other hand, leave
it up to you how to isolate separate test cases using the same
database. You could use file locks, put them on each conflicting test
case to prevent race conditions, but this is not really handy, of
course.

Overall, I think, this is a good example of a situation when we put
code to incubator when it wasn't really ready to be reused by other
projects. We should have added docs at least on how to use those
migrations tests properly. This is something we should become better
at as a team.

Ok, so at least we know about the problem and [1] should make it
easier for everyone in the consuming projects to run their migrations
tests.

Thanks,
Roman

[1] https://review.openstack.org/#/c/93424/

On Sat, Jun 7, 2014 at 3:12 AM, Devananda van der Veen
<devananda.vdv at gmail.com> wrote:
> I think some things are broken in the oslo-incubator db migration code.
>
> Ironic moved to this when Juno opened and things seemed fine, until recently
> when Lucas tried to add a DB migration and noticed that it didn't run... So
> I looked into it a bit today. Below are my findings.
>
> Firstly, I filed this bug and proposed a fix, because I think that tests
> that don't run any code should not report that they passed -- they should
> report that they were skipped.
>   https://bugs.launchpad.net/oslo/+bug/1327397
>   "No notice given when db migrations are not run due to missing engine"
>
> Then, I edited the test_migrations.conf file appropriately for my local
> mysql service, ran the tests again, and verified that migration tests ran --
> and they passed. Great!
>
> Now, a little background... Ironic's TestMigrations class inherits from
> oslo's BaseMigrationTestCase, then "opportunistically" checks each back-end,
> if it's available. This opportunistic checking was inherited from Nova so
> that tests could pass on developer workstations where not all backends are
> present (eg, I have mysql installed, but not postgres), and still
> transparently run on all backends in the gate. I couldn't find such
> opportunistic testing in the oslo db migration test code, unfortunately -
> but maybe it's well hidden.
>
> Anyhow. When I stopped the local mysql service (leaving the configuration
> unchanged), I expected the tests to be skipped, but instead I got two
> surprise failures:
> - test_mysql_opportunistically() failed because setUp() raises an exception
> before the test code could call calling _have_mysql()
> - test_mysql_connect_fail() actually failed! Again, because setUp() raises
> an exception before running the test itself
>
> Unfortunately, there's one more problem... when I run the tests in parallel,
> they fail randomly because sometimes two test threads run different
> migration tests, and the setUp() for one thread (remember, it calls
> _reset_databases) blows up the other test.
>
> Out of 10 runs, it failed three times, each with different errors:
>   NoSuchTableError: `chassis`
>   ERROR 1007 (HY000) at line 1: Can't create database 'test_migrations';
> database exists
>   ProgrammingError: (ProgrammingError) (1146, "Table
> 'test_migrations.alembic_version' doesn't exist")
>
> As far as I can tell, this is all coming from:
>
> https://github.com/openstack/oslo-incubator/blob/master/openstack/common/db/sqlalchemy/test_migrations.py#L86;L111
>
>
> So, Ironic devs -- if you see a DB migration proposed, pay extra attention
> to it. We aren't running migration tests in our check or gate queues right
> now, and we shouldn't enable them until this fixed.
>
> Regards,
> Devananda
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>