)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f6349f95fadea024a74431dcb179a6541d2255f6","unresolved":false,"context_lines":[{"line_number":12,"context_line":"for handling untargeted local database accesses in nova-compute with"},{"line_number":13,"context_line":"the logic being, if nova-api targeted the cell2 database, it is likely"},{"line_number":14,"context_line":"that the next untargeted database access will be by nova-compute as"},{"line_number":15,"context_line":"part of the same request, thus the same cell database can be assumed."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"However, since then, we\u0027ve had improvements to the CellDatabases"},{"line_number":18,"context_line":"fixture [1][2] which make the old hack unnecessary. As one of the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7faddb67_ae6fa290","line":15,"range":{"start_line":15,"start_character":40,"end_line":15,"end_character":44},"updated":"2019-07-24 22:27:56.000000000","message":"cell2","commit_id":"ffdef64323852810deeefb91e6ee655cb73b81e9"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8aeccf833510cc27b599e14ebc0b16fd47011aa6","unresolved":false,"context_lines":[{"line_number":14,"context_line":"that the next untargeted database access will be by nova-compute as"},{"line_number":15,"context_line":"part of the same request, thus the same cell database can be assumed."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"However, since then, we\u0027ve had improvements to the CellDatabases"},{"line_number":18,"context_line":"fixture [1][2] which make the old hack unnecessary. As one of the"},{"line_number":19,"context_line":"to-be-removed code comments states, the presence of this hack can hide"},{"line_number":20,"context_line":"some (not all, because of \"default context manager\") bugs where we are"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7faddb67_a6dfad0a","line":17,"range":{"start_line":17,"start_character":31,"end_line":17,"end_character":43},"updated":"2019-07-26 14:30:33.000000000","message":"I assume you\u0027re referring mostly to the handing of a targeted context to Service, and thus compute manager in [2]. The problem with relying on that is that if *any* code in any path that ends up being called by compute manager just does context.get_admin_context(), they\u0027ll end up pointing at the wrong database. There may not by any such path currently (although there may be and we\u0027re just getting lucky in the execution of this patch), but it would be trivial to add one in the future and have things start failing in a confusing way.","commit_id":"ffdef64323852810deeefb91e6ee655cb73b81e9"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cb67ec583571356774ab95cb272b86ccb5630ea3","unresolved":false,"context_lines":[{"line_number":14,"context_line":"that the next untargeted database access will be by nova-compute as"},{"line_number":15,"context_line":"part of the same request, thus the same cell database can be assumed."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"However, since then, we\u0027ve had improvements to the CellDatabases"},{"line_number":18,"context_line":"fixture [1][2] which make the old hack unnecessary. As one of the"},{"line_number":19,"context_line":"to-be-removed code comments states, the presence of this hack can hide"},{"line_number":20,"context_line":"some (not all, because of \"default context manager\") bugs where we are"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7faddb67_f77c7121","line":17,"range":{"start_line":17,"start_character":31,"end_line":17,"end_character":43},"in_reply_to":"7faddb67_a6dfad0a","updated":"2019-07-26 17:58:55.000000000","message":"Wasn\u0027t only mostly talking about the targeted context to Service, I also think the use of CheatingSerializer is the better way to take care of untargeted get_admin_context(), for example, in the compute manager.\n\nCheatingSerializer accomplishes the same thing that last context manager set out to do, which is essentially carry the db_connection target from nova-api to nova-compute. That\u0027s the main reason why I created last context manager when I first wrote CellDatabases. But once CheatingSerializer came on the scene, it mostly removed the usefulness of last context manager. And then once the targeted-context-to-Service change landed, it removed all usefulness that I was aware of.\n\nThe only way (I\u0027m aware of) that a get_admin_context() could end up targeting the wrong database, is if someone runs the periodic_tasks manually in a func test without using target_cell around the periodic_tasks call. Periodic tasks do not run automatically because of our ConfFixture setting periodic_enable\u003dFalse:\n\nhttps://github.com/openstack/nova/blob/d358df32f7a2a1125962e27f2425359c768f79c1/nova/tests/unit/conf_fixture.py#L44","commit_id":"ffdef64323852810deeefb91e6ee655cb73b81e9"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"6349c5702596d8b93c8e69959512a04aa01db247","unresolved":false,"context_lines":[{"line_number":14,"context_line":"that the next untargeted database access will be by nova-compute as"},{"line_number":15,"context_line":"part of the same request, thus the same cell database can be assumed."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"However, since then, we\u0027ve had improvements to the CellDatabases"},{"line_number":18,"context_line":"fixture [1][2] which make the old hack unnecessary. As one of the"},{"line_number":19,"context_line":"to-be-removed code comments states, the presence of this hack can hide"},{"line_number":20,"context_line":"some (not all, because of \"default context manager\") bugs where we are"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7faddb67_ade308a4","line":17,"range":{"start_line":17,"start_character":31,"end_line":17,"end_character":43},"in_reply_to":"7faddb67_b21a8714","updated":"2019-07-26 19:49:24.000000000","message":"No, you\u0027re correct. I was missing this point before.","commit_id":"ffdef64323852810deeefb91e6ee655cb73b81e9"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"0deeb0c6caa1eb817ccf1f614588f9f5267e5d83","unresolved":false,"context_lines":[{"line_number":14,"context_line":"that the next untargeted database access will be by nova-compute as"},{"line_number":15,"context_line":"part of the same request, thus the same cell database can be assumed."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"However, since then, we\u0027ve had improvements to the CellDatabases"},{"line_number":18,"context_line":"fixture [1][2] which make the old hack unnecessary. As one of the"},{"line_number":19,"context_line":"to-be-removed code comments states, the presence of this hack can hide"},{"line_number":20,"context_line":"some (not all, because of \"default context manager\") bugs where we are"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7faddb67_b21a8714","line":17,"range":{"start_line":17,"start_character":31,"end_line":17,"end_character":43},"in_reply_to":"7faddb67_f77c7121","updated":"2019-07-26 18:25:18.000000000","message":"\u003e Wasn\u0027t only mostly talking about the targeted context to Service, I\n \u003e also think the use of CheatingSerializer is the better way to take\n \u003e care of untargeted get_admin_context(), for example, in the compute\n \u003e manager.\n \u003e \n \u003e CheatingSerializer accomplishes the same thing that last context\n \u003e manager set out to do, which is essentially carry the db_connection\n \u003e target from nova-api to nova-compute.\n\nIt does for requests, but if I make a call to compute, CheatingSerializer will hand compute a targeted context. But then if compute (or some code compute uses) calls get_admin_context() (instead of context.elevated()) it will get a context pointing at the default instead of the targeted one, unless I\u0027m misremembering something.","commit_id":"ffdef64323852810deeefb91e6ee655cb73b81e9"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8aeccf833510cc27b599e14ebc0b16fd47011aa6","unresolved":false,"context_lines":[{"line_number":19,"context_line":"to-be-removed code comments states, the presence of this hack can hide"},{"line_number":20,"context_line":"some (not all, because of \"default context manager\") bugs where we are"},{"line_number":21,"context_line":"not targeting a request context to a cell when we should be targeting"},{"line_number":22,"context_line":"it."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"I came across the bug hiding problem again while trying to func test a"},{"line_number":25,"context_line":"patch for making \u0027nova-manage db archive_deleted_rows\u0027 multi-cell"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7faddb67_a4433313","line":22,"updated":"2019-07-26 14:30:33.000000000","message":"This is the primary complaint, right? That we can add new code to the api that does not target a cell and it\u0027s hard to notice that right? If the default cell for api stuff is cell0, then I would think things would only go unnoticed if they don\u0027t do anything with computes (or are overly mocked). Maybe we have tests being added that choose cell1 (i.e. not cell0) as the default? If the compute manager is using the service context targeted at cell1, then I would think we should be setting the default cell in the figure to cell0 (like it should be in real life) and then we\u0027d catch the cases where api targets the default cell by default.","commit_id":"ffdef64323852810deeefb91e6ee655cb73b81e9"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cb67ec583571356774ab95cb272b86ccb5630ea3","unresolved":false,"context_lines":[{"line_number":19,"context_line":"to-be-removed code comments states, the presence of this hack can hide"},{"line_number":20,"context_line":"some (not all, because of \"default context manager\") bugs where we are"},{"line_number":21,"context_line":"not targeting a request context to a cell when we should be targeting"},{"line_number":22,"context_line":"it."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"I came across the bug hiding problem again while trying to func test a"},{"line_number":25,"context_line":"patch for making \u0027nova-manage db archive_deleted_rows\u0027 multi-cell"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7faddb67_f726b116","line":22,"in_reply_to":"7faddb67_a4433313","updated":"2019-07-26 17:58:55.000000000","message":"That is the primary concern, yes. As we discussed on IRC today, my case was a special one where code in nova/cmd/manage.py targeted a cell, then called a db api method that was not passing a RequestContext in a direct get_engine() call. So each target_cell saved last context manager and the RequestContext-less get_engine() call erroneously got a targeted context via last context manager.\n\nSo the default cell never came into play in this case. But in general, I agree that cell0 would be a better default than our current default: cell1, for the reasons you describe.","commit_id":"ffdef64323852810deeefb91e6ee655cb73b81e9"}],"nova/tests/fixtures.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8aeccf833510cc27b599e14ebc0b16fd47011aa6","unresolved":false,"context_lines":[{"line_number":502,"context_line":"                self._last_ctxt_mgr \u003d desired"},{"line_number":503,"context_line":""},{"line_number":504,"context_line":"        with self._cell_lock.read_lock():"},{"line_number":505,"context_line":"            if self._last_ctxt_mgr !\u003d desired:"},{"line_number":506,"context_line":"                # NOTE(danms): This is unlikely to happen, but it\u0027s possible"},{"line_number":507,"context_line":"                # another waiting writer changed the state between us letting"},{"line_number":508,"context_line":"                # it go and re-acquiring as a reader. If lockutils supported"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_6659b533","side":"PARENT","line":505,"updated":"2019-07-26 14:30:33.000000000","message":"I know this is the targeted thing to remove, but it\u0027s what helped me track down all the places where we were hitting the database from the compute node but not with a request context. If we remove this, we lose that sanity checking. I\u0027m afraid to do that because at real runtime, everything in the compute node is supposed to be untargeted and hitting the compute\u0027s default database. In the tests we can\u0027t have that be different from the api\u0027s default, so we\u0027re attempting to always target a context (but only in the tests) to get around that. If we\u0027re going to do something like this, I think we need to find a way to catch these leaks if they get added in the future.","commit_id":"5fece42f9f431a672a1ec48324f55b12c2d49060"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"0deeb0c6caa1eb817ccf1f614588f9f5267e5d83","unresolved":false,"context_lines":[{"line_number":502,"context_line":"                self._last_ctxt_mgr \u003d desired"},{"line_number":503,"context_line":""},{"line_number":504,"context_line":"        with self._cell_lock.read_lock():"},{"line_number":505,"context_line":"            if self._last_ctxt_mgr !\u003d desired:"},{"line_number":506,"context_line":"                # NOTE(danms): This is unlikely to happen, but it\u0027s possible"},{"line_number":507,"context_line":"                # another waiting writer changed the state between us letting"},{"line_number":508,"context_line":"                # it go and re-acquiring as a reader. If lockutils supported"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_d2c38356","side":"PARENT","line":505,"in_reply_to":"7faddb67_37b76989","updated":"2019-07-26 18:25:18.000000000","message":"I don\u0027t think it will, per my other reply. The straight path from api-\u003ecompute-\u003ereturn is easy. API targets a cell, CheatingSerializer keeps it targeted for the compute, compute uses it, done. It\u0027s the case where compute calls something, which calls something, which calls something else, which calls some innocent utility method that ... oopsie, calls get_admin_context() and then you don\u0027t have a targeted context.\n\nBut, there are definitely dragons in both paths, which I think we agree on :)","commit_id":"5fece42f9f431a672a1ec48324f55b12c2d49060"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cb67ec583571356774ab95cb272b86ccb5630ea3","unresolved":false,"context_lines":[{"line_number":502,"context_line":"                self._last_ctxt_mgr \u003d desired"},{"line_number":503,"context_line":""},{"line_number":504,"context_line":"        with self._cell_lock.read_lock():"},{"line_number":505,"context_line":"            if self._last_ctxt_mgr !\u003d desired:"},{"line_number":506,"context_line":"                # NOTE(danms): This is unlikely to happen, but it\u0027s possible"},{"line_number":507,"context_line":"                # another waiting writer changed the state between us letting"},{"line_number":508,"context_line":"                # it go and re-acquiring as a reader. If lockutils supported"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_37b76989","side":"PARENT","line":505,"in_reply_to":"7faddb67_6659b533","updated":"2019-07-26 17:58:55.000000000","message":"IIUC, your case would be covered by CheatingSerializer. See my other comments.","commit_id":"5fece42f9f431a672a1ec48324f55b12c2d49060"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"6349c5702596d8b93c8e69959512a04aa01db247","unresolved":false,"context_lines":[{"line_number":502,"context_line":"                self._last_ctxt_mgr \u003d desired"},{"line_number":503,"context_line":""},{"line_number":504,"context_line":"        with self._cell_lock.read_lock():"},{"line_number":505,"context_line":"            if self._last_ctxt_mgr !\u003d desired:"},{"line_number":506,"context_line":"                # NOTE(danms): This is unlikely to happen, but it\u0027s possible"},{"line_number":507,"context_line":"                # another waiting writer changed the state between us letting"},{"line_number":508,"context_line":"                # it go and re-acquiring as a reader. If lockutils supported"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_edd900f4","side":"PARENT","line":505,"in_reply_to":"7faddb67_d2c38356","updated":"2019-07-26 19:49:24.000000000","message":"Thanks for clarifying the example you had in mind. I, for whatever reason, was missing it before.","commit_id":"5fece42f9f431a672a1ec48324f55b12c2d49060"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8aeccf833510cc27b599e14ebc0b16fd47011aa6","unresolved":false,"context_lines":[{"line_number":543,"context_line":""},{"line_number":544,"context_line":"        # NOTE(melwitt): This is a hack to try to deal with"},{"line_number":545,"context_line":"        # local accesses i.e. non target_cell accesses."},{"line_number":546,"context_line":"        with self._cell_lock.read_lock():"},{"line_number":547,"context_line":"            # FIXME(mriedem): This is actually misleading and means we don\u0027t"},{"line_number":548,"context_line":"            # catch things like bug 1717000 where a context should be targeted"},{"line_number":549,"context_line":"            # to a cell but it\u0027s not, and the fixture here just returns the"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_046d6797","side":"PARENT","line":546,"updated":"2019-07-26 14:30:33.000000000","message":"This is the thing that protects us against hitting the wrong database by some deeply-nested piece of code that grabs its own context. In the new code, you\u0027ll return the default context manager, which is going to be for cell0 (or something other than what we want). Either that means we\u0027ll write something into the wrong database, then read from it and not realize it\u0027s wrong, or we\u0027ll crash looking for something we swear should be in there but isn\u0027t.","commit_id":"5fece42f9f431a672a1ec48324f55b12c2d49060"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cb67ec583571356774ab95cb272b86ccb5630ea3","unresolved":false,"context_lines":[{"line_number":543,"context_line":""},{"line_number":544,"context_line":"        # NOTE(melwitt): This is a hack to try to deal with"},{"line_number":545,"context_line":"        # local accesses i.e. non target_cell accesses."},{"line_number":546,"context_line":"        with self._cell_lock.read_lock():"},{"line_number":547,"context_line":"            # FIXME(mriedem): This is actually misleading and means we don\u0027t"},{"line_number":548,"context_line":"            # catch things like bug 1717000 where a context should be targeted"},{"line_number":549,"context_line":"            # to a cell but it\u0027s not, and the fixture here just returns the"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_d7de15d9","side":"PARENT","line":546,"in_reply_to":"7faddb67_046d6797","updated":"2019-07-26 17:58:55.000000000","message":"I think the scenario you describe would be covered by the behavior of CheatingSerializer (carries RequestContext.db_connection from nova-api to all the other services in the call/cast chain), unless you\u0027re talking about func tests containing manual periodic_tasks run calls that are untargeted.","commit_id":"5fece42f9f431a672a1ec48324f55b12c2d49060"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8aeccf833510cc27b599e14ebc0b16fd47011aa6","unresolved":false,"context_lines":[{"line_number":442,"context_line":"    @contextmanager"},{"line_number":443,"context_line":"    def _wrap_target_cell(self, context, cell_mapping):"},{"line_number":444,"context_line":"        with self._real_target_cell(context, cell_mapping) as ccontext:"},{"line_number":445,"context_line":"            yield ccontext"},{"line_number":446,"context_line":""},{"line_number":447,"context_line":"    def _wrap_create_context_manager(self, connection\u003dNone):"},{"line_number":448,"context_line":"        ctxt_mgr \u003d self._ctxt_mgrs[connection]"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_64f3fbe8","line":445,"updated":"2019-07-26 14:30:33.000000000","message":"What is the point of even having this wrapper anymore? You\u0027re just proxying straight through to the real one now, so no need to mock it anymore right?","commit_id":"ffdef64323852810deeefb91e6ee655cb73b81e9"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cb67ec583571356774ab95cb272b86ccb5630ea3","unresolved":false,"context_lines":[{"line_number":442,"context_line":"    @contextmanager"},{"line_number":443,"context_line":"    def _wrap_target_cell(self, context, cell_mapping):"},{"line_number":444,"context_line":"        with self._real_target_cell(context, cell_mapping) as ccontext:"},{"line_number":445,"context_line":"            yield ccontext"},{"line_number":446,"context_line":""},{"line_number":447,"context_line":"    def _wrap_create_context_manager(self, connection\u003dNone):"},{"line_number":448,"context_line":"        ctxt_mgr \u003d self._ctxt_mgrs[connection]"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_1767cd53","line":445,"in_reply_to":"7faddb67_64f3fbe8","updated":"2019-07-26 17:58:55.000000000","message":"Yeah, good call. Brain lapse.","commit_id":"ffdef64323852810deeefb91e6ee655cb73b81e9"}]}