)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dbce7d11ab42987d8892591b0d76b7c071c77606","unresolved":false,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":"The solution to this issue is pretty simple. Instead of unsetting the"},{"line_number":28,"context_line":"old flavor record from the migration at the start of the various move"},{"line_number":29,"context_line":"operations, do it afterwards."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"[1] https://github.com/openstack/nova/blob/6557d67/nova/compute/resource_tracker.py#L1288"},{"line_number":32,"context_line":"[2] https://github.com/openstack/nova/blob/6557d67/nova/compute/manager.py#L4310-L4315"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"9f560f44_44156a3f","line":29,"updated":"2020-08-06 22:03:41.000000000","message":"Really appreciate this detailed explanation of the problem and proposed solution, complete with references so I can step through the code path and understand the what/why/how. Makes review so much easier.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"}],"nova/compute/manager.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"fda90c37854d2a47b4c3676f005b7d1266700c81","unresolved":false,"context_lines":[{"line_number":4271,"context_line":"                    self._delete_scheduler_instance_info("},{"line_number":4272,"context_line":"                        context, instance.uuid)"},{"line_number":4273,"context_line":"                    # The cached flavor information is confusing and useless so"},{"line_number":4274,"context_line":"                    # unset it"},{"line_number":4275,"context_line":"                    self._delete_stashed_migration_information(instance)"},{"line_number":4276,"context_line":""},{"line_number":4277,"context_line":"        do_confirm_resize(context, instance, migration)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_6dbfc660","line":4274,"updated":"2020-08-05 23:45:43.000000000","message":"I\u0027d recommend an example or other context here instead of \"confusing and useless\" for the code comment. And if it\u0027s important when it\u0027s done, mention that too.\n\nI do wonder about the ordering of these steps wrt how they might potentially race in other ways. (And would want to explain in a code comment why we delete the data here vs somewhere else, if we determine the order matters). I will look through it more tomorrow.","commit_id":"a851ebd45846480e2817741739b955ab493049d2"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"76da80bff88bd8473a0796793a80f5295d9916c1","unresolved":false,"context_lines":[{"line_number":4271,"context_line":"                    self._delete_scheduler_instance_info("},{"line_number":4272,"context_line":"                        context, instance.uuid)"},{"line_number":4273,"context_line":"                    # The cached flavor information is confusing and useless so"},{"line_number":4274,"context_line":"                    # unset it"},{"line_number":4275,"context_line":"                    self._delete_stashed_migration_information(instance)"},{"line_number":4276,"context_line":""},{"line_number":4277,"context_line":"        do_confirm_resize(context, instance, migration)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_383a25e6","line":4274,"in_reply_to":"9f560f44_6dbfc660","updated":"2020-08-06 09:53:24.000000000","message":"Done","commit_id":"a851ebd45846480e2817741739b955ab493049d2"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dbce7d11ab42987d8892591b0d76b7c071c77606","unresolved":false,"context_lines":[{"line_number":4205,"context_line":"    @wrap_instance_event(prefix\u003d\u0027compute\u0027)"},{"line_number":4206,"context_line":"    @errors_out_migration"},{"line_number":4207,"context_line":"    @wrap_instance_fault"},{"line_number":4208,"context_line":"    def confirm_resize(self, context, instance, migration):"},{"line_number":4209,"context_line":"        \"\"\"Confirms a migration/resize and deletes the \u0027old\u0027 instance."},{"line_number":4210,"context_line":""},{"line_number":4211,"context_line":"        This is called from the API and runs on the source host."}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_a42d6696","line":4208,"updated":"2020-08-06 22:03:41.000000000","message":"Note to self: here we\u0027re deleting the stashed flavor info at the end of confirm_resize.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dbce7d11ab42987d8892591b0d76b7c071c77606","unresolved":false,"context_lines":[{"line_number":4684,"context_line":"        try:"},{"line_number":4685,"context_line":"            do_revert()"},{"line_number":4686,"context_line":"        finally:"},{"line_number":4687,"context_line":"            self._delete_stashed_migration_information(instance)"},{"line_number":4688,"context_line":""},{"line_number":4689,"context_line":"        # Broadcast to all schedulers that the instance is on this host."},{"line_number":4690,"context_line":"        # This is best effort so if anything fails just log it."}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_0456d206","line":4687,"updated":"2020-08-06 22:03:41.000000000","message":"Note to self: deleting stashed flavor info at the end of a revert of a snapshot-based resize.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dbce7d11ab42987d8892591b0d76b7c071c77606","unresolved":false,"context_lines":[{"line_number":4717,"context_line":"            \u0027old_vm_state\u0027, vm_states.ACTIVE)"},{"line_number":4718,"context_line":""},{"line_number":4719,"context_line":"        # Revert the flavor and host/node fields to their previous values"},{"line_number":4720,"context_line":"        self._set_instance_info(instance, instance.old_flavor)"},{"line_number":4721,"context_line":"        instance.host \u003d migration.source_compute"},{"line_number":4722,"context_line":"        instance.node \u003d migration.source_node"},{"line_number":4723,"context_line":"        instance.save(expected_task_state\u003d[task_states.RESIZE_REVERTING])"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_2495d687","line":4720,"updated":"2020-08-06 22:03:41.000000000","message":"Note to self: this sets the instance.flavor and instance.vcpus etc fields to that of the old flavor.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dbce7d11ab42987d8892591b0d76b7c071c77606","unresolved":false,"context_lines":[{"line_number":4932,"context_line":"                self._finish_revert_resize("},{"line_number":4933,"context_line":"                    context, instance, migration, request_spec)"},{"line_number":4934,"context_line":"        finally:"},{"line_number":4935,"context_line":"            self._delete_stashed_migration_information(instance)"},{"line_number":4936,"context_line":""},{"line_number":4937,"context_line":"    def _finish_revert_resize("},{"line_number":4938,"context_line":"        self,"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_2448362b","line":4935,"updated":"2020-08-06 22:03:41.000000000","message":"Note to self: deleting stashed flavor info at the end of revert resize.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dbce7d11ab42987d8892591b0d76b7c071c77606","unresolved":false,"context_lines":[{"line_number":4953,"context_line":"        # Get stashed old_vm_state information to determine if guest should"},{"line_number":4954,"context_line":"        # be powered on after spawn; we default to ACTIVE for backwards"},{"line_number":4955,"context_line":"        # compatibility if old_vm_state is not set"},{"line_number":4956,"context_line":"        old_vm_state \u003d instance.system_metadata.get("},{"line_number":4957,"context_line":"            \u0027old_vm_state\u0027, vm_states.ACTIVE)"},{"line_number":4958,"context_line":""},{"line_number":4959,"context_line":"        # Revert the flavor and host/node fields to their previous values"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_04cb72a4","line":4956,"range":{"start_line":4956,"start_character":48,"end_line":4956,"end_character":51},"updated":"2020-08-06 22:03:41.000000000","message":"What is the reason for changing this from pop() to get()? AFAICT the intention was to remove the \u0027old_vm_state\u0027 from the system_metadata as part of this.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"a6b48cd868e12da88e02f5e5908c56cc20af859d","unresolved":false,"context_lines":[{"line_number":4953,"context_line":"        # Get stashed old_vm_state information to determine if guest should"},{"line_number":4954,"context_line":"        # be powered on after spawn; we default to ACTIVE for backwards"},{"line_number":4955,"context_line":"        # compatibility if old_vm_state is not set"},{"line_number":4956,"context_line":"        old_vm_state \u003d instance.system_metadata.get("},{"line_number":4957,"context_line":"            \u0027old_vm_state\u0027, vm_states.ACTIVE)"},{"line_number":4958,"context_line":""},{"line_number":4959,"context_line":"        # Revert the flavor and host/node fields to their previous values"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_40f70d8c","line":4956,"range":{"start_line":4956,"start_character":48,"end_line":4956,"end_character":51},"in_reply_to":"9f560f44_04cb72a4","updated":"2020-08-07 13:38:08.000000000","message":"We do this as part of the \u0027_delete_stashed_migration_information\u0027, so there\u0027s no reason to do it here. I forgot to update the other instance of this though. Done.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"9ab871a3540ea67dde0c6ab4435dad1b672b31eb","unresolved":false,"context_lines":[{"line_number":4953,"context_line":"        # Get stashed old_vm_state information to determine if guest should"},{"line_number":4954,"context_line":"        # be powered on after spawn; we default to ACTIVE for backwards"},{"line_number":4955,"context_line":"        # compatibility if old_vm_state is not set"},{"line_number":4956,"context_line":"        old_vm_state \u003d instance.system_metadata.get("},{"line_number":4957,"context_line":"            \u0027old_vm_state\u0027, vm_states.ACTIVE)"},{"line_number":4958,"context_line":""},{"line_number":4959,"context_line":"        # Revert the flavor and host/node fields to their previous values"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_29362604","line":4956,"range":{"start_line":4956,"start_character":48,"end_line":4956,"end_character":51},"in_reply_to":"9f560f44_40f70d8c","updated":"2020-08-10 19:15:04.000000000","message":"Ah, I see, I had missed that it was removed in _delete_stashed_migration_information. Thanks.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dbce7d11ab42987d8892591b0d76b7c071c77606","unresolved":false,"context_lines":[{"line_number":4960,"context_line":"        self._set_instance_info(instance, instance.old_flavor)"},{"line_number":4961,"context_line":"        instance.host \u003d migration.source_compute"},{"line_number":4962,"context_line":"        instance.node \u003d migration.source_node"},{"line_number":4963,"context_line":"        instance.save(expected_task_state\u003d[task_states.RESIZE_REVERTING])"},{"line_number":4964,"context_line":""},{"line_number":4965,"context_line":"        try:"},{"line_number":4966,"context_line":"            source_allocations \u003d self._revert_allocation("}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_c4397aa4","line":4963,"updated":"2020-08-06 22:03:41.000000000","message":"Shouldn\u0027t we have kept the helper function (and modify and rename it) for doing this part to keep this DRYer?","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"9ab871a3540ea67dde0c6ab4435dad1b672b31eb","unresolved":false,"context_lines":[{"line_number":4960,"context_line":"        self._set_instance_info(instance, instance.old_flavor)"},{"line_number":4961,"context_line":"        instance.host \u003d migration.source_compute"},{"line_number":4962,"context_line":"        instance.node \u003d migration.source_node"},{"line_number":4963,"context_line":"        instance.save(expected_task_state\u003d[task_states.RESIZE_REVERTING])"},{"line_number":4964,"context_line":""},{"line_number":4965,"context_line":"        try:"},{"line_number":4966,"context_line":"            source_allocations \u003d self._revert_allocation("}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_491c3a77","line":4963,"in_reply_to":"9f560f44_200db98f","updated":"2020-08-10 19:15:04.000000000","message":"I hear you, but this sequence does look to me like that could possibly get tweaked in the future and better to have updated in one place vs multiple. Just MHO.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"a6b48cd868e12da88e02f5e5908c56cc20af859d","unresolved":false,"context_lines":[{"line_number":4960,"context_line":"        self._set_instance_info(instance, instance.old_flavor)"},{"line_number":4961,"context_line":"        instance.host \u003d migration.source_compute"},{"line_number":4962,"context_line":"        instance.node \u003d migration.source_node"},{"line_number":4963,"context_line":"        instance.save(expected_task_state\u003d[task_states.RESIZE_REVERTING])"},{"line_number":4964,"context_line":""},{"line_number":4965,"context_line":"        try:"},{"line_number":4966,"context_line":"            source_allocations \u003d self._revert_allocation("}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_200db98f","line":4963,"in_reply_to":"9f560f44_c4397aa4","updated":"2020-08-07 13:38:08.000000000","message":"I don\u0027t particularly like those small, 2-5 line functions that have side effects, since they force me to jump around the code figuring out what\u0027s going on. Given there are only two callers, the little bit of duplication seemed worth it to preserve context. With that said, it\u0027s not a big deal so done","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"cf695b55ed23e4c084a2ea28233c179df917882e","unresolved":false,"context_lines":[{"line_number":4387,"context_line":"                       \u0027migration_uuid\u0027: migration.uuid})"},{"line_number":4388,"context_line":"            raise"},{"line_number":4389,"context_line":""},{"line_number":4390,"context_line":"    def _delete_stashed_migration_information(self, instance):"},{"line_number":4391,"context_line":"        \"\"\"Remove information about the flavor change after a resize.\"\"\""},{"line_number":4392,"context_line":"        instance.old_flavor \u003d None"},{"line_number":4393,"context_line":"        instance.new_flavor \u003d None"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_e643bd3d","line":4390,"updated":"2020-08-10 17:06:39.000000000","message":"Might just be me, but this method seems confusingly named. I would expect it to be a thing we\u0027re doing on a migration, but it\u0027s really just the instance-focused stuff, and in most cases, it\u0027ll be only meaningful for a resize. If it weren\u0027t for the sysmeta change, I\u0027d suggest \"_delete_stashed_flavor_info()\" or something. Anyway, just a nit I guess, since I don\u0027t have a great suggestion.","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"6ca9aaf02b70da6ca7c980744784be3c6417d6e9","unresolved":false,"context_lines":[{"line_number":4387,"context_line":"                       \u0027migration_uuid\u0027: migration.uuid})"},{"line_number":4388,"context_line":"            raise"},{"line_number":4389,"context_line":""},{"line_number":4390,"context_line":"    def _delete_stashed_migration_information(self, instance):"},{"line_number":4391,"context_line":"        \"\"\"Remove information about the flavor change after a resize.\"\"\""},{"line_number":4392,"context_line":"        instance.old_flavor \u003d None"},{"line_number":4393,"context_line":"        instance.new_flavor \u003d None"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_5a87d110","line":4390,"in_reply_to":"9f560f44_1a6399b3","updated":"2020-08-11 13:41:01.000000000","message":"Done","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ee297850bb1b0f57f570608b1a0b7f7b33e4e4c4","unresolved":false,"context_lines":[{"line_number":4387,"context_line":"                       \u0027migration_uuid\u0027: migration.uuid})"},{"line_number":4388,"context_line":"            raise"},{"line_number":4389,"context_line":""},{"line_number":4390,"context_line":"    def _delete_stashed_migration_information(self, instance):"},{"line_number":4391,"context_line":"        \"\"\"Remove information about the flavor change after a resize.\"\"\""},{"line_number":4392,"context_line":"        instance.old_flavor \u003d None"},{"line_number":4393,"context_line":"        instance.new_flavor \u003d None"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_1a6399b3","line":4390,"in_reply_to":"9f560f44_29c3c6ee","updated":"2020-08-11 10:34:21.000000000","message":"Yup, not married to this name. Will go with \u0027_delete_stashed_flavor_info\u0027","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"9ab871a3540ea67dde0c6ab4435dad1b672b31eb","unresolved":false,"context_lines":[{"line_number":4387,"context_line":"                       \u0027migration_uuid\u0027: migration.uuid})"},{"line_number":4388,"context_line":"            raise"},{"line_number":4389,"context_line":""},{"line_number":4390,"context_line":"    def _delete_stashed_migration_information(self, instance):"},{"line_number":4391,"context_line":"        \"\"\"Remove information about the flavor change after a resize.\"\"\""},{"line_number":4392,"context_line":"        instance.old_flavor \u003d None"},{"line_number":4393,"context_line":"        instance.new_flavor \u003d None"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_29c3c6ee","line":4390,"in_reply_to":"9f560f44_e643bd3d","updated":"2020-08-10 19:15:04.000000000","message":"I thought the exact same thing, typed up a comment on the previous PS, and then deleted it think it was probably just me. xD\n\nI suggested (in my deleted comment) something similar to \"_deleted_stashed_flavor_info\" to just be clear it\u0027s only about flavor info and not migration data/objects.","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"cf695b55ed23e4c084a2ea28233c179df917882e","unresolved":false,"context_lines":[{"line_number":4921,"context_line":"                self._finish_revert_resize("},{"line_number":4922,"context_line":"                    context, instance, migration, request_spec)"},{"line_number":4923,"context_line":"        finally:"},{"line_number":4924,"context_line":"            self._delete_stashed_migration_information(instance)"},{"line_number":4925,"context_line":""},{"line_number":4926,"context_line":"    def _finish_revert_resize("},{"line_number":4927,"context_line":"        self,"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_1291ccf1","line":4924,"updated":"2020-08-10 17:06:39.000000000","message":"While this refactor yields nicer code in the long run, it means that every line below is changed because of the indenting which makes it pretty hard to suss out the actual changed lines from the refactor. Is there some reason we can\u0027t just make the minimal change for backportability and then do your refactor after?\n\nLike, renaming this method to _finish_revert_resize() where it is and adding a new finish_revert_resize above it that has the decorator stack on it, and wraps the call to the inner with try...finally should result in *just* the lines of code you\u0027re actually changing being hit, right?","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ee297850bb1b0f57f570608b1a0b7f7b33e4e4c4","unresolved":false,"context_lines":[{"line_number":4921,"context_line":"                self._finish_revert_resize("},{"line_number":4922,"context_line":"                    context, instance, migration, request_spec)"},{"line_number":4923,"context_line":"        finally:"},{"line_number":4924,"context_line":"            self._delete_stashed_migration_information(instance)"},{"line_number":4925,"context_line":""},{"line_number":4926,"context_line":"    def _finish_revert_resize("},{"line_number":4927,"context_line":"        self,"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_5a12310b","line":4924,"in_reply_to":"9f560f44_1291ccf1","updated":"2020-08-11 10:34:21.000000000","message":"Sure, let me try that. It\u0027s unfortunate that the UI isn\u0027t properly picking up that this is mostly a simple dedenting. Normally it does an inserts spacer rows on one of the side (if you\u0027re using side-by-side view)","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"5fba9002f2ac62135f98e952f786480de5a6683f","unresolved":false,"context_lines":[{"line_number":4921,"context_line":"                self._finish_revert_resize("},{"line_number":4922,"context_line":"                    context, instance, migration, request_spec)"},{"line_number":4923,"context_line":"        finally:"},{"line_number":4924,"context_line":"            self._delete_stashed_migration_information(instance)"},{"line_number":4925,"context_line":""},{"line_number":4926,"context_line":"    def _finish_revert_resize("},{"line_number":4927,"context_line":"        self,"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_a0c6c048","line":4924,"in_reply_to":"9f560f44_5a12310b","updated":"2020-08-11 13:41:25.000000000","message":"Done","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"cf695b55ed23e4c084a2ea28233c179df917882e","unresolved":false,"context_lines":[{"line_number":4929,"context_line":"        instance: \u0027objects.Instance\u0027,"},{"line_number":4930,"context_line":"        migration: \u0027objects.Migration\u0027,"},{"line_number":4931,"context_line":"        request_spec: ty.Optional[\u0027objects.RequestSpec\u0027] \u003d None,"},{"line_number":4932,"context_line":"    ) -\u003e None:"},{"line_number":4933,"context_line":"        \"\"\"Inner version of finish_revert_resize.\"\"\""},{"line_number":4934,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":4935,"context_line":"            context, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_12fc2c3c","line":4932,"updated":"2020-08-10 17:06:39.000000000","message":"I just want to lodge my opinion that unenforced type annotations are harmful, IMHO. In addition to hurting readability (yes really), if they\u0027re wrong they\u0027re actively confusing the reader. I\u0027m sure everyone else loves them but me, but I definitely would vote to not have them in here.","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"6ca9aaf02b70da6ca7c980744784be3c6417d6e9","unresolved":false,"context_lines":[{"line_number":4929,"context_line":"        instance: \u0027objects.Instance\u0027,"},{"line_number":4930,"context_line":"        migration: \u0027objects.Migration\u0027,"},{"line_number":4931,"context_line":"        request_spec: ty.Optional[\u0027objects.RequestSpec\u0027] \u003d None,"},{"line_number":4932,"context_line":"    ) -\u003e None:"},{"line_number":4933,"context_line":"        \"\"\"Inner version of finish_revert_resize.\"\"\""},{"line_number":4934,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":4935,"context_line":"            context, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_9a396941","line":4932,"in_reply_to":"9f560f44_12fc2c3c","updated":"2020-08-11 13:41:01.000000000","message":"\u003e I just want to lodge my opinion that unenforced type annotations\n \u003e are harmful, IMHO. In addition to hurting readability (yes really),\n \u003e if they\u0027re wrong they\u0027re actively confusing the reader. I\u0027m sure\n \u003e everyone else loves them but me, but I definitely would vote to not\n \u003e have them in here.\n\nThe same argument can and has been made about comments in general. See \"good code is self-documenting\". We\u0027re not going to settle such philosophical debates in Gerrit comments, but I\u0027ll note that the plan is that we eventually will enforce all of these hints. Some are already enforced, in fact. For example, try to call \u0027nova.crypto.generate_fingerprint\u0027 with a bytestring or other non-string argument from a file and function that has type checking enabled right now and run mypy on that [1][2]. It has to be incremental though, because no one\u0027s going to review a giant patch to add type hints to everything in one fell swoop, and I seriously doubt I\u0027d even be able to churn that out in a remotely reasonable time.\n\n[1] http://paste.openstack.org/show/796725/\n[2] http://paste.openstack.org/show/796726/","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"9ab871a3540ea67dde0c6ab4435dad1b672b31eb","unresolved":false,"context_lines":[{"line_number":4929,"context_line":"        instance: \u0027objects.Instance\u0027,"},{"line_number":4930,"context_line":"        migration: \u0027objects.Migration\u0027,"},{"line_number":4931,"context_line":"        request_spec: ty.Optional[\u0027objects.RequestSpec\u0027] \u003d None,"},{"line_number":4932,"context_line":"    ) -\u003e None:"},{"line_number":4933,"context_line":"        \"\"\"Inner version of finish_revert_resize.\"\"\""},{"line_number":4934,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":4935,"context_line":"            context, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_a9aeb62a","line":4932,"in_reply_to":"9f560f44_12fc2c3c","updated":"2020-08-10 19:15:04.000000000","message":"I agree with you but it has seemed to me that the consensus is that everyone else wants these or at least doesn\u0027t mind them.","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ee297850bb1b0f57f570608b1a0b7f7b33e4e4c4","unresolved":false,"context_lines":[{"line_number":4929,"context_line":"        instance: \u0027objects.Instance\u0027,"},{"line_number":4930,"context_line":"        migration: \u0027objects.Migration\u0027,"},{"line_number":4931,"context_line":"        request_spec: ty.Optional[\u0027objects.RequestSpec\u0027] \u003d None,"},{"line_number":4932,"context_line":"    ) -\u003e None:"},{"line_number":4933,"context_line":"        \"\"\"Inner version of finish_revert_resize.\"\"\""},{"line_number":4934,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":4935,"context_line":"            context, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_9a45e918","line":4932,"in_reply_to":"9f560f44_a9aeb62a","updated":"2020-08-11 10:34:21.000000000","message":"I actually meant to drag these out to the follow-up (see top-level comment from me on PS2). Will do so for real in the next file","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"7bee80e86d81a33f632be9c1936a2e197e7269f3","unresolved":false,"context_lines":[{"line_number":4922,"context_line":""},{"line_number":4923,"context_line":"        Bring the original source instance state back (active/shutoff) and"},{"line_number":4924,"context_line":"        revert the resized attributes in the database."},{"line_number":4925,"context_line":""},{"line_number":4926,"context_line":"        \"\"\""},{"line_number":4927,"context_line":"        with self._error_out_instance_on_exception(context, instance):"},{"line_number":4928,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_3cc0a77c","side":"PARENT","line":4925,"updated":"2020-08-19 17:58:26.000000000","message":"Also just whitespace damage?","commit_id":"013d64bb8d4dc8b0438eca5edb542cde5350234d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bff7700684a43120c516abb452347843f1a93779","unresolved":false,"context_lines":[{"line_number":4922,"context_line":""},{"line_number":4923,"context_line":"        Bring the original source instance state back (active/shutoff) and"},{"line_number":4924,"context_line":"        revert the resized attributes in the database."},{"line_number":4925,"context_line":""},{"line_number":4926,"context_line":"        \"\"\""},{"line_number":4927,"context_line":"        with self._error_out_instance_on_exception(context, instance):"},{"line_number":4928,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_a792e4f4","side":"PARENT","line":4925,"in_reply_to":"9f560f44_3cc0a77c","updated":"2020-08-19 19:32:22.000000000","message":"as above","commit_id":"013d64bb8d4dc8b0438eca5edb542cde5350234d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"7bee80e86d81a33f632be9c1936a2e197e7269f3","unresolved":false,"context_lines":[{"line_number":4265,"context_line":"                    # in placement by deleting them here..."},{"line_number":4266,"context_line":"                    self._delete_allocation_after_move("},{"line_number":4267,"context_line":"                        context, instance, migration)"},{"line_number":4268,"context_line":"                    # ...inform the scheduler about the move..."},{"line_number":4269,"context_line":"                    self._delete_scheduler_instance_info("},{"line_number":4270,"context_line":"                        context, instance.uuid)"},{"line_number":4271,"context_line":"                    # ...and unset the cached flavor information (this is done"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_1c41630c","line":4268,"updated":"2020-08-19 17:58:26.000000000","message":"This and the comment change above don\u0027t seem to be changing the actual content, so I\u0027m not sure why we wouldn\u0027t just leave them especially for the backport...","commit_id":"f2a0fc6974fe51bdc2bc8f2e7a3dfb2c582d2bd7"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bff7700684a43120c516abb452347843f1a93779","unresolved":false,"context_lines":[{"line_number":4265,"context_line":"                    # in placement by deleting them here..."},{"line_number":4266,"context_line":"                    self._delete_allocation_after_move("},{"line_number":4267,"context_line":"                        context, instance, migration)"},{"line_number":4268,"context_line":"                    # ...inform the scheduler about the move..."},{"line_number":4269,"context_line":"                    self._delete_scheduler_instance_info("},{"line_number":4270,"context_line":"                        context, instance.uuid)"},{"line_number":4271,"context_line":"                    # ...and unset the cached flavor information (this is done"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_a707841c","line":4268,"in_reply_to":"9f560f44_1c41630c","updated":"2020-08-19 19:32:22.000000000","message":"This code as added in Ussuri (change If6cdd627f66a66c412f3c1d4997baab3552a9a9c) and hasn\u0027t been touched since, so we\u0027ll have no conflicts before that and then we\u0027ll have to drop this hunk anyway as we go back further. I can drop, but only if you think it\u0027s worse than it was before","commit_id":"f2a0fc6974fe51bdc2bc8f2e7a3dfb2c582d2bd7"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"7bee80e86d81a33f632be9c1936a2e197e7269f3","unresolved":false,"context_lines":[{"line_number":4910,"context_line":"    @wrap_instance_fault"},{"line_number":4911,"context_line":"    def finish_revert_resize("},{"line_number":4912,"context_line":"        self, context, instance, migration, request_spec\u003dNone,"},{"line_number":4913,"context_line":"    ):"},{"line_number":4914,"context_line":"        \"\"\"Finishes the second half of reverting a resize on the source host."},{"line_number":4915,"context_line":""},{"line_number":4916,"context_line":"        Bring the original source instance state back (active/shutoff) and"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_bcd4b748","line":4913,"updated":"2020-08-19 17:58:26.000000000","message":"This is just style damage, AFAICT. Not only does it not match the rest of the file (or project), but it\u0027s unrelated to the change, right?","commit_id":"f2a0fc6974fe51bdc2bc8f2e7a3dfb2c582d2bd7"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bff7700684a43120c516abb452347843f1a93779","unresolved":false,"context_lines":[{"line_number":4910,"context_line":"    @wrap_instance_fault"},{"line_number":4911,"context_line":"    def finish_revert_resize("},{"line_number":4912,"context_line":"        self, context, instance, migration, request_spec\u003dNone,"},{"line_number":4913,"context_line":"    ):"},{"line_number":4914,"context_line":"        \"\"\"Finishes the second half of reverting a resize on the source host."},{"line_number":4915,"context_line":""},{"line_number":4916,"context_line":"        Bring the original source instance state back (active/shutoff) and"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_478308aa","line":4913,"in_reply_to":"9f560f44_bcd4b748","updated":"2020-08-19 19:32:22.000000000","message":"Yup, sorry. I manually rewound the type hints and missed this, evidently","commit_id":"f2a0fc6974fe51bdc2bc8f2e7a3dfb2c582d2bd7"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"7bee80e86d81a33f632be9c1936a2e197e7269f3","unresolved":false,"context_lines":[{"line_number":4924,"context_line":""},{"line_number":4925,"context_line":"    def _finish_revert_resize("},{"line_number":4926,"context_line":"        self, context, instance, migration, request_spec\u003dNone,"},{"line_number":4927,"context_line":"    ):"},{"line_number":4928,"context_line":"        \"\"\"Inner version of finish_revert_resize.\"\"\""},{"line_number":4929,"context_line":"        with self._error_out_instance_on_exception(context, instance):"},{"line_number":4930,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_bc62f7ab","line":4927,"updated":"2020-08-19 17:58:26.000000000","message":":(","commit_id":"f2a0fc6974fe51bdc2bc8f2e7a3dfb2c582d2bd7"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bff7700684a43120c516abb452347843f1a93779","unresolved":false,"context_lines":[{"line_number":4924,"context_line":""},{"line_number":4925,"context_line":"    def _finish_revert_resize("},{"line_number":4926,"context_line":"        self, context, instance, migration, request_spec\u003dNone,"},{"line_number":4927,"context_line":"    ):"},{"line_number":4928,"context_line":"        \"\"\"Inner version of finish_revert_resize.\"\"\""},{"line_number":4929,"context_line":"        with self._error_out_instance_on_exception(context, instance):"},{"line_number":4930,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_276d3404","line":4927,"in_reply_to":"9f560f44_bc62f7ab","updated":"2020-08-19 19:32:22.000000000","message":"I can take or leave the inline stuff, but I really like this style for function signatures (where they can\u0027t fit on one line). It offers maximum horizontal space for arguments (useful for type hints especially) while maintaining a visual distinction between the function signature/definition and body, all for the cost of one measly additional line. No ugly hanging indents or double indents either and it\u0027s kosher per pep8. A win all-round, IMHO.","commit_id":"f2a0fc6974fe51bdc2bc8f2e7a3dfb2c582d2bd7"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"8677218b11cffcb7403e36965e99d703a2c83e52","unresolved":false,"context_lines":[{"line_number":4325,"context_line":"        self.driver.confirm_migration(context, migration, instance,"},{"line_number":4326,"context_line":"                                      network_info)"},{"line_number":4327,"context_line":""},{"line_number":4328,"context_line":"        migration.status \u003d \u0027confirmed\u0027"},{"line_number":4329,"context_line":"        migration.save()"},{"line_number":4330,"context_line":""},{"line_number":4331,"context_line":"        # NOTE(mriedem): drop_move_claim relies on"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_0a10eb4d","line":4328,"updated":"2020-08-20 09:32:29.000000000","message":"Here","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"4b8e5aa8ad9f37a71188acd3ac83fcedc6b46969","unresolved":false,"context_lines":[{"line_number":4332,"context_line":"        # instance.migration_context so make sure to not call"},{"line_number":4333,"context_line":"        # instance.drop_migration_context() until after drop_move_claim"},{"line_number":4334,"context_line":"        # is called."},{"line_number":4335,"context_line":"        self.rt.drop_move_claim("},{"line_number":4336,"context_line":"            context, instance, migration.source_node, instance.old_flavor,"},{"line_number":4337,"context_line":"            prefix\u003d\u0027old_\u0027)"},{"line_number":4338,"context_line":"        instance.drop_migration_context()"},{"line_number":4339,"context_line":""},{"line_number":4340,"context_line":"        # NOTE(mriedem): The old_vm_state could be STOPPED but the user"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_1b08402b","line":4337,"range":{"start_line":4335,"start_character":8,"end_line":4337,"end_character":26},"updated":"2020-08-20 08:30:50.000000000","message":"I\u0027m thinking the case: after this line, if the RT.update_available_resources is running again, then the usage of this migration will be count again.\n\nThe usage of this migration can be removed after the next around of update_available_resources.\n\nAnd thinking of more...\nafter this line, an new booting request are using the same pinned cpu as this confirmed instance. And the RT.update_available_resources is running again, then it conflicts.\n\nIf we removed stashed flavor info in the original place, that is a signal for RT no tracking this migration again, then this drop_move_claim just a call trigger usage update rather than waiting for next periodic task.\n\n\nSo I\u0027m thinking maybe we should keep the remove stashed flavor into in the original placement, but change rt.drop_move_claim to work with the case of update_available_resource already run.","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"8677218b11cffcb7403e36965e99d703a2c83e52","unresolved":false,"context_lines":[{"line_number":4332,"context_line":"        # instance.migration_context so make sure to not call"},{"line_number":4333,"context_line":"        # instance.drop_migration_context() until after drop_move_claim"},{"line_number":4334,"context_line":"        # is called."},{"line_number":4335,"context_line":"        self.rt.drop_move_claim("},{"line_number":4336,"context_line":"            context, instance, migration.source_node, instance.old_flavor,"},{"line_number":4337,"context_line":"            prefix\u003d\u0027old_\u0027)"},{"line_number":4338,"context_line":"        instance.drop_migration_context()"},{"line_number":4339,"context_line":""},{"line_number":4340,"context_line":"        # NOTE(mriedem): The old_vm_state could be STOPPED but the user"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_ca0a731e","line":4337,"range":{"start_line":4335,"start_character":8,"end_line":4337,"end_character":26},"in_reply_to":"9f560f44_1b08402b","updated":"2020-08-20 09:32:29.000000000","message":"You had me worried there. I think we\u0027re okay. As noted above, we\u0027ve already set the migration to \u0027confirmed\u0027 here. If \u0027update_available_resources\u0027 runs now, it will attempt to find all *in-progress* migrations [1] which means any whose status !\u003d (confirmed, reverted, error, failed, completed, cancelled, done). Since this migration is no longer in-progress, it won\u0027t be tracked.\n\n[1] https://github.com/openstack/nova/blob/a503e42a17322db8b92033628c3684ca80d4cb45/nova/compute/resource_tracker.py#L890-L892\n[2] https://github.com/openstack/nova/blob/a503e42a17322db8b92033628c3684ca80d4cb45/nova/db/sqlalchemy/api.py#L3151-L3154","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"2f0224bb1ae845b8f178b254ab29dcfd5ebf024f","unresolved":false,"context_lines":[{"line_number":4332,"context_line":"        # instance.migration_context so make sure to not call"},{"line_number":4333,"context_line":"        # instance.drop_migration_context() until after drop_move_claim"},{"line_number":4334,"context_line":"        # is called."},{"line_number":4335,"context_line":"        self.rt.drop_move_claim("},{"line_number":4336,"context_line":"            context, instance, migration.source_node, instance.old_flavor,"},{"line_number":4337,"context_line":"            prefix\u003d\u0027old_\u0027)"},{"line_number":4338,"context_line":"        instance.drop_migration_context()"},{"line_number":4339,"context_line":""},{"line_number":4340,"context_line":"        # NOTE(mriedem): The old_vm_state could be STOPPED but the user"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_b4312b5c","line":4337,"range":{"start_line":4335,"start_character":8,"end_line":4337,"end_character":26},"in_reply_to":"9f560f44_3628b6e0","updated":"2020-08-24 14:58:29.000000000","message":"Okay. It turns out we can and should place the migration update under the semaphore. However, we also need this fix to move unsetting old_flavor and new_flavor to after the \u0027drop_move_claim\u0027 has run. I\u0027ve changed closes-bug to partial-bug and posted a follow-up that tackles the semaphore issue.","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"8d13a5d44a91a622db947ab5451e37421d0aa008","unresolved":false,"context_lines":[{"line_number":4332,"context_line":"        # instance.migration_context so make sure to not call"},{"line_number":4333,"context_line":"        # instance.drop_migration_context() until after drop_move_claim"},{"line_number":4334,"context_line":"        # is called."},{"line_number":4335,"context_line":"        self.rt.drop_move_claim("},{"line_number":4336,"context_line":"            context, instance, migration.source_node, instance.old_flavor,"},{"line_number":4337,"context_line":"            prefix\u003d\u0027old_\u0027)"},{"line_number":4338,"context_line":"        instance.drop_migration_context()"},{"line_number":4339,"context_line":""},{"line_number":4340,"context_line":"        # NOTE(mriedem): The old_vm_state could be STOPPED but the user"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_d084e2d5","line":4337,"range":{"start_line":4335,"start_character":8,"end_line":4337,"end_character":26},"in_reply_to":"9f560f44_4d283d96","updated":"2020-08-20 12:16:38.000000000","message":"I suspect so, yes. So there\u0027s still a race here, it\u0027s just a (much) smaller window. So I wonder should I keep this as-is but harden \u0027drop_move_claim\u0027 to handle this potential race?","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"0743db0e0d1b07389b6e91a83895981d40c43239","unresolved":false,"context_lines":[{"line_number":4332,"context_line":"        # instance.migration_context so make sure to not call"},{"line_number":4333,"context_line":"        # instance.drop_migration_context() until after drop_move_claim"},{"line_number":4334,"context_line":"        # is called."},{"line_number":4335,"context_line":"        self.rt.drop_move_claim("},{"line_number":4336,"context_line":"            context, instance, migration.source_node, instance.old_flavor,"},{"line_number":4337,"context_line":"            prefix\u003d\u0027old_\u0027)"},{"line_number":4338,"context_line":"        instance.drop_migration_context()"},{"line_number":4339,"context_line":""},{"line_number":4340,"context_line":"        # NOTE(mriedem): The old_vm_state could be STOPPED but the user"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_4d283d96","line":4337,"range":{"start_line":4335,"start_character":8,"end_line":4337,"end_character":26},"in_reply_to":"9f560f44_ca0a731e","updated":"2020-08-20 11:23:08.000000000","message":"so in theory, this bug can be triggered if the update_available_resource is running between line 4329 and 4335?","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"9d098d34f8cc43487afa28dc5cf2fee22e6a1088","unresolved":false,"context_lines":[{"line_number":4332,"context_line":"        # instance.migration_context so make sure to not call"},{"line_number":4333,"context_line":"        # instance.drop_migration_context() until after drop_move_claim"},{"line_number":4334,"context_line":"        # is called."},{"line_number":4335,"context_line":"        self.rt.drop_move_claim("},{"line_number":4336,"context_line":"            context, instance, migration.source_node, instance.old_flavor,"},{"line_number":4337,"context_line":"            prefix\u003d\u0027old_\u0027)"},{"line_number":4338,"context_line":"        instance.drop_migration_context()"},{"line_number":4339,"context_line":""},{"line_number":4340,"context_line":"        # NOTE(mriedem): The old_vm_state could be STOPPED but the user"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_dbac03a1","line":4337,"range":{"start_line":4335,"start_character":8,"end_line":4337,"end_character":26},"in_reply_to":"9f560f44_d084e2d5","updated":"2020-08-20 12:57:59.000000000","message":"two options in my mind:\n1. avoid update usage again in drop_move_claim..\n2. put the instance db update into the same lock with drop_move_claim, just like instance claim update the instance.host.","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"5edc1f08fc2169729816f2e3b9ad31f6022427ab","unresolved":false,"context_lines":[{"line_number":4332,"context_line":"        # instance.migration_context so make sure to not call"},{"line_number":4333,"context_line":"        # instance.drop_migration_context() until after drop_move_claim"},{"line_number":4334,"context_line":"        # is called."},{"line_number":4335,"context_line":"        self.rt.drop_move_claim("},{"line_number":4336,"context_line":"            context, instance, migration.source_node, instance.old_flavor,"},{"line_number":4337,"context_line":"            prefix\u003d\u0027old_\u0027)"},{"line_number":4338,"context_line":"        instance.drop_migration_context()"},{"line_number":4339,"context_line":""},{"line_number":4340,"context_line":"        # NOTE(mriedem): The old_vm_state could be STOPPED but the user"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_3628b6e0","line":4337,"range":{"start_line":4335,"start_character":8,"end_line":4337,"end_character":26},"in_reply_to":"9f560f44_dbac03a1","updated":"2020-08-24 10:16:49.000000000","message":"I was able to reproduce this with the functional test so it\u0027s definitely an issue.\n\nWe don\u0027t want to do 1. because the whole idea of doing this update was to avoid hogging resources until the next time the periodic task runs (there\u0027s a commit from mriedem that purposefully added this functionality). By 2. I assume you mean put the *migration* db update into the same lock? That works.","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"4b8e5aa8ad9f37a71188acd3ac83fcedc6b46969","unresolved":false,"context_lines":[{"line_number":4355,"context_line":""},{"line_number":4356,"context_line":"        instance.vm_state \u003d vm_state"},{"line_number":4357,"context_line":"        instance.task_state \u003d None"},{"line_number":4358,"context_line":"        instance.save(expected_task_state\u003d[None, task_states.DELETING,"},{"line_number":4359,"context_line":"                                           task_states.SOFT_DELETING])"},{"line_number":4360,"context_line":""},{"line_number":4361,"context_line":"        self._notify_about_instance_usage("},{"line_number":4362,"context_line":"            context, instance, \"resize.confirm.end\","}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_3b6c2435","line":4359,"range":{"start_line":4358,"start_character":8,"end_line":4359,"end_character":70},"updated":"2020-08-20 08:30:50.000000000","message":"we can remove this save. Then we save one DB call.","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"8677218b11cffcb7403e36965e99d703a2c83e52","unresolved":false,"context_lines":[{"line_number":4355,"context_line":""},{"line_number":4356,"context_line":"        instance.vm_state \u003d vm_state"},{"line_number":4357,"context_line":"        instance.task_state \u003d None"},{"line_number":4358,"context_line":"        instance.save(expected_task_state\u003d[None, task_states.DELETING,"},{"line_number":4359,"context_line":"                                           task_states.SOFT_DELETING])"},{"line_number":4360,"context_line":""},{"line_number":4361,"context_line":"        self._notify_about_instance_usage("},{"line_number":4362,"context_line":"            context, instance, \"resize.confirm.end\","}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_aa28df85","line":4359,"range":{"start_line":4358,"start_character":8,"end_line":4359,"end_character":70},"in_reply_to":"9f560f44_3b6c2435","updated":"2020-08-20 09:32:29.000000000","message":"If I remove this, we won\u0027t be able to validate the expected task state. Is that okay?","commit_id":"29d2b5c9591ccccfcd6f694ad72757ce32e13690"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":4307,"context_line":"            phase\u003dfields.NotificationPhase.START)"},{"line_number":4308,"context_line":""},{"line_number":4309,"context_line":"        # NOTE(danms): delete stashed migration information"},{"line_number":4310,"context_line":"        old_instance_type \u003d instance.old_flavor"},{"line_number":4311,"context_line":"        instance.old_flavor \u003d None"},{"line_number":4312,"context_line":"        instance.new_flavor \u003d None"},{"line_number":4313,"context_line":"        instance.system_metadata.pop(\u0027old_vm_state\u0027, None)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_95cb0170","side":"PARENT","line":4310,"updated":"2020-08-27 18:30:23.000000000","message":"You\u0027re not saving old_instance_type anymore...","commit_id":"e1adbced92453329f7285ec38c1dc7821ebb52c7"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":4306,"context_line":"            self.host, action\u003dfields.NotificationAction.RESIZE_CONFIRM,"},{"line_number":4307,"context_line":"            phase\u003dfields.NotificationPhase.START)"},{"line_number":4308,"context_line":""},{"line_number":4309,"context_line":"        # NOTE(danms): delete stashed migration information"},{"line_number":4310,"context_line":"        old_instance_type \u003d instance.old_flavor"},{"line_number":4311,"context_line":"        instance.old_flavor \u003d None"},{"line_number":4312,"context_line":"        instance.new_flavor \u003d None"},{"line_number":4313,"context_line":"        instance.system_metadata.pop(\u0027old_vm_state\u0027, None)"},{"line_number":4314,"context_line":"        instance.save()"},{"line_number":4315,"context_line":""},{"line_number":4316,"context_line":"        # NOTE(tr3buchet): tear down networks on source host"},{"line_number":4317,"context_line":"        self.network_api.setup_networks_on_host(context, instance,"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_95f4a1d0","side":"PARENT","line":4314,"range":{"start_line":4309,"start_character":0,"end_line":4314,"end_character":23},"updated":"2020-08-27 18:30:23.000000000","message":"So this has moved into the new _delete_stashed_flavor_info() helper...","commit_id":"e1adbced92453329f7285ec38c1dc7821ebb52c7"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":4333,"context_line":"        # instance.drop_migration_context() until after drop_move_claim"},{"line_number":4334,"context_line":"        # is called."},{"line_number":4335,"context_line":"        self.rt.drop_move_claim("},{"line_number":4336,"context_line":"            context, instance, migration.source_node, instance.old_flavor,"},{"line_number":4337,"context_line":"            prefix\u003d\u0027old_\u0027)"},{"line_number":4338,"context_line":"        instance.drop_migration_context()"},{"line_number":4339,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_158ed12c","line":4336,"range":{"start_line":4336,"start_character":54,"end_line":4336,"end_character":73},"updated":"2020-08-27 18:30:23.000000000","message":"So because you\u0027re not saving old_instance_type anymore, you need to change to instance.old_flavor here. This is fine, because the new _delete_stashed_flavor_info() which deletes is only called at the very end of the synchronized do_confirm_resize() which calls the _confirm_resize() method that we\u0027re currently in.","commit_id":"312e47d5df9ac7bb781d71552b5a06a2323146f4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":4493,"context_line":"        self._delete_volume_attachments(ctxt, instance.get_bdms())"},{"line_number":4494,"context_line":""},{"line_number":4495,"context_line":"        # Free up the old_flavor usage from the resource tracker for this host."},{"line_number":4496,"context_line":"        self.rt.drop_move_claim("},{"line_number":4497,"context_line":"            ctxt, instance, migration.source_node, instance.old_flavor,"},{"line_number":4498,"context_line":"            prefix\u003d\u0027old_\u0027)"},{"line_number":4499,"context_line":"        instance.drop_migration_context()"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_3077a3a2","line":4496,"updated":"2020-08-27 18:30:23.000000000","message":"In here we do drop_move_claim() for some reason, and not the \"manual\" instance.old_flavor and instance.new_flavor cleanups...","commit_id":"312e47d5df9ac7bb781d71552b5a06a2323146f4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":4644,"context_line":"        self.rt.drop_move_claim(ctxt, instance, instance.node,"},{"line_number":4645,"context_line":"                                instance_type\u003dinstance.new_flavor)"},{"line_number":4646,"context_line":""},{"line_number":4647,"context_line":"    def _revert_instance_flavor_host_node(self, instance, migration):"},{"line_number":4648,"context_line":"        \"\"\"Revert host, node and flavor fields after a resize-revert.\"\"\""},{"line_number":4649,"context_line":"        self._set_instance_info(instance, instance.old_flavor)"},{"line_number":4650,"context_line":"        instance.host \u003d migration.source_compute"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_95c78145","line":4647,"updated":"2020-08-27 18:30:23.000000000","message":"So this is replacing the old _finish_revert_resize_update_instance_flavor_host_node() helper, but shouldn\u0027t it also be setting instance.old_flavor \u003d instance.new_flavor \u003d None?\n\n\u003clater\u003e Ah, you\u0027re relying on having called _delete_stashed_flavor_inf() first.","commit_id":"312e47d5df9ac7bb781d71552b5a06a2323146f4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":4681,"context_line":"                    ctxt, instance, migration)"},{"line_number":4682,"context_line":""},{"line_number":4683,"context_line":"        try:"},{"line_number":4684,"context_line":"            do_revert()"},{"line_number":4685,"context_line":"        finally:"},{"line_number":4686,"context_line":"            self._delete_stashed_flavor_info(instance)"},{"line_number":4687,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_b5fa8571","line":4684,"updated":"2020-08-27 18:30:23.000000000","message":"So this just calls _finish_revert_snapshot_based_resize_at_source()","commit_id":"312e47d5df9ac7bb781d71552b5a06a2323146f4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":4683,"context_line":"        try:"},{"line_number":4684,"context_line":"            do_revert()"},{"line_number":4685,"context_line":"        finally:"},{"line_number":4686,"context_line":"            self._delete_stashed_flavor_info(instance)"},{"line_number":4687,"context_line":""},{"line_number":4688,"context_line":"        # Broadcast to all schedulers that the instance is on this host."},{"line_number":4689,"context_line":"        # This is best effort so if anything fails just log it."}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_95cca161","line":4686,"updated":"2020-08-27 18:30:23.000000000","message":"So here in all cases we\u0027ll delete instnace.old_flavor and instance.new_flavor...","commit_id":"312e47d5df9ac7bb781d71552b5a06a2323146f4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":4712,"context_line":"            \u0027old_vm_state\u0027, vm_states.ACTIVE)"},{"line_number":4713,"context_line":""},{"line_number":4714,"context_line":"        # Revert the flavor and host/node fields to their previous values"},{"line_number":4715,"context_line":"        self._revert_instance_flavor_host_node(instance, migration)"},{"line_number":4716,"context_line":""},{"line_number":4717,"context_line":"        # Move the allocations against the source compute node resource"},{"line_number":4718,"context_line":"        # provider, held by the migration, to the instance which will drop"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_7532ad5f","line":4715,"updated":"2020-08-27 18:30:23.000000000","message":"And so here you just revert the flavor info, but count on that `finally` on L4686 to remove the stashed old_flavor and new_flavor.","commit_id":"312e47d5df9ac7bb781d71552b5a06a2323146f4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":4919,"context_line":"            self._finish_revert_resize("},{"line_number":4920,"context_line":"                context, instance, migration, request_spec)"},{"line_number":4921,"context_line":"        finally:"},{"line_number":4922,"context_line":"            self._delete_stashed_flavor_info(instance)"},{"line_number":4923,"context_line":""},{"line_number":4924,"context_line":"    def _finish_revert_resize("},{"line_number":4925,"context_line":"        self, context, instance, migration, request_spec\u003dNone,"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_3515d59c","line":4922,"updated":"2020-08-27 18:30:23.000000000","message":"And all this bit is just to ensure we delete instance.old_flavor and instance.new_flavor at the very end of finish_rever_resize()","commit_id":"312e47d5df9ac7bb781d71552b5a06a2323146f4"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"179955113210632c38dff08691489648a8950c40","unresolved":false,"context_lines":[{"line_number":5684,"context_line":"                    self.volume_api.attachment_complete("},{"line_number":5685,"context_line":"                        context, bdm.attachment_id)"},{"line_number":5686,"context_line":""},{"line_number":5687,"context_line":"    def _finish_resize(self, context, instance, migration, disk_info,"},{"line_number":5688,"context_line":"                       image_meta, bdms, request_spec):"},{"line_number":5689,"context_line":"        resize_instance \u003d False  # indicates disks have been resized"},{"line_number":5690,"context_line":"        old_instance_type_id \u003d migration[\u0027old_instance_type_id\u0027]"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_d551f931","line":5687,"updated":"2020-08-27 18:30:23.000000000","message":"There\u0027s a bunch of references to old_flavor here, so I\u0027m afraid of races, but IIUC finish_resize is just to get the instance to the CONFIRM_RESIZE state, *after* which point the user has to call either confirm or revert via the API. Right? So there would be no danger of races here...","commit_id":"312e47d5df9ac7bb781d71552b5a06a2323146f4"}],"nova/compute/resource_tracker.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c23d46bf622b5dd85f2380b7af51f1b5267bd1cc","unresolved":false,"context_lines":[{"line_number":1208,"context_line":""},{"line_number":1209,"context_line":"        # calculate the NUMA usage, assuming the instance is actually using"},{"line_number":1210,"context_line":"        # NUMA, of course"},{"line_number":1211,"context_line":"        if cn.numa_topology and usage.get(\u0027numa_topology\u0027):"},{"line_number":1212,"context_line":"            instance_numa_topology \u003d usage.get(\u0027numa_topology\u0027)"},{"line_number":1213,"context_line":"            # the ComputeNode.numa_topology field is a StringField, so"},{"line_number":1214,"context_line":"            # deserialize"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_e2e3d3a2","line":1211,"updated":"2020-08-06 19:19:12.000000000","message":"Here","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"f13169c732d579c617a60a69a6d5d4437b94cb44","unresolved":false,"context_lines":[{"line_number":1248,"context_line":"        itype \u003d None"},{"line_number":1249,"context_line":"        numa_topology \u003d None"},{"line_number":1250,"context_line":"        sign \u003d 0"},{"line_number":1251,"context_line":"        if same_node:"},{"line_number":1252,"context_line":"            # Same node resize. Record usage for the \u0027new_\u0027 resources.  This"},{"line_number":1253,"context_line":"            # is executed on resize_claim()."},{"line_number":1254,"context_line":"            if (instance[\u0027instance_type_id\u0027] \u003d\u003d"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_e29d9366","line":1251,"updated":"2020-08-06 18:59:46.000000000","message":"So in the case of a different-node resize this whole block is skipped...","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"f13169c732d579c617a60a69a6d5d4437b94cb44","unresolved":false,"context_lines":[{"line_number":1273,"context_line":"                numa_topology \u003d self._get_migration_context_resource("},{"line_number":1274,"context_line":"                    \u0027numa_topology\u0027, instance, prefix\u003d\u0027old_\u0027)"},{"line_number":1275,"context_line":""},{"line_number":1276,"context_line":"        elif incoming and not tracked:"},{"line_number":1277,"context_line":"            # instance has not yet migrated here:"},{"line_number":1278,"context_line":"            itype \u003d self._get_instance_type(instance, \u0027new_\u0027, migration)"},{"line_number":1279,"context_line":"            numa_topology \u003d self._get_migration_context_resource("}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_228dcb31","line":1276,"updated":"2020-08-06 18:59:46.000000000","message":"As is this (on the source)","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"f13169c732d579c617a60a69a6d5d4437b94cb44","unresolved":false,"context_lines":[{"line_number":1283,"context_line":"            LOG.debug(\u0027Starting to track incoming migration %s with flavor %s\u0027,"},{"line_number":1284,"context_line":"                      migration.uuid, itype.flavorid, instance\u003dinstance)"},{"line_number":1285,"context_line":""},{"line_number":1286,"context_line":"        elif outbound and not tracked:"},{"line_number":1287,"context_line":"            # instance migrated, but record usage for a possible revert:"},{"line_number":1288,"context_line":"            itype \u003d self._get_instance_type(instance, \u0027old_\u0027, migration)"},{"line_number":1289,"context_line":"            numa_topology \u003d self._get_migration_context_resource("}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_02908749","line":1286,"updated":"2020-08-06 18:59:46.000000000","message":"But we enter this block (on the source)","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"f13169c732d579c617a60a69a6d5d4437b94cb44","unresolved":false,"context_lines":[{"line_number":1285,"context_line":""},{"line_number":1286,"context_line":"        elif outbound and not tracked:"},{"line_number":1287,"context_line":"            # instance migrated, but record usage for a possible revert:"},{"line_number":1288,"context_line":"            itype \u003d self._get_instance_type(instance, \u0027old_\u0027, migration)"},{"line_number":1289,"context_line":"            numa_topology \u003d self._get_migration_context_resource("},{"line_number":1290,"context_line":"                \u0027numa_topology\u0027, instance, prefix\u003d\u0027old_\u0027)"},{"line_number":1291,"context_line":"            # We could be racing with confirm_resize setting the"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_62834305","line":1288,"updated":"2020-08-06 18:59:46.000000000","message":"But then this gives us None, because by this time, the old flavor has been removed from the instance object.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"f13169c732d579c617a60a69a6d5d4437b94cb44","unresolved":false,"context_lines":[{"line_number":1293,"context_line":"            # is \"confirmed\" so if we did not find the flavor in the outgoing"},{"line_number":1294,"context_line":"            # resized instance we won\u0027t track it."},{"line_number":1295,"context_line":"            if itype:"},{"line_number":1296,"context_line":"                LOG.debug(\u0027Starting to track outgoing migration %s with \u0027"},{"line_number":1297,"context_line":"                          \u0027flavor %s\u0027, migration.uuid, itype.flavorid,"},{"line_number":1298,"context_line":"                          instance\u003dinstance)"},{"line_number":1299,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_4286ff14","line":1296,"updated":"2020-08-06 18:59:46.000000000","message":"So this is not logged...","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"f13169c732d579c617a60a69a6d5d4437b94cb44","unresolved":false,"context_lines":[{"line_number":1297,"context_line":"                          \u0027flavor %s\u0027, migration.uuid, itype.flavorid,"},{"line_number":1298,"context_line":"                          instance\u003dinstance)"},{"line_number":1299,"context_line":""},{"line_number":1300,"context_line":"        if itype:"},{"line_number":1301,"context_line":"            cn \u003d self.compute_nodes[nodename]"},{"line_number":1302,"context_line":"            usage \u003d self._get_usage_dict("},{"line_number":1303,"context_line":"                        itype, instance, numa_topology\u003dnuma_topology)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_a2a55ba4","line":1300,"updated":"2020-08-06 18:59:46.000000000","message":"And this entire blocked never gets executed.\n\nWhat I don\u0027t get is - how does any of this cause the periodic task to free up the instance\u0027s PCPUs? Because that\u0027s what I assume is happening, right? The periodic frees it up, and when we get to the drop_move_claim at the end of the confirm_resize, we attempt to unpin CPUs that are no longer pinned.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c23d46bf622b5dd85f2380b7af51f1b5267bd1cc","unresolved":false,"context_lines":[{"line_number":1304,"context_line":"            if self.pci_tracker and sign:"},{"line_number":1305,"context_line":"                self.pci_tracker.update_pci_for_instance("},{"line_number":1306,"context_line":"                    context, instance, sign\u003dsign)"},{"line_number":1307,"context_line":"            self._update_usage(usage, nodename)"},{"line_number":1308,"context_line":"            if self.pci_tracker:"},{"line_number":1309,"context_line":"                obj \u003d self.pci_tracker.stats.to_device_pools_obj()"},{"line_number":1310,"context_line":"                cn.pci_device_pools \u003d obj"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_c2496fb7","line":1307,"updated":"2020-08-06 19:19:12.000000000","message":"In reply to the comment on line 1300, this is the issue. Remember, when we say \"unpin\" in this context, we\u0027re not actually do anything with libvirt - it\u0027s purely resource tracking. Because we don\u0027t enter this \u0027if itype:\u0027 block, we never call \u0027_update_usage\u0027. As I highlighted above, that\u0027s responsible for increasing the usage of the \u0027ComputeNode.numa_topology\u0027 for the instance via a call to \u0027hardware.numa_usage_from_instance_numa\u0027. If that happens, the usage gets out of whack","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"ce5f9d5bd0029ab08afa996745fd504e2c2577db","unresolved":false,"context_lines":[{"line_number":1304,"context_line":"            if self.pci_tracker and sign:"},{"line_number":1305,"context_line":"                self.pci_tracker.update_pci_for_instance("},{"line_number":1306,"context_line":"                    context, instance, sign\u003dsign)"},{"line_number":1307,"context_line":"            self._update_usage(usage, nodename)"},{"line_number":1308,"context_line":"            if self.pci_tracker:"},{"line_number":1309,"context_line":"                obj \u003d self.pci_tracker.stats.to_device_pools_obj()"},{"line_number":1310,"context_line":"                cn.pci_device_pools \u003d obj"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_62450370","line":1307,"in_reply_to":"9f560f44_c2496fb7","updated":"2020-08-06 19:22:41.000000000","message":"Ah, we don\u0027t \"delta\" the usage, we rebuild it from scratch every time. So if a certain migration/instance doesn\u0027t get counted as \"tracked\", it\u0027ll effectively unpin its PCPUs.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"}],"nova/tests/unit/compute/test_compute_mgr.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dbce7d11ab42987d8892591b0d76b7c071c77606","unresolved":false,"context_lines":[{"line_number":8789,"context_line":"                                        self.migration)"},{"line_number":8790,"context_line":"            mock_delete.assert_called_once_with(self.context, self.instance,"},{"line_number":8791,"context_line":"                                                self.migration)"},{"line_number":8792,"context_line":"            mock_save.assert_called()"},{"line_number":8793,"context_line":"            mock_delete_scheduler_info.assert_called_once_with("},{"line_number":8794,"context_line":"                self.context, self.instance.uuid)"},{"line_number":8795,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_24c95658","line":8792,"updated":"2020-08-06 22:03:41.000000000","message":"Curious why the expected_task_state removed here?\n\nHere\u0027s the corresponding call:\n\nhttps://review.opendev.org/#/c/744958/2/nova/compute/manager.py@4360","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"9ab871a3540ea67dde0c6ab4435dad1b672b31eb","unresolved":false,"context_lines":[{"line_number":8789,"context_line":"                                        self.migration)"},{"line_number":8790,"context_line":"            mock_delete.assert_called_once_with(self.context, self.instance,"},{"line_number":8791,"context_line":"                                                self.migration)"},{"line_number":8792,"context_line":"            mock_save.assert_called()"},{"line_number":8793,"context_line":"            mock_delete_scheduler_info.assert_called_once_with("},{"line_number":8794,"context_line":"                self.context, self.instance.uuid)"},{"line_number":8795,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_0906c220","line":8792,"in_reply_to":"9f560f44_1ea45207","updated":"2020-08-10 19:15:04.000000000","message":"Ack thanks.","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"a6b48cd868e12da88e02f5e5908c56cc20af859d","unresolved":false,"context_lines":[{"line_number":8789,"context_line":"                                        self.migration)"},{"line_number":8790,"context_line":"            mock_delete.assert_called_once_with(self.context, self.instance,"},{"line_number":8791,"context_line":"                                                self.migration)"},{"line_number":8792,"context_line":"            mock_save.assert_called()"},{"line_number":8793,"context_line":"            mock_delete_scheduler_info.assert_called_once_with("},{"line_number":8794,"context_line":"                self.context, self.instance.uuid)"},{"line_number":8795,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_1ea45207","line":8792,"in_reply_to":"9f560f44_24c95658","updated":"2020-08-07 13:38:08.000000000","message":"\u0027assert_called_with\u0027 checks the last call. The call you linked to is no longer the last once, since we call \u0027save\u0027 again after we return from the function (via the \u0027finally\u0027 block). Turns out those are the only two callers though (I thought there\u0027d be more) so I can explicitly check both.\n\nDone","commit_id":"1c55102ea0408143106254eba6fd12063f19e205"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"cf695b55ed23e4c084a2ea28233c179df917882e","unresolved":false,"context_lines":[{"line_number":8795,"context_line":"                        None, task_states.DELETING, task_states.SOFT_DELETING,"},{"line_number":8796,"context_line":"                    ],"},{"line_number":8797,"context_line":"                ),"},{"line_number":8798,"context_line":"                mock.call(),"},{"line_number":8799,"context_line":"            ])"},{"line_number":8800,"context_line":"            mock_delete_scheduler_info.assert_called_once_with("},{"line_number":8801,"context_line":"                self.context, self.instance.uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_a694258c","line":8798,"updated":"2020-08-10 17:06:39.000000000","message":"It\u0027s unclear since the original was not \"once_with\", but it does seem like you\u0027re adding an extra save() in the compute manager code, although it\u0027s a little hard to follow.. is that right?","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ee297850bb1b0f57f570608b1a0b7f7b33e4e4c4","unresolved":false,"context_lines":[{"line_number":8795,"context_line":"                        None, task_states.DELETING, task_states.SOFT_DELETING,"},{"line_number":8796,"context_line":"                    ],"},{"line_number":8797,"context_line":"                ),"},{"line_number":8798,"context_line":"                mock.call(),"},{"line_number":8799,"context_line":"            ])"},{"line_number":8800,"context_line":"            mock_delete_scheduler_info.assert_called_once_with("},{"line_number":8801,"context_line":"                self.context, self.instance.uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_da4f6132","line":8798,"in_reply_to":"9f560f44_a694258c","updated":"2020-08-11 10:34:21.000000000","message":"See [1]. tl;dr: the order of save() calls has changed and we now need to check all of the save() calls rather than just the last one\n\n[1] https://review.opendev.org/#/c/744958/2/nova/tests/unit/compute/test_compute_mgr.py@8792","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"7bee80e86d81a33f632be9c1936a2e197e7269f3","unresolved":false,"context_lines":[{"line_number":8795,"context_line":"                        None, task_states.DELETING, task_states.SOFT_DELETING,"},{"line_number":8796,"context_line":"                    ],"},{"line_number":8797,"context_line":"                ),"},{"line_number":8798,"context_line":"                mock.call(),"},{"line_number":8799,"context_line":"            ])"},{"line_number":8800,"context_line":"            mock_delete_scheduler_info.assert_called_once_with("},{"line_number":8801,"context_line":"                self.context, self.instance.uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f560f44_9c6d739a","line":8798,"in_reply_to":"9f560f44_da4f6132","updated":"2020-08-19 17:58:26.000000000","message":"Ack, yeah, thanks.","commit_id":"ba4bab2e1eff1efb4cd30eea0260e132b70d8c51"}]}