)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ccad41fa1d627c03d93b3ebd13445eee1fac1427","unresolved":true,"context_lines":[{"line_number":18,"context_line":"factory, one for the api DB and one for the main DB. There was a global"},{"line_number":19,"context_line":"flag SESSION_CONFIGURED in our Database fixture that guarded against"},{"line_number":20,"context_line":"double initialization of the factory. But the faulty test cases in"},{"line_number":21,"context_line":"question does not use our Database fixture but use the"},{"line_number":22,"context_line":"OpportunisticDBTestMixin from oslo_db. Obviously that fixture does not"},{"line_number":23,"context_line":"know about our SESSION_CONFIGURED global. So if one of the offending"},{"line_number":24,"context_line":"test case run first in an executor then that could initialize the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"3f685d40_382b4976","line":21,"range":{"start_line":21,"start_character":9,"end_line":21,"end_character":13},"updated":"2021-10-27 16:23:00.000000000","message":"nit: do (or did, if going with past tense)","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"23d0878fbd5cc9493d056c5ed31a7e4e6e6f8572","unresolved":false,"context_lines":[{"line_number":18,"context_line":"factory, one for the api DB and one for the main DB. There was a global"},{"line_number":19,"context_line":"flag SESSION_CONFIGURED in our Database fixture that guarded against"},{"line_number":20,"context_line":"double initialization of the factory. But the faulty test cases in"},{"line_number":21,"context_line":"question does not use our Database fixture but use the"},{"line_number":22,"context_line":"OpportunisticDBTestMixin from oslo_db. Obviously that fixture does not"},{"line_number":23,"context_line":"know about our SESSION_CONFIGURED global. So if one of the offending"},{"line_number":24,"context_line":"test case run first in an executor then that could initialize the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"66ded1c4_fbdf32d3","line":21,"range":{"start_line":21,"start_character":9,"end_line":21,"end_character":13},"in_reply_to":"3f685d40_382b4976","updated":"2021-10-27 17:20:18.000000000","message":"Done","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ccad41fa1d627c03d93b3ebd13445eee1fac1427","unresolved":true,"context_lines":[{"line_number":20,"context_line":"double initialization of the factory. But the faulty test cases in"},{"line_number":21,"context_line":"question does not use our Database fixture but use the"},{"line_number":22,"context_line":"OpportunisticDBTestMixin from oslo_db. Obviously that fixture does not"},{"line_number":23,"context_line":"know about our SESSION_CONFIGURED global. So if one of the offending"},{"line_number":24,"context_line":"test case run first in an executor then that could initialize the"},{"line_number":25,"context_line":"transaction factory globally and a later test that uses our Database"},{"line_number":26,"context_line":"fixture will try to configure it again leading to the error. For some"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"a38bfcbd_c706fa01","line":23,"updated":"2021-10-27 16:23:00.000000000","message":"This is no longer true since you removed the global in the previous patch, right. Should we rework this to reflect that? Alternatively, you could switch the order of these so we can backport this specific patch easier\n\nLater: ah wait, you\u0027ve actually reflected the change in the next patch. This would be clearer if it was written in past tense throughout, e.g.\n\n  This error happened because...","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"23d0878fbd5cc9493d056c5ed31a7e4e6e6f8572","unresolved":false,"context_lines":[{"line_number":20,"context_line":"double initialization of the factory. But the faulty test cases in"},{"line_number":21,"context_line":"question does not use our Database fixture but use the"},{"line_number":22,"context_line":"OpportunisticDBTestMixin from oslo_db. Obviously that fixture does not"},{"line_number":23,"context_line":"know about our SESSION_CONFIGURED global. So if one of the offending"},{"line_number":24,"context_line":"test case run first in an executor then that could initialize the"},{"line_number":25,"context_line":"transaction factory globally and a later test that uses our Database"},{"line_number":26,"context_line":"fixture will try to configure it again leading to the error. For some"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"8ca560a3_b38c83cb","line":23,"in_reply_to":"a38bfcbd_c706fa01","updated":"2021-10-27 17:20:18.000000000","message":"Done","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ccad41fa1d627c03d93b3ebd13445eee1fac1427","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"e653567c_e3c96648","line":52,"updated":"2021-10-27 16:23:00.000000000","message":"Are we backporting? Care to open a quick bug if so?","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"23d0878fbd5cc9493d056c5ed31a7e4e6e6f8572","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"7413ae19_813adb09","line":52,"in_reply_to":"e653567c_e3c96648","updated":"2021-10-27 17:20:18.000000000","message":"Done","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ccad41fa1d627c03d93b3ebd13445eee1fac1427","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"ac15ca46_69a42b7c","updated":"2021-10-27 16:23:00.000000000","message":"I have some nits on the commit message that I\u0027d be happy to see addressed, but I think this is good enough as-is. If you do respin, let me know and I\u0027ll re +2 (or feel free to carry my +2)","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"}],"nova/tests/fixtures/nova.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ccad41fa1d627c03d93b3ebd13445eee1fac1427","unresolved":true,"context_lines":[{"line_number":199,"context_line":"            \u0027_create_session\u0027,"},{"line_number":200,"context_line":"            self._poison_configure))"},{"line_number":201,"context_line":""},{"line_number":202,"context_line":"        self.useFixture(fixtures.MonkeyPatch("},{"line_number":203,"context_line":"           \u0027oslo_db.sqlalchemy.enginefacade._TransactionFactory._start\u0027,"},{"line_number":204,"context_line":"           self._poison_configure))"},{"line_number":205,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1f624753_5a8c23a3","line":202,"updated":"2021-10-27 16:23:00.000000000","message":"A comment about why we do this would be helpful to avoid it becoming tribal knowledge","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"23d0878fbd5cc9493d056c5ed31a7e4e6e6f8572","unresolved":false,"context_lines":[{"line_number":199,"context_line":"            \u0027_create_session\u0027,"},{"line_number":200,"context_line":"            self._poison_configure))"},{"line_number":201,"context_line":""},{"line_number":202,"context_line":"        self.useFixture(fixtures.MonkeyPatch("},{"line_number":203,"context_line":"           \u0027oslo_db.sqlalchemy.enginefacade._TransactionFactory._start\u0027,"},{"line_number":204,"context_line":"           self._poison_configure))"},{"line_number":205,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"d6c98d37_75ab11a5","line":202,"in_reply_to":"1f624753_5a8c23a3","updated":"2021-10-27 17:20:18.000000000","message":"Done","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"}],"nova/tests/unit/db/api/test_migrations.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"481db1ed22728722ea99fae15ce6deb1e8c0ef40","unresolved":true,"context_lines":[{"line_number":255,"context_line":""},{"line_number":256,"context_line":"        with mock.patch.object(migration, \u0027_get_engine\u0027, return_value\u003dengine):"},{"line_number":257,"context_line":"            migration.db_sync(database\u003d\u0027api\u0027)"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":"            script \u003d alembic_script.ScriptDirectory.from_config(self.config)"},{"line_number":260,"context_line":"            head \u003d script.get_current_head()"},{"line_number":261,"context_line":"            self.assertEqual(head, migration.db_version(database\u003d\u0027api\u0027))"}],"source_content_type":"text/x-python","patch_set":3,"id":"49827eb3_e2c8838a","line":258,"updated":"2021-10-20 19:17:20.000000000","message":"I looked through the code and I think I see why doing this helps. As you show here, the migration.db_sync() will call migration._get_engine(), which if unpatched, will call the global _TransactionContextManager get_engine() method [1]. The get_engine() method calls the internal _TransactionFactory get_engine() method [2] and that will \"start\" the factory if it is not already started.\n\nWhen this test uses the global engine and this test runs before a test using one of the Database fixtures, the Database fixture will blow up when it tries to configure() the global _TransactionContextManager [3] as the eventual call to the internal _TransactionFactory configure() method [4] will raise if it finds the _TransactionFactory already started.\n\nI guess the only reason this fails on a subset of tests is because this test would have to run before any test using Database fixture, which becomes more unlikely the more tests are run in the set.\n\nSo, this works to fix the issue.\n\nI note that the Database fixtures use a global SESSION_CONFIGURED to avoid hitting the AlreadyStartedError. I wondered if another way to fix this would be to make the Database fixture call the global _TransactionContextManager is_started() method [6] and do something different if so, but AFAICT it would have to involve swapping a new engine or factory into the global _TransactionContextManager as a reset, because once a _TransactionFactory is started, there isn\u0027t a way to reset it to not _started so the fixture could configure() it.\n\nMaybe we want to do that? Not sure. I\u0027m just thinking of how it might be tough to remember to use a local engine for any new non-Database fixture test that might trigger the global _TransactionFactory to \"start\" if it runs before the first test that uses Database fixture. _TransactionContextManager does have a couple of methods available for doing this: patch_factory() and patch_engine() [7], that are intended for use with test suites. \n\n[1] https://github.com/openstack/oslo.db/blob/d7742015f732625ff0c3e455ac5e1c01ba3d9a45/oslo_db/sqlalchemy/enginefacade.py#L819\n[2] https://github.com/openstack/oslo.db/blob/d7742015f732625ff0c3e455ac5e1c01ba3d9a45/oslo_db/sqlalchemy/enginefacade.py#L361\n[3] https://github.com/openstack/nova/blob/e14eef0719eceef35e7e96b3e3d242ec79a80969/nova/tests/fixtures/nova.py#L616\n[4] https://github.com/openstack/oslo.db/blob/d7742015f732625ff0c3e455ac5e1c01ba3d9a45/oslo_db/sqlalchemy/enginefacade.py#L320-L322\n[5] https://github.com/openstack/oslo.db/blob/d7742015f732625ff0c3e455ac5e1c01ba3d9a45/oslo_db/sqlalchemy/enginefacade.py#L790\n[6] https://github.com/openstack/oslo.db/blob/d7742015f732625ff0c3e455ac5e1c01ba3d9a45/oslo_db/sqlalchemy/enginefacade.py#L790\n[7] https://github.com/openstack/oslo.db/blob/d7742015f732625ff0c3e455ac5e1c01ba3d9a45/oslo_db/sqlalchemy/enginefacade.py#L871","commit_id":"a14125feb0e6cfcdbf18d223235d736d9ae48a13"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"401c02db84abe21c77a2baab4ca9ccf3979786b5","unresolved":true,"context_lines":[{"line_number":255,"context_line":""},{"line_number":256,"context_line":"        with mock.patch.object(migration, \u0027_get_engine\u0027, return_value\u003dengine):"},{"line_number":257,"context_line":"            migration.db_sync(database\u003d\u0027api\u0027)"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":"            script \u003d alembic_script.ScriptDirectory.from_config(self.config)"},{"line_number":260,"context_line":"            head \u003d script.get_current_head()"},{"line_number":261,"context_line":"            self.assertEqual(head, migration.db_version(database\u003d\u0027api\u0027))"}],"source_content_type":"text/x-python","patch_set":3,"id":"049c37cd_7aee2588","line":258,"in_reply_to":"49827eb3_e2c8838a","updated":"2021-10-27 15:01:36.000000000","message":"Thanks Melanie! Finally I can update the commit message with explanation :). Also I was able to extend the existing DB poison to detect similar problems in the future directly. It needed a bunch of refactorings and I ended up removing the global SESSION_CONFIGURED flag as well based on your suggestions. (See the parent patches)","commit_id":"a14125feb0e6cfcdbf18d223235d736d9ae48a13"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ccad41fa1d627c03d93b3ebd13445eee1fac1427","unresolved":false,"context_lines":[{"line_number":261,"context_line":"    def test_db_version_alembic(self):"},{"line_number":262,"context_line":"        engine \u003d enginefacade.writer.get_engine()"},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"        with mock.patch.object(migration, \u0027_get_engine\u0027, return_value\u003dengine):"},{"line_number":265,"context_line":"            migration.db_sync(database\u003d\u0027api\u0027)"},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"            script \u003d alembic_script.ScriptDirectory.from_config(self.config)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9959a3e3_948a9f00","line":264,"updated":"2021-10-27 16:23:00.000000000","message":"I feel I\u0027ve already said this in a review, but we should drop the \u0027_get_engine\u0027 mechanism in favour of injecting the engine as a parameter to the db_sync method. Another time","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"}],"nova/tests/unit/db/test_migration.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ccad41fa1d627c03d93b3ebd13445eee1fac1427","unresolved":false,"context_lines":[{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class TestDBURL(test.NoDBTestCase):"},{"line_number":33,"context_line":"    USES_DB_SELF \u003d True"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"    def test_db_sync_with_special_symbols_in_connection_string(self):"},{"line_number":36,"context_line":"        qargs \u003d \u0027read_default_group\u003ddata with/a+percent_%-and%20symbols!\u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"c0578115_89e424b0","line":33,"updated":"2021-10-27 16:23:00.000000000","message":"nit: A \u0027CustomDBTestCase\u0027 or \u0027SelfManagedDBTestCase\u0027 base class would be a neat addition, since this parameter is not at all clear. Again though, not really your issue 😊","commit_id":"44d1f22a98eae2fdd00f9d96422a3a291bcfda1a"}]}