)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"4e022941fda3eb5c25c84c2d19d6c9fcb51d7a0b","unresolved":true,"context_lines":[{"line_number":47,"context_line":" # on destination host, we rely on instance object to cleanup"},{"line_number":48,"context_line":" # specific resources like vpmem"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"However I don\u0027t see any vpmem related cleanup in this code path neither"},{"line_number":51,"context_line":"now nor in the original patches introduced the mutation. Also by"},{"line_number":52,"context_line":"removing the mutation no vpmem specific unit of functional test fails."},{"line_number":53,"context_line":"So I conclude that this mutation is simply not necessary."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"b0b5b33f_2bbd6279","line":50,"updated":"2022-08-02 15:40:40.000000000","message":"That\u0027s not exactly true. If you trace the code in the libvirt driver, it does:\n\n  rollback_live_migration_at_destination()\n  self.destroy()\n  self.cleanup()\n  self._cleanup()\n  \n  # zero the data on backend pmem device\n  vpmems \u003d self._get_vpmems(instance)\n  if vpmems:\n    self._cleanup_vpmems(vpmems)\n    \n_get_vpmems() does accept a `prefix` kwarg, so maybe we can just have rollback_live_migration_at_destination() pass down a flag/prefix/something down to self._cleanup() to grab the `new_` prefix to clean up the correct vpmems.\n\nI wonder if similar logs elsewhere, like for volumes, but a quick look didn\u0027t turn up anything.","commit_id":"0d56d1cc085635be1c25a1b7287caa5effd80f09"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8e22b67a651e2d3e41fc800831c414678433005f","unresolved":false,"context_lines":[{"line_number":47,"context_line":" # on destination host, we rely on instance object to cleanup"},{"line_number":48,"context_line":" # specific resources like vpmem"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"However I don\u0027t see any vpmem related cleanup in this code path neither"},{"line_number":51,"context_line":"now nor in the original patches introduced the mutation. Also by"},{"line_number":52,"context_line":"removing the mutation no vpmem specific unit of functional test fails."},{"line_number":53,"context_line":"So I conclude that this mutation is simply not necessary."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"7471efd7_1ea96add","line":50,"in_reply_to":"774cc4a2_5dabc4d9","updated":"2022-08-03 13:14:04.000000000","message":"Looking at the code more I decided not to pass down yet another flag via both driver.destroy and driver.cleanup. Instead I can avoid relying on the instance.resources being correct during clenaup. The instance.resources is used to know which vpmem devices needs to be cleaned. But such information is also present in the libvirt domain, without the need to account for any migration context.","commit_id":"0d56d1cc085635be1c25a1b7287caa5effd80f09"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"db22971dbdf15a45d2eca11ece76f7effceddf05","unresolved":true,"context_lines":[{"line_number":47,"context_line":" # on destination host, we rely on instance object to cleanup"},{"line_number":48,"context_line":" # specific resources like vpmem"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"However I don\u0027t see any vpmem related cleanup in this code path neither"},{"line_number":51,"context_line":"now nor in the original patches introduced the mutation. Also by"},{"line_number":52,"context_line":"removing the mutation no vpmem specific unit of functional test fails."},{"line_number":53,"context_line":"So I conclude that this mutation is simply not necessary."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"774cc4a2_5dabc4d9","line":50,"in_reply_to":"b0b5b33f_2bbd6279","updated":"2022-08-03 08:43:18.000000000","message":"thanks for the pointer I did missed that vpmems cleanup. It is sad that we had zero test coverage on that code path. \n\nI will look at the possibility to pass down a flag...","commit_id":"0d56d1cc085635be1c25a1b7287caa5effd80f09"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"84af6bf616f5d14a9ed79553f21340c42c37f1fa","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"922a6357_cfe9acac","updated":"2022-08-02 09:36:19.000000000","message":"WIP as per TODO in the commit message","commit_id":"b5e6a66bdc093b7ab91c2856513b915d14da0354"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"1603571a740cbd3df9cd04d0aa04bfba4ca6aef5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"4b71b8cd_977e3926","updated":"2022-08-02 13:39:26.000000000","message":"I absolutely do not think we should do anything like this. It sounds like setting the migration context on the instance just to call the driver method is the first hack, and this compounds that by mangling the instance object to avoid a save, which is a bad idea, IMHO.\n\nIt sounds to me like some refactoring of the driver method and/or relationship between it and compute manager is what needs to happen, instead of this hackery.","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bbf6bb053b8572b215975c9385da967f60e00936","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"fd04e7ea_15f15af3","updated":"2022-08-03 16:11:58.000000000","message":"A couple questions inline, but definitely better than the first alternative, thanks.","commit_id":"0b4107029650b687f012ab9519d68bd866349b54"}],"nova/compute/manager.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bbf6bb053b8572b215975c9385da967f60e00936","unresolved":true,"context_lines":[{"line_number":9397,"context_line":"            # NOTE(luyao): Apply migration_context temporarily since it\u0027s"},{"line_number":9398,"context_line":"            # on destination host, we rely on instance object to cleanup"},{"line_number":9399,"context_line":"            # specific resources like vpmem"},{"line_number":9400,"context_line":"            with instance.mutated_migration_context():"},{"line_number":9401,"context_line":"                self.driver.rollback_live_migration_at_destination("},{"line_number":9402,"context_line":"                    context, instance, network_info, block_device_info,"},{"line_number":9403,"context_line":"                    destroy_disks\u003ddestroy_disks, migrate_data\u003dmigrate_data)"}],"source_content_type":"text/x-python","patch_set":5,"id":"96aea45a_39258d47","side":"PARENT","line":9400,"updated":"2022-08-03 16:11:58.000000000","message":"Definitely seems like a big improvement if we can get away from this hacky (and risky) temporary mutation altogether.","commit_id":"11710cd1c43d865ba5df7e02e62ea30595500aea"}],"nova/objects/instance.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"1603571a740cbd3df9cd04d0aa04bfba4ca6aef5","unresolved":true,"context_lines":[{"line_number":772,"context_line":"                \u0027by that mutation. The following changes are not saved: %s\u0027,"},{"line_number":773,"context_line":"                self.obj_what_changed(), instance\u003dself,"},{"line_number":774,"context_line":"            )"},{"line_number":775,"context_line":"            return"},{"line_number":776,"context_line":""},{"line_number":777,"context_line":"        return self._remote_save("},{"line_number":778,"context_line":"            expected_vm_state, expected_task_state, admin_state_reset"}],"source_content_type":"text/x-python","patch_set":2,"id":"4fa33203_5a25f4cd","line":775,"updated":"2022-08-02 13:39:26.000000000","message":"This seems like a totally subversive hack and I don\u0027t think we should do anything like this. A random flag that says \"don\u0027t persist data to the database even when asked\"? Really not great... :(","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8e22b67a651e2d3e41fc800831c414678433005f","unresolved":false,"context_lines":[{"line_number":772,"context_line":"                \u0027by that mutation. The following changes are not saved: %s\u0027,"},{"line_number":773,"context_line":"                self.obj_what_changed(), instance\u003dself,"},{"line_number":774,"context_line":"            )"},{"line_number":775,"context_line":"            return"},{"line_number":776,"context_line":""},{"line_number":777,"context_line":"        return self._remote_save("},{"line_number":778,"context_line":"            expected_vm_state, expected_task_state, admin_state_reset"}],"source_content_type":"text/x-python","patch_set":2,"id":"e3a53084_8f158880","line":775,"in_reply_to":"4fa33203_5a25f4cd","updated":"2022-08-03 13:14:04.000000000","message":"OK, I\u0027m convinced. Changed direction and removed the mutation instead.","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"b4a19b750aac710cdba9df38a5970a26de4c273d","unresolved":true,"context_lines":[{"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":"    @base.remotable"},{"line_number":784,"context_line":"    def _remote_save("},{"line_number":785,"context_line":"        self,"},{"line_number":786,"context_line":"        expected_vm_state\u003dNone,"},{"line_number":787,"context_line":"        expected_task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"238f3626_c0d67ba1","line":784,"updated":"2022-08-02 12:06:40.000000000","message":"Dan Smith can keep me honest here, but I think this is going to be a problem for rolling minor updates.\n\nImagine the case wherein this patch is backported to wallaby, and someone is doing a update to their wallaby deployment. Because we don\u0027t enforce ordering for updates, a compute host gets this patch, but not the conductor.\n\nThe compute calls _save() locally, which tries to call the remote _remote_save(), which doesn\u0027t exist.","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"1603571a740cbd3df9cd04d0aa04bfba4ca6aef5","unresolved":true,"context_lines":[{"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":"    @base.remotable"},{"line_number":784,"context_line":"    def _remote_save("},{"line_number":785,"context_line":"        self,"},{"line_number":786,"context_line":"        expected_vm_state\u003dNone,"},{"line_number":787,"context_line":"        expected_task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"d606c7ec_6e56f662","line":784,"in_reply_to":"238f3626_c0d67ba1","updated":"2022-08-02 13:39:26.000000000","message":"Yeah, you can\u0027t change remotable methods without a major version bump. That\u0027s why you\u0027re changing the hash and not the version, which is also not allowed.","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"cff07f36658cee9faceb2513a8aa693c301cf8fd","unresolved":true,"context_lines":[{"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":"    @base.remotable"},{"line_number":784,"context_line":"    def _remote_save("},{"line_number":785,"context_line":"        self,"},{"line_number":786,"context_line":"        expected_vm_state\u003dNone,"},{"line_number":787,"context_line":"        expected_task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"4c7ed62e_346802b0","line":784,"in_reply_to":"238f3626_c0d67ba1","updated":"2022-08-02 13:35:51.000000000","message":"if the conductor is upgraded first then the remote end will have both save and _remote_save so both the old an the new compute can have its remote function exists.\n\nIf it make less scary then I can replace the save -\u003e _remote_save redirection with a decorator that does the same thing but keep the save unchanged.","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"6ecfbbb64b094bea82a65aa58950af8bec12d995","unresolved":true,"context_lines":[{"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":"    @base.remotable"},{"line_number":784,"context_line":"    def _remote_save("},{"line_number":785,"context_line":"        self,"},{"line_number":786,"context_line":"        expected_vm_state\u003dNone,"},{"line_number":787,"context_line":"        expected_task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"d0644536_63a9ce4c","line":784,"in_reply_to":"4c7ed62e_346802b0","updated":"2022-08-02 13:45:20.000000000","message":"As long as the conductor gets the new code first (which should always be the case for major version upgrades) we\u0027ll be fine.\n\nThe concern is only if we backport this to a stable release, because for \"minor\" updates within the same release I\u0027m not aware of any requirement to update controller-tier first, so compute hosts could conceivably end up running code that\u0027s not yet deployed in the conductor.\n\nI think a decorator would be... fragile? As in, the local forbid-save-under-migration-context decorator needs to be called first, before the remotable decorator. I\u0027m sure there\u0027s a way to achieve that, but I *think* it\u0027s done according to the order the decorators are called in, so if we flip two lines, we\u0027d break stuff:\n\n  @remotable\n  @forbid_save_under_migration_context\n  \nIs not the same as\n\n  @forbid_save_under_migration_context\n  @remotable","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8e22b67a651e2d3e41fc800831c414678433005f","unresolved":false,"context_lines":[{"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":"    @base.remotable"},{"line_number":784,"context_line":"    def _remote_save("},{"line_number":785,"context_line":"        self,"},{"line_number":786,"context_line":"        expected_vm_state\u003dNone,"},{"line_number":787,"context_line":"        expected_task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fc96dfff_6b6f2e4d","line":784,"in_reply_to":"598cd1b0_354e7174","updated":"2022-08-03 13:14:04.000000000","message":"removed the mutation instead. The decorator idea is still useful, thanks, and used in the follow up patch.","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"b400d05c05a42fb851ed5e195ddf15a04d718e09","unresolved":true,"context_lines":[{"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":"    @base.remotable"},{"line_number":784,"context_line":"    def _remote_save("},{"line_number":785,"context_line":"        self,"},{"line_number":786,"context_line":"        expected_vm_state\u003dNone,"},{"line_number":787,"context_line":"        expected_task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"598cd1b0_354e7174","line":784,"in_reply_to":"d0644536_63a9ce4c","updated":"2022-08-02 13:47:45.000000000","message":"\u003e Further, however, as soon as the conductor is upgraded, you\u0027ve broke save for any of the un-upgraded computes because *they* will no longer be able to save, since the method does not exist.\n\nAh, didn\u0027t think of that. So yeah, no way to make this work without an RCP bump, and a deprecation period, and no way to remove the remotable save() until the next major RPC version.","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"2cb2b68d5c41b3bbf4d012fd01c8119bc0c904c6","unresolved":true,"context_lines":[{"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":"    @base.remotable"},{"line_number":784,"context_line":"    def _remote_save("},{"line_number":785,"context_line":"        self,"},{"line_number":786,"context_line":"        expected_vm_state\u003dNone,"},{"line_number":787,"context_line":"        expected_task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"00afb2f9_8b7bbc0f","line":784,"in_reply_to":"d606c7ec_6e56f662","updated":"2022-08-02 13:44:53.000000000","message":"gibi responded while I was crafting my reply.\n\nArtom\u0027s right, and is talking about minor updates, which can be applied in any order, which this would break. Further, however, as soon as the conductor is upgraded, you\u0027ve broke save for any of the un-upgraded computes because *they* will no longer be able to save, since the method does not exist.\n\nI\u0027m -2 on even the decorator approach because I think this is a bad idea, regardless of the mechanics.","commit_id":"3ebb0ff5c5362581af93e48f23e7f5a2f2c829c4"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bbf6bb053b8572b215975c9385da967f60e00936","unresolved":true,"context_lines":[{"line_number":1616,"context_line":"        # zero the data on backend pmem device"},{"line_number":1617,"context_line":"        vpmems \u003d self._get_vpmems(instance)"},{"line_number":1618,"context_line":"        if vpmems:"},{"line_number":1619,"context_line":"            self._cleanup_vpmems(vpmems)"},{"line_number":1620,"context_line":""},{"line_number":1621,"context_line":"        if destroy_vifs:"},{"line_number":1622,"context_line":"            self._unplug_vifs(instance, network_info, True)"}],"source_content_type":"text/x-python","patch_set":5,"id":"c3ebd009_8717f3cc","side":"PARENT","line":1619,"updated":"2022-08-03 16:11:58.000000000","message":"This is still used in one location, so I understand why it\u0027s not removed. However, wouldn\u0027t it be cleaner to just make _cleanup_vpmems() do the instance-\u003edomain-\u003ecleanup_vpmem thing all the time and avoid having basically two copies of this? Meaning, is there a reason not to always cleanup by domain?","commit_id":"11710cd1c43d865ba5df7e02e62ea30595500aea"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bbf6bb053b8572b215975c9385da967f60e00936","unresolved":true,"context_lines":[{"line_number":1603,"context_line":"            try:"},{"line_number":1604,"context_line":"                nova.privsep.libvirt.cleanup_vpmem(vpmem.source_path)"},{"line_number":1605,"context_line":"            except Exception as e:"},{"line_number":1606,"context_line":"                raise exception.VPMEMCleanupFailed("},{"line_number":1607,"context_line":"                    dev\u003dvpmem.source_path, error\u003de)"},{"line_number":1608,"context_line":""},{"line_number":1609,"context_line":"    def _cleanup(self, context, instance, network_info, block_device_info\u003dNone,"}],"source_content_type":"text/x-python","patch_set":5,"id":"84e06b87_fbd40382","line":1606,"updated":"2022-08-03 16:11:58.000000000","message":"I know this mirrors the existing behavior, but is it really right to abort the loop here? Seems like this is potentially security-sensitive and we should record the error(s), continue trying to clean up each vpmem, and then raise the first one we hit after doing so, no?","commit_id":"0b4107029650b687f012ab9519d68bd866349b54"}]}