)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"1960b0494dd911711b0b8b71a181f9466d5b51be","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"8bd5fbc0_8e3567ee","updated":"2022-07-29 18:27:11.000000000","message":"We obviously can\u0027t merge this as-is because turns out we *do* call instance.save() under a migration context, so our CI explodes on this patch, but yeah, we need to fix all those cases.","commit_id":"9794acfdabf1c014beb8f7d43dd6a3cee6c53f2a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"be0a140be89921302f6754dbcc3ad1f496779b0a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"96635c04_320333d3","updated":"2022-08-03 16:25:42.000000000","message":"If we\u0027re not able to avoid keeping instance.mutated_migration_context(), what if we were to make this more explicit all around? We could make it look like this:\n\n def mutated_migration_context(self):\n     with self.obj_alternate_context(None):\n         # other details removed here for brevity\n         self.apply_migration_context():\n         try:\n             yield\n         except:\n             self.revert_migration_context()\n             \nThat would mean that the instance was basically kneecapped while we\u0027re using it with the mutated context. It can\u0027t be save()\u0027d nor have anything lazy-loaded into it, etc during that time.\n\nTo me that would be cleaner and safer than the flag approach, which feels like it could get out of sync at some point (like obj_clone() doesn\u0027t know about it, for example).\n\nThat said, I\u0027d really rather remove the mutated_migration_context() entirely if we can eliminate the need for it in the last place.","commit_id":"03d3fb1368afa948aced1deb9f0173eb58bee7b8"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"dbda9e1fe19e502aa4c538ef15c2f3b43a39076c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"386f0b35_945f0bab","in_reply_to":"96635c04_320333d3","updated":"2022-08-03 16:30:13.000000000","message":"Sorry, my revert on except isn\u0027t the same behavior, which is unintended. Just trying to show the technique of hacking the context out to prevent a save() instead of the flag.","commit_id":"03d3fb1368afa948aced1deb9f0173eb58bee7b8"}],"nova/compute/manager.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8d2e8be403daad0f658d80eefe09133bf9aa4fe8","unresolved":true,"context_lines":[{"line_number":3793,"context_line":"            evacuate\u003devacuate,"},{"line_number":3794,"context_line":"            accel_uuids\u003daccel_uuids)"},{"line_number":3795,"context_line":"        try:"},{"line_number":3796,"context_line":"            with instance.mutated_migration_context():"},{"line_number":3797,"context_line":"                self.driver.rebuild(**kwargs)"},{"line_number":3798,"context_line":"        except NotImplementedError:"},{"line_number":3799,"context_line":"            # NOTE(rpodolyaka): driver doesn\u0027t provide specialized version"},{"line_number":3800,"context_line":"            # of rebuild, fall back to the default implementation"}],"source_content_type":"text/x-python","patch_set":3,"id":"0cdc6651_a9aa10e2","line":3797,"range":{"start_line":3796,"start_character":12,"end_line":3797,"end_character":45},"updated":"2022-08-02 14:06:09.000000000","message":"testing shows that this is also a problematic place where the ironic virt driver saves the instance while under the mutated migration context.\n\nI think (but I need confirmation from others) in this case what we really want is:\n1) apply the migration context\n2) call the virt driver.rebuild() to do what its needed including saving the instance\n3a) if the driver does not raise any exception then we are done\n3b) if the driver raises then we need to revert the migration context in the except block and potentially save the instance again.\n\nDo you agree?","commit_id":"d7c104384d857b97e388f8e280edddb468f1f165"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b9a3812d391de722be59cbe42d82d3ee3c11dc55","unresolved":true,"context_lines":[{"line_number":3793,"context_line":"            evacuate\u003devacuate,"},{"line_number":3794,"context_line":"            accel_uuids\u003daccel_uuids)"},{"line_number":3795,"context_line":"        try:"},{"line_number":3796,"context_line":"            with instance.mutated_migration_context():"},{"line_number":3797,"context_line":"                self.driver.rebuild(**kwargs)"},{"line_number":3798,"context_line":"        except NotImplementedError:"},{"line_number":3799,"context_line":"            # NOTE(rpodolyaka): driver doesn\u0027t provide specialized version"},{"line_number":3800,"context_line":"            # of rebuild, fall back to the default implementation"}],"source_content_type":"text/x-python","patch_set":3,"id":"151656cb_bbf33695","line":3797,"range":{"start_line":3796,"start_character":12,"end_line":3797,"end_character":45},"in_reply_to":"0cdc6651_a9aa10e2","updated":"2022-08-02 14:58:27.000000000","message":"I don\u0027t know the answer to this without digging further into the problem, but what you described here seems fine to me if it solves the problem.","commit_id":"d7c104384d857b97e388f8e280edddb468f1f165"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"e11981767e896e4bb46fb61466f99dc67c67ebc9","unresolved":false,"context_lines":[{"line_number":3793,"context_line":"            evacuate\u003devacuate,"},{"line_number":3794,"context_line":"            accel_uuids\u003daccel_uuids)"},{"line_number":3795,"context_line":"        try:"},{"line_number":3796,"context_line":"            with instance.mutated_migration_context():"},{"line_number":3797,"context_line":"                self.driver.rebuild(**kwargs)"},{"line_number":3798,"context_line":"        except NotImplementedError:"},{"line_number":3799,"context_line":"            # NOTE(rpodolyaka): driver doesn\u0027t provide specialized version"},{"line_number":3800,"context_line":"            # of rebuild, fall back to the default implementation"}],"source_content_type":"text/x-python","patch_set":3,"id":"d54746a6_684a54e1","line":3797,"range":{"start_line":3796,"start_character":12,"end_line":3797,"end_character":45},"in_reply_to":"151656cb_bbf33695","updated":"2022-08-02 15:25:52.000000000","message":"Ack. Done.","commit_id":"d7c104384d857b97e388f8e280edddb468f1f165"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"be0a140be89921302f6754dbcc3ad1f496779b0a","unresolved":true,"context_lines":[{"line_number":3799,"context_line":"            # NOTE(rpodolyaka): driver doesn\u0027t provide specialized version"},{"line_number":3800,"context_line":"            # of rebuild, fall back to the default implementation"},{"line_number":3801,"context_line":"            try:"},{"line_number":3802,"context_line":"                self._rebuild_default_impl(**kwargs)"},{"line_number":3803,"context_line":"            except Exception:"},{"line_number":3804,"context_line":"                # the @reverts_task_state decorator will save the instance"},{"line_number":3805,"context_line":"                # state. So we have to make sure that the migration context is"}],"source_content_type":"text/x-python","patch_set":7,"id":"4963d071_0d3a89e7","line":3802,"updated":"2022-08-03 16:25:42.000000000","message":"This will attempt to use mutated_migration_context(), but we\u0027ve already applied it right?","commit_id":"03d3fb1368afa948aced1deb9f0173eb58bee7b8"}],"nova/objects/instance.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"be0a140be89921302f6754dbcc3ad1f496779b0a","unresolved":true,"context_lines":[{"line_number":780,"context_line":""},{"line_number":781,"context_line":"    # TODO(stephenfin): Remove the \u0027admin_state_reset\u0027 field in version 3.0 of"},{"line_number":782,"context_line":"    # the object"},{"line_number":783,"context_line":"    @forbid_under_mutated_migration_context"},{"line_number":784,"context_line":"    @base.remotable"},{"line_number":785,"context_line":"    def save(self, expected_vm_state\u003dNone,"},{"line_number":786,"context_line":"             expected_task_state\u003dNone, admin_state_reset\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":7,"id":"b02f62a5_e28b6eb1","line":783,"updated":"2022-08-03 16:25:42.000000000","message":"It seems like if we remove the mutation in rebuild_default_impl(), there will only be one more place where we use mutated_migration_context(), in _cleanup_incomplete_migrations(). Wouldn\u0027t it be cleaner to just eliminate that as well and remove the problematic context manager altogether? We know it\u0027s risky, it has always been fragile, why not just close the door?","commit_id":"03d3fb1368afa948aced1deb9f0173eb58bee7b8"}],"nova/virt/ironic/driver.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8d2e8be403daad0f658d80eefe09133bf9aa4fe8","unresolved":true,"context_lines":[{"line_number":1675,"context_line":"        LOG.debug(\u0027Rebuild called for instance\u0027, instance\u003dinstance)"},{"line_number":1676,"context_line":""},{"line_number":1677,"context_line":"        instance.task_state \u003d task_states.REBUILD_SPAWNING"},{"line_number":1678,"context_line":"        instance.save(expected_task_state\u003d[task_states.REBUILDING])"},{"line_number":1679,"context_line":""},{"line_number":1680,"context_line":"        node_uuid \u003d instance.node"},{"line_number":1681,"context_line":"        node \u003d self._get_node(node_uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"bb33af7a_f594b218","line":1678,"updated":"2022-08-02 14:06:09.000000000","message":"this save happens under a mutated migration context. We should not do either this save or having a mutated migration context.","commit_id":"d7c104384d857b97e388f8e280edddb468f1f165"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"e11981767e896e4bb46fb61466f99dc67c67ebc9","unresolved":true,"context_lines":[{"line_number":1675,"context_line":"        LOG.debug(\u0027Rebuild called for instance\u0027, instance\u003dinstance)"},{"line_number":1676,"context_line":""},{"line_number":1677,"context_line":"        instance.task_state \u003d task_states.REBUILD_SPAWNING"},{"line_number":1678,"context_line":"        instance.save(expected_task_state\u003d[task_states.REBUILDING])"},{"line_number":1679,"context_line":""},{"line_number":1680,"context_line":"        node_uuid \u003d instance.node"},{"line_number":1681,"context_line":"        node \u003d self._get_node(node_uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"f9faa2e3_7e226339","line":1678,"in_reply_to":"022f6fb4_231bb85f","updated":"2022-08-02 15:25:52.000000000","message":"That is a good question. \n\nThe libvirt driver calls instance.save() at 5 places. The ironic virt driver does it in 3 places. So the validity of generic contract would need more investigation.\n\nThe specific contract for the rebuild itself seems to be backwards. The manager._do_rebuild_instance() expects that the driver.rebuild or if that not exists then the manager._rebuild_default_impl puts the instance into REBUILD_SPAWNING state before returns. \nSee https://review.opendev.org/c/openstack/nova/+/850746/3/nova/compute/manager.py#3803 where the expected_task_state is used to express this.\n\nbottom line, while such contract is attracting I don\u0027t think we ever had such contract in place and also I don\u0027t think we can make it without invalidating the assumptions of the out of tree virt drivers.","commit_id":"d7c104384d857b97e388f8e280edddb468f1f165"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b9a3812d391de722be59cbe42d82d3ee3c11dc55","unresolved":true,"context_lines":[{"line_number":1675,"context_line":"        LOG.debug(\u0027Rebuild called for instance\u0027, instance\u003dinstance)"},{"line_number":1676,"context_line":""},{"line_number":1677,"context_line":"        instance.task_state \u003d task_states.REBUILD_SPAWNING"},{"line_number":1678,"context_line":"        instance.save(expected_task_state\u003d[task_states.REBUILDING])"},{"line_number":1679,"context_line":""},{"line_number":1680,"context_line":"        node_uuid \u003d instance.node"},{"line_number":1681,"context_line":"        node \u003d self._get_node(node_uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"022f6fb4_231bb85f","line":1678,"in_reply_to":"bb33af7a_f594b218","updated":"2022-08-02 14:58:27.000000000","message":"Yeah, is there any reason not to just remove this save() and the other related ones? I think there\u0027s probably no reason not to just have the contract with the virt driver be \"the compute manager will save() the instance after calling driver.rebuild()\". Meaning, is that rebuild code called elsewhere which can\u0027t rely on that contract?","commit_id":"d7c104384d857b97e388f8e280edddb468f1f165"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"eda5f5006b24129699b288ee07cd46930036f540","unresolved":true,"context_lines":[{"line_number":1675,"context_line":"        LOG.debug(\u0027Rebuild called for instance\u0027, instance\u003dinstance)"},{"line_number":1676,"context_line":""},{"line_number":1677,"context_line":"        instance.task_state \u003d task_states.REBUILD_SPAWNING"},{"line_number":1678,"context_line":"        instance.save(expected_task_state\u003d[task_states.REBUILDING])"},{"line_number":1679,"context_line":""},{"line_number":1680,"context_line":"        node_uuid \u003d instance.node"},{"line_number":1681,"context_line":"        node \u003d self._get_node(node_uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba5811b1_99a971da","line":1678,"in_reply_to":"f9faa2e3_7e226339","updated":"2022-08-02 16:54:31.000000000","message":"Oh, but the good news is, out of tree virt drivers have no contract, so we can\u0027t possibly break it :)","commit_id":"d7c104384d857b97e388f8e280edddb468f1f165"}]}