)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"fd64a6a7467ae965467a330fdea123bb0f7d045b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c098ac74_8988161d","updated":"2024-08-19 17:44:07.000000000","message":"\u003e We handle writer and reader connections in the\nsame path which means that even for requests\nthat are pure read operations we always\nconsume resources to instatiate connections\nto the primary database even if only _READER\nmode is requested.\n\nis there a demonstration case and/or launchpad issue?  This is not what the code would be doing here because the creation of an engine in SQLAlchemy does not imply connectivity, see https://docs.sqlalchemy.org/en/20/core/engines.html#engine-configuration\n\nif this unwanted connection is due to oslo.db\u0027s \"test connection\" I would simply turn that feature off, set max_retries to zero: https://github.com/openstack/oslo.db/blob/a59dba4479f9f391b63901b847d25a77093a460a/oslo_db/sqlalchemy/engines.py#L159   this \"immediate connect\" is a behavior that I think should ultimately be removed from oslo.db","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"7e09a7d4c607281605760bfcc2dd66de2ee82145","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"358bb1ec_461a22ac","updated":"2024-08-21 14:04:51.000000000","message":"I\u0027ve provided the correct fix for keystone at Ia4b4dc14bc4163463ef4271851b117e36baae0ec .  the reader session is not used in a \"sync\" context because we dont want reader failures due to replication lag.  if you dont care about replication lag you need to use either the asnyc_ modifier or turn off the \"sync reader\" feature","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"f50bc911c0f55b014cccb93ce23cc375106e0352","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"22823b79_6120d16c","updated":"2024-08-21 14:26:12.000000000","message":"That\u0027s awesome, I can confirm that it works with changing that in Keystone as per your patch.","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"64d7a14a5ff2d0bbf773b0bfdefc746c9f8993dd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"5457673f_0c7075a8","updated":"2024-08-21 12:40:49.000000000","message":"can you share with me how to run keystone in a plain wsgi commandline and where to put config as well as curl commands to get it to the point of the given stack trace?    mod_wsgi etc should not be needed to reproduce this problem","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"37407e75eaaa1f250c2352ff20efdaf8c0ccb5b1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"1c9d496f_1061fc2f","updated":"2024-08-21 12:36:15.000000000","message":"mostly what I\u0027d like to see here is starting with a bug report for keystone.   \"we set max_retries to zero, and disconnected the writer engine, and still the writer engine is being used\".  that\u0027s the bug.  it really should be in launchpad, if that\u0027s what openstack is still using to post issues.   Then I need background on how to reproduce in the most simple way and I will go into it and see what\u0027s wrong.   I think rushing ahead to make a major architectural change in enginefacade without understanding why our assumptions about what enginefacade does seem to not be holding up is the wrong approach and is going to introduce more issues.\n\nI really dont want this to merge until we know what\u0027s going on, thanks","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"9aa6b1386bf0ad5b594b3a27bdba403f6bdd1b40","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"b4cd36c2_6796eaf3","updated":"2024-08-19 17:45:46.000000000","message":"yah I see nothing in enginefacade that is making a connection without one being requested, so I would prefer to not change things in this way unless there is a compelling case that \"max_retries\" behavior is essential (that\u0027s what I need to see, rather than can we please just deprecate max_retries)","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"662839f2927eafe90a475dc945bb170fc2bb4f04","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e1b73816_bfa075ce","in_reply_to":"17f8b2a0_65031eb5","updated":"2024-08-21 14:37:20.000000000","message":"synchronous_reader is not part of the oslo.config part of it.   An application can\u0027t switch that setting; if a particular reader method depends on the availability of data that was just written to a write master and can\u0027t tolerate replication lag, you would never want to change that\n\nI can see this maybe being configurable if an application can tolerate small replication lag but not large replication lag, and that could vary, but that seems like a recipe for confusion","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"7a62fe44e0aa1ceec4eea291d0434b74045562e0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"be27ebcf_6cffc455","in_reply_to":"1d2fa370_19b7c534","updated":"2024-08-21 08:33:49.000000000","message":"I tried the changes (max_retries\u003d0) per [1] but still if the primary (connection cfg) database is dead all API requests (keystone) will fail with HTTP 500 trying to connect to that even though replica database (slave_connection cfg) is available.\n\nThis patch, splitting read and write, kept all API requests that only use read operations against slave_connection working even when primary (connection) database was down.\n\n[1] https://review.opendev.org/c/openstack/oslo.db/+/926649","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"6d77771d7cffeaea2d2fd13f362aac0960892760","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"1d2fa370_19b7c534","in_reply_to":"2e7e7c22_3dc042e8","updated":"2024-08-20 14:42:36.000000000","message":"I think what you have here will be what we\u0027d need and un-abandon if either im forgetting something, or if we really cant get rid of max_retries for some reason.   But the reason it\u0027s the way it is right now is because i did not forsee these \"startups\" as actually connecting to anything.  We should definitely have unit tests to ensure they stay that way.","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"2aaae80f0142ad90891486318d79c5537c70375a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"17f8b2a0_65031eb5","in_reply_to":"358bb1ec_461a22ac","updated":"2024-08-21 14:22:04.000000000","message":"from oslo_config this should be configurable as max_retries\u003d0 if we dont do that patch that removes it, but what about synchronous_reader is that configurable with config?","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"6f545a9a9916d439b745fa7d6022e5f6e253b052","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"f1c5d911_28fe7b27","in_reply_to":"42a8b571_c39157c6","updated":"2024-08-21 14:09:10.000000000","message":"never mind,  Ia4b4dc14bc4163463ef4271851b117e36baae0ec provides the correct configuration for keystone which is currently incorrect.  can you test with that please?\n\nHere\u0027s a demonstration script if you want to try something isolated\n\n\n    from oslo_db.sqlalchemy import enginefacade\n    from sqlalchemy import select\n    import threading\n\n    _CONTEXT \u003d None\n    def _get_context():\n        global _CONTEXT\n        if _CONTEXT is None:\n            # NOTE(dims): Delay the `threading.local` import to allow for\n            # eventlet/gevent monkeypatching to happen\n            import threading\n\n            _CONTEXT \u003d threading.local()\n        return _CONTEXT\n\n\n    _main_context_manager \u003d enginefacade.transaction_context()\n\n    _main_context_manager.configure(\n        sqlite_fk\u003dTrue,\n        max_retries\u003d0,\n\n        # it\u0027s OK to use the \"reader\" session for a call that is not decorated\n        # with \"async\"; meaning, it\u0027s OK that we might read from a replicated\n        # DB that has replication lag from the master.    if this is not set,\n        # then \"reader\" without using \"async.reader\" will use the **writer**\n        # session\n        synchronous_reader\u003dFalse,\n\n\n        connection\u003d\"mysql+pymysql://wronguser:wrongpass@localhost/test\",\n        slave_connection\u003d\"mysql+pymysql://scott:tiger@localhost/test\",\n    )\n\n    def session_for_read():\n        reader \u003d _main_context_manager.reader\n\n        return reader.using(_get_context())\n\n        # or use the \"async_\" reader here\n        # return reader.async_.using(_get_context())\n\n\n    def session_for_write():\n        writer \u003d _main_context_manager.writer\n        return writer.using(_get_context())\n\n\n    def reader_method():\n        with session_for_read() as sess:\n            print(\n                sess.execute(select(1)).scalar()\n            )\n\n    def writer_method():\n        with session_for_write() as sess:\n            print(\n                sess.execute(select(1)).scalar()\n            )\n\n    reader_method()","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"2b2bf3ca23bfeeb5b07b4339f142fe81c2548543","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"de7a0472_cc5c5829","in_reply_to":"43ed76b4_ffb5ca9e","updated":"2024-08-21 11:43:40.000000000","message":"Here is one https://paste.opendev.org/show/bEa54iK6ElVAFRB6nZBY/","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"09601cfdd8dc3876185063bc7fa58719167fe890","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"42a8b571_c39157c6","in_reply_to":"5457673f_0c7075a8","updated":"2024-08-21 13:43:07.000000000","message":"that will be a pain but can probably be done using pifpaf to create two mysql servers, then setup replication between them\n\nconfigure first as connection and second as slave_connection\n\nthen simply stopping the primary database and testing any auth operations (openstack token issue) that is not writing to db\n\ni\u0027m hacking this in a test environment directly, other potential is cooking it up with devstack","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"68ee6c38326124c6a970c175db289f8e322bb5ad","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"fc6a0508_373a5637","in_reply_to":"6bb55900_40a8b3b3","updated":"2024-08-21 12:31:33.000000000","message":"that stack trace is coming from a keystone method that claims to be using the reader session.   It has nothing to do with _start(), which was already called.  \n\n1. that\u0027s not actually the \"reader\" because we are seeing a required behavior of enginefacade which is that a \"reader\" method can be upgraded to a \"writer\" if the enclosing context is \"writer\"\n2. that\u0027s not actually the \"reader\" and something is going wrong in keystone and/or enginefacade when things get set up\n3. the \"reader\" db is offline\n\n\nSee this is the thing, if enginefacade is not working as I expected, I really need to fix that first.  or if keystone is setting things up in a way that breaks assumptions in enginefacade, we need to fix that on one or both ends.  in particular the change in logic that is occurring in the _create_connection() method of your patch I\u0027m not really sure is correct, and might be breaking an expected behavior if in fact this method is supposed to be upgrading to the writer session.","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"c48ad8c102c2cb379a10e028d4c7ef301acd40a3","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"b82196b0_d2c209b7","in_reply_to":"825499f3_1f89204f","updated":"2024-08-20 11:43:11.000000000","message":"So I want to get us on the same page first, because it\u0027s not clear to me that we are.   I want us to establish a common truth first here regarding the max_retries setting which I think should be deprecated, and which I think *either* keystone, *or* enginefacade right here (!) should hardcode to zero right now to solve this issue (and if it doesnt solve the issue, then I\u0027m missing something about this code, which is very possible!)\n\n\u003e We always do setup_connection() in _start() for the writer connection in _TransactionFactory even if _WRITER mode was never explicitly requested.\n\nyup, I know\n\n\u003e If we have a slave_connection configured, that is used when _READER mode\nis requested but we still try to connect to the _WRITER database when\nwe do start in the factory.\n\nthe key word here is \"connect\" which here means, set up a TCP socket to a MySQL database using pymysql.connect().   enginefacade does not want to \"connect\" in _start(), this \"connect\" should not happen to the degree that oslo.db is written in the spirit of how SQLAlchemy works.  The only reason that _start() would \"connect\" is because of this \"max_retries\" setting that forces oslo_db.sqlalchemy.create_engine to actually make a database connection.\n\nPlease note that SQLAlchemy\u0027s create_engine() call **does not connect to the database**.  It is a passive call that constructs some objects.  That\u0027s what I want oslo_db.sqlalchemy.create_engine to do, and if max_retries were turned off, that\u0027s what it would do, last time I worked with this.\n\nNow, if there\u0027s a whole reason that no, we really want max_retries on, and we really want our create_engine() to immediately connect so that connectivity errors are found at some specific spot, I\u0027d want to see that - *HOWEVER*, that argument itself is legacy and predates the current enginefacade.  Because, *our _start() method is also lazy-initializing*.\n\nSo to reiterate my current take, I\u0027d prefer that _start() does not have the secondary effect of actually connecting to the database.   because it really shouldn\u0027t.   now if that\u0027s impossible for *reasons* let\u0027s hear those, but I think _start() could just hardcode max_retries\u003d0 to oslo_db.sqlalchemy.create_engine because it\u0027s lazy-initializing in any case.    this is a simpler solution to the problem by removing unnecessary complexity and side effects, rather than adding more code.","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"b14c50b7f3fa310444338a4155f75a36bfd3ed6b","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"825499f3_1f89204f","in_reply_to":"b4cd36c2_6796eaf3","updated":"2024-08-20 06:26:20.000000000","message":"Thanks for looking into this, I could use some feedback, let me try to clear things up.\n\nWe always do `setup_connection()` in `_start()` for the writer connection in `_TransactionFactory` even if `_WRITER` mode was never explicitly requested.\n\nIf we have a slave_connection configured, that is used when `_READER` mode\nis requested but we still try to connect to the `_WRITER` database when\nwe do start in the factory.\n\nIf you take this into account for the Keystone API, where lets say 95% of all\nrequests is authentication that only needs to read the database, we would be doing hundreds/thousands (depends on the amount of requests and how workers handle them) of database connection attempts to the primary database when it\u0027s down even though the requests coming in only needs to be able to read from the database configured in `slave_connection`.\n\nNow if you have a badly configured `max_retries` and `retry_interval` (really the defaults) it doesn\u0027t really matter that you have `slave_connection` because you will likely running out of workers for your requests because all of them will hang and eventually eat up your workers.\n\nLets say you change `max_retries\u003d3` and `retry_interval\u003d1` now atleast your authentication API requests will be successful after 3 seconds when the primary database connection fails, and your workers will be hung up for 3 seconds but still proccess after a while, but 3 seconds is forever in computing/authentication time.\n\nWhat we could do is to try to honor what mode was requested (_READER or _WRITER) and get a lesser penalty when only _READER is needed.\n\nDo you have any feedback/ideas on a path forward for improving the above?\n\nI\u0027m thinking this was never in the oslo.db contract in the first place and it\u0027s more like:\n\n\u003e The database configured in connection must always be available, slave_connection can be configured to handle all read queries.\n\nAnd what I\u0027m thinking here is more:\n\n\u003e The database configured in connection must be available for write queries, slave_connection can be configured to handle all read queries even when write queries is unavailable.","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"f56810e2deeab12a5bbfeae5c5064cc4ac5485b8","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"2e7e7c22_3dc042e8","in_reply_to":"b82196b0_d2c209b7","updated":"2024-08-20 14:40:24.000000000","message":"Thanks for the detailed explanation, I think you\u0027re describing exactly what I\u0027m after and we\u0027re aligned on the goal here, I just had no idea that `max_retries` itself was the culprit here.\n\nI will see if I can get some time available to test it out and verify, thanks!","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"abf73f945396a107f2e81f3369bbd9cbd16f33e9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"43ed76b4_ffb5ca9e","in_reply_to":"be27ebcf_6cffc455","updated":"2024-08-21 11:21:15.000000000","message":"can you share the stack trace from the application server when that happens?  that will show the database error received as well as where the non working writer connection originates and I need to see how that\u0027s happening.  with the max retries feature turned off there should be no connectivity in the start method.    so maybe something is happening somewhere else.\n\nbasically I want enginefacade to work a certain way here and as stated, the goal of my other patch is to ensure that it does .","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"fedbb951ff9278d6b73c625db72c52bd82507338","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"6bb55900_40a8b3b3","in_reply_to":"de7a0472_cc5c5829","updated":"2024-08-21 11:46:04.000000000","message":"Just for reference as well this is running inside httpd mod_wsgi so I think a worker lives long enough for it to handle both a read and write operation.","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"001a372181e02ec678fcac6257900a669c967bfa","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c7db41ff_fae02423","in_reply_to":"e1b73816_bfa075ce","updated":"2024-08-21 14:47:25.000000000","message":"Ok, thanks! Changing in Keystone solves my use-case and I can just go with that for now.","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"1a864bdb9d28867aa7c03ac148e49579960afccb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0f28149c_c5e9751f","in_reply_to":"f1c5d911_28fe7b27","updated":"2024-08-21 14:16:50.000000000","message":"Thanks I will try this out and report back!","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"a09646a1b69c654a62d230b198106b9bc9df17ac","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"a264d506_5152feb0","in_reply_to":"fc6a0508_373a5637","updated":"2024-08-21 12:32:22.000000000","message":"edit for above, it\u0027s missing the line, \"the issue is one of these three things\"","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"}],"oslo_db/sqlalchemy/enginefacade.py":[{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"68ee6c38326124c6a970c175db289f8e322bb5ad","unresolved":true,"context_lines":[{"line_number":374,"context_line":"        return self._reader_maker"},{"line_number":375,"context_line":""},{"line_number":376,"context_line":"    def _is_reader(self, mode):"},{"line_number":377,"context_line":"        sync_reader \u003d self._facade_cfg[\u0027synchronous_reader\u0027]"},{"line_number":378,"context_line":"        return ("},{"line_number":379,"context_line":"            mode is _ASYNC_READER or"},{"line_number":380,"context_line":"            (mode is _READER and not sync_reader)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5141a2ff_be31dbb6","line":377,"updated":"2024-08-21 12:31:33.000000000","message":"this logic should not be changed here, the \"self.synchronous_reader\" attribute is what should be being read here.   I dont know that just changing this to ignore that attribute is correct.","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"7e09a7d4c607281605760bfcc2dd66de2ee82145","unresolved":true,"context_lines":[{"line_number":374,"context_line":"        return self._reader_maker"},{"line_number":375,"context_line":""},{"line_number":376,"context_line":"    def _is_reader(self, mode):"},{"line_number":377,"context_line":"        sync_reader \u003d self._facade_cfg[\u0027synchronous_reader\u0027]"},{"line_number":378,"context_line":"        return ("},{"line_number":379,"context_line":"            mode is _ASYNC_READER or"},{"line_number":380,"context_line":"            (mode is _READER and not sync_reader)"}],"source_content_type":"text/x-python","patch_set":1,"id":"71ae27bd_443f3ff8","line":377,"in_reply_to":"5141a2ff_be31dbb6","updated":"2024-08-21 14:04:51.000000000","message":"This is definitely wrong, this is also *reversing* the boolean.    please see Ia4b4dc14bc4163463ef4271851b117e36baae0ec for the correct changes to keystone","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"2aaae80f0142ad90891486318d79c5537c70375a","unresolved":true,"context_lines":[{"line_number":374,"context_line":"        return self._reader_maker"},{"line_number":375,"context_line":""},{"line_number":376,"context_line":"    def _is_reader(self, mode):"},{"line_number":377,"context_line":"        sync_reader \u003d self._facade_cfg[\u0027synchronous_reader\u0027]"},{"line_number":378,"context_line":"        return ("},{"line_number":379,"context_line":"            mode is _ASYNC_READER or"},{"line_number":380,"context_line":"            (mode is _READER and not sync_reader)"}],"source_content_type":"text/x-python","patch_set":1,"id":"d913cede_8c31b01c","line":377,"in_reply_to":"71ae27bd_443f3ff8","updated":"2024-08-21 14:22:04.000000000","message":"IIRC it was copied directly from existing code but maybe i messed it up somewhere","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"c44c882db5aa0b8966a9d2a582c4b76436c6a5f6","unresolved":true,"context_lines":[{"line_number":374,"context_line":"        return self._reader_maker"},{"line_number":375,"context_line":""},{"line_number":376,"context_line":"    def _is_reader(self, mode):"},{"line_number":377,"context_line":"        sync_reader \u003d self._facade_cfg[\u0027synchronous_reader\u0027]"},{"line_number":378,"context_line":"        return ("},{"line_number":379,"context_line":"            mode is _ASYNC_READER or"},{"line_number":380,"context_line":"            (mode is _READER and not sync_reader)"}],"source_content_type":"text/x-python","patch_set":1,"id":"e943ac28_e1526d6a","line":377,"in_reply_to":"d913cede_8c31b01c","updated":"2024-08-21 14:33:08.000000000","message":"this change is why we see the failures here, which is fortunate because the enginefacade test suite intends to be very robust, and these failures show the incorrect engine being selected for various scenarios:\n\nhttps://zuul.opendev.org/t/openstack/build/1a2490fd02a84196b623ea95930c2365\n\nwhere\u0027s the code this came from?  it is also be wrong if someone is hardcoding this somewhere","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"68ee6c38326124c6a970c175db289f8e322bb5ad","unresolved":true,"context_lines":[{"line_number":381,"context_line":"        )"},{"line_number":382,"context_line":""},{"line_number":383,"context_line":"    def _create_connection(self, mode):"},{"line_number":384,"context_line":"        if self._is_reader(mode):"},{"line_number":385,"context_line":"            if not self._reader_started:"},{"line_number":386,"context_line":"                self._reader_start()"},{"line_number":387,"context_line":"            return self._reader_engine.connect()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a793109_776a53ba","line":384,"updated":"2024-08-21 12:31:33.000000000","message":"I think the method here should not be changed structurally.   if we want to split _start() into reader and writer, it should do exactly that and nothing else.   That is, the method should read:\n\n    diff --git a/oslo_db/sqlalchemy/enginefacade.py b/oslo_db/sqlalchemy/enginefacade.py\n    index df8a872..ed5d595 100644\n    --- a/oslo_db/sqlalchemy/enginefacade.py\n    +++ b/oslo_db/sqlalchemy/enginefacade.py\n    @@ -385,14 +385,18 @@ class _TransactionFactory:\n            return self._reader_maker\n    \n        def _create_connection(self, mode):\n    -        if not self._started:\n    -            self._start()\n            if mode is _WRITER:\n    +            if not self._writer_started:\n    +                self._writer_start()\n                return self._writer_engine.connect()\n            elif mode is _ASYNC_READER or \\\n                    (mode is _READER and not self.synchronous_reader):\n    +            if not self._reader_started:\n    +                self._reader_start()\n                return self._reader_engine.connect()\n            else:\n    +            if not self._writer_started:\n    +                self._writer_start()\n                return self._writer_engine.connect()\n    \n        def _create_session(self, mode, bind\u003dNone):\n\n\nthat\u0027s it.  if that code change is not sufficient for the keystone error to go away then something else is going on which needs to be addressed.","commit_id":"74a5c7847204eaa48b6dcf5eb6f35f02997f3d49"}]}