test_keystoneclient_sql fails when run by itself

[Brant Knudson]

Commit efc30beab10faf4720a96322adb135726662b025 creates or exposes a problem with
test_keystoneclient_sql so that it doesn't run by itself using sqlite.
(I also tried with mysql and it didn't fail when run by itself.)

These 2 tests in test_keystoneclient_sql.KcMasterSqlTestCase fail:
test_ec2_credentials_delete_user_forbidden
test_ec2_credentials_get_user_forbidden

The error messages are like:
ClientException: An unexpected error prevented the server from fulfilling your request. (OperationalError) no such table: ec2_credential

Note that can run the test by itself but if run test_ec2_credentials_delete_user_forbidden after another ec2 test in
KcMasterSqlTestCase it then fails.

Here's some examples:
./run_tests.sh test_keystoneclient_sql:KcMasterSqlTestCase.test_ec2_credentials_delete_user_forbidden
- WORKS
./run_tests.sh test_keystoneclient_sql:KcMasterSqlTestCase.test_ec2_credentials_get_user_forbidden
- WORKS

./run_tests.sh \
 test_keystoneclient_sql:KcMasterSqlTestCase.test_ec2_credentials_delete_user_forbidden \
 test_keystoneclient_sql:KcMasterSqlTestCase.test_ec2_credentials_get_user_forbidden
- test_ec2_credentials_delete_user_forbidden WORKS
- test_ec2_credentials_get_user_forbidden FAILS

./run_tests.sh -s \
 test_keystoneclient_sql:KcMasterSqlTestCase.test_ec2_credential_crud \
 test_keystoneclient_sql:KcMasterSqlTestCase.test_ec2_credentials_delete_user_forbidden
- test_ec2_credential_crud WORKS
- test_ec2_credentials_delete_user_forbidden FAILS

First, for some reason none of the tables appear to be available to the server when running the ec2 test after another test has run. You can query them right after the create_all, but then they dissappear to the ec2 backend. Not sure how this could happen. Maybe a sqlite/sqlalchemy usage problem, or multithreading or timing issue?

Second, it looks like it's possible that sql.ModelBase.metadata.create_all() gets called before all the models are available (have been imported) a this point:
https://github.com/openstack/keystone/blob/master/tests/test_keystoneclient_sql.py#L40

Specifically, the Ec2Token model is in keystone.contrib.ec2.backends.sql and that's not always imported with the rest of the models. Maybe the Ec2Token model should be moved into a common module with the rest of the models so that they're all imported together.

One thing that should be considered is whether this bit of code is safe:
https://github.com/openstack/keystone/blob/master/keystone/common/sql/core.py#L253

It seems unsafe to implicitly create tables since that should be taken care of by the tester or keystone-manage.

[Brant Knudson (blk-u)]

I have a theory for why sqlite is failing. I think when you create different engines with in-memory sqlite:// they're not the same database. (Whereas for Mysql different engines all go to the same database server.)

[Adam Young (ayoung)]

You are corret, Brant.

[Brant Knudson (blk-u)]

I added some print statement and this shows that the second test is using a cached engine.

$ ./run_tests.sh -s \
 test_keystoneclient_sql:KcMasterSqlTestCase.test_ec2_credentials_delete_user_forbidden \
 test_keystoneclient_sql:KcMasterSqlTestCase.test_ec2_credentials_get_user_forbidden

KcMasterSqlTestCase
    test_ec2_credentials_delete_user_forbidden
create_all with engine id=40764432
create_credential engine id=8970912
disposing engine id=40764432
OK
    test_ec2_credentials_get_user_forbidden
create_all with engine id=48610960
create_credential engine id=40764432
ERROR
disposing engine id=48610960

Looks like it's getting cached here: https://github.com/openstack/keystone/blob/master/keystone/common/sql/core.py#L220
In Base class, does "self._engine = self._engine or self.get_engine()"

Base doesn't know that the engine it had stored has been disposed.

Options
a) Maybe it's just not safe to dispose the engine in the tests, so stop doing that
b) Maybe Base shouldn't cache the engine.
c) Implement notify Base when engine is disposed

[Brant Knudson (blk-u)]

I removed the caching from Base and that seems to work. From looking at the caching code, it wasn't safe anyways, since you could get a autocommit=False session when you were expecting an autocommit=True one.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/29321

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/29669

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/29670

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/29669
Committed: http://github.com/openstack/keystone/commit/eb930fd2f8d34ed8c5a8701934a314ee459c428b
Submitter: Jenkins
Branch: master

commit eb930fd2f8d34ed8c5a8701934a314ee459c428b
Author: Brant Knudson <email address hidden>
Date: Sat May 18 13:52:49 2013 -0500

    Add callbacks for set_global_engine

    This adds functionality where a class can monitor for the global
    engine changing.

    This is useful for a class that caches the global engine and
    wants to know when its cached global engine isn't valid anymore.

    Part of fix for bug 1179259

    Change-Id: I5736a05308c63de9fccb8af7720ddd70530f4270

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/29670
Committed: http://github.com/openstack/keystone/commit/405a914db7d2938a76384821e556df9024e6c8ac
Submitter: Jenkins
Branch: master

commit 405a914db7d2938a76384821e556df9024e6c8ac
Author: Brant Knudson <email address hidden>
Date: Sat May 18 14:28:34 2013 -0500

    Clear cached engine when global engine changes

    The keystone.common.sql.core.Base class cached the global database
    engine when get_session() was called. When the global database engine
    changed to a new instance, the cached copy was used in subsequent
    calls to get_session(), leading to using the old engine and tests
    failing to run by themselves.

    This change makes it so that when the global database engine is
    changed, Base will use the new engine rather than the invalid one.

    Change-Id: I75aa3c230d9b4fd666ab8d478c9e9a27669905e8
    Fixes: Bug #1179259