)]}'
{"nova/compute/manager.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"379070c974aa6ea49f09fabe1abd5b4de0873d95","unresolved":false,"context_lines":[{"line_number":4578,"context_line":"            # ones which will be used later if the resize is reverted."},{"line_number":4579,"context_line":"            LOG.debug(\u0027Deleting volume attachments for the source host.\u0027,"},{"line_number":4580,"context_line":"                      instance\u003dinstance)"},{"line_number":4581,"context_line":"            self._terminate_volume_connections(ctxt, instance, bdms)"},{"line_number":4582,"context_line":""},{"line_number":4583,"context_line":"            # At this point the VIFs are unplugged from this source host."},{"line_number":4584,"context_line":"            # However, we do not call self.network_api.migrate_instance_start"}],"source_content_type":"text/x-python","patch_set":10,"id":"9fdfeff1_551fa1b6","line":4581,"range":{"start_line":4581,"start_character":17,"end_line":4581,"end_character":46},"updated":"2019-02-20 22:53:03.000000000","message":"This is likely where our attachment is leaking.","commit_id":"ace8f6940087dda4e9e20d4cd87bc781057ab15b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a4255ecb572b29a69024d7fa121e85b481203cd2","unresolved":false,"context_lines":[{"line_number":4578,"context_line":"            # ones which will be used later if the resize is reverted."},{"line_number":4579,"context_line":"            LOG.debug(\u0027Deleting volume attachments for the source host.\u0027,"},{"line_number":4580,"context_line":"                      instance\u003dinstance)"},{"line_number":4581,"context_line":"            self._terminate_volume_connections(ctxt, instance, bdms)"},{"line_number":4582,"context_line":""},{"line_number":4583,"context_line":"            # At this point the VIFs are unplugged from this source host."},{"line_number":4584,"context_line":"            # However, we do not call self.network_api.migrate_instance_start"}],"source_content_type":"text/x-python","patch_set":10,"id":"9fdfeff1_41a6a2c2","line":4581,"range":{"start_line":4581,"start_character":17,"end_line":4581,"end_character":46},"in_reply_to":"9fdfeff1_551fa1b6","updated":"2019-02-26 15:02:09.000000000","message":"This will delete any original bdm attachments on the source instance before the resize was initiated and create empty attachments in the source cell db in case of a possible revert.","commit_id":"ace8f6940087dda4e9e20d4cd87bc781057ab15b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"31e034f55cdb65c92b9e1446a2cb28020d94a704","unresolved":false,"context_lines":[{"line_number":4545,"context_line":"                self._revert_allocation(ctxt, instance, migration)"},{"line_number":4546,"context_line":""},{"line_number":4547,"context_line":"    @delete_image_on_error"},{"line_number":4548,"context_line":"    def _snapshot_for_resize(self, ctxt, image_id, instance):"},{"line_number":4549,"context_line":"        \"\"\"Uploads snapshot for the instance during a snapshot-based resize"},{"line_number":4550,"context_line":""},{"line_number":4551,"context_line":"        If the snapshot operation fails the image will be deleted."}],"source_content_type":"text/x-python","patch_set":29,"id":"bfb3d3c7_9f4d2774","line":4548,"updated":"2019-05-21 16:28:32.000000000","message":"Could be interesting to use the timeutils.StopWatch or timeutils.time_it decorator in this method to time how long the overall snapshot takes.","commit_id":"0775237daa99b4938d52075d7465ee0d5e72d65e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8fdec522fca8be278c265854358adc5dd694a40b","unresolved":false,"context_lines":[{"line_number":4545,"context_line":"                self._revert_allocation(ctxt, instance, migration)"},{"line_number":4546,"context_line":""},{"line_number":4547,"context_line":"    @delete_image_on_error"},{"line_number":4548,"context_line":"    def _snapshot_for_resize(self, ctxt, image_id, instance):"},{"line_number":4549,"context_line":"        \"\"\"Uploads snapshot for the instance during a snapshot-based resize"},{"line_number":4550,"context_line":""},{"line_number":4551,"context_line":"        If the snapshot operation fails the image will be deleted."}],"source_content_type":"text/x-python","patch_set":29,"id":"bfb3d3c7_2d41723a","line":4548,"in_reply_to":"bfb3d3c7_9f4d2774","updated":"2019-05-21 20:50:27.000000000","message":"Done","commit_id":"0775237daa99b4938d52075d7465ee0d5e72d65e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b18ae0de839fe4d613587444477dcf149155c62c","unresolved":false,"context_lines":[{"line_number":4622,"context_line":""},{"line_number":4623,"context_line":"            # At this point the VIFs are unplugged from this source host."},{"line_number":4624,"context_line":"            # Activate the dest host port bindings created by conductor."},{"line_number":4625,"context_line":"            # TODO(mriedem): If https://review.opendev.org/#/c/653506/ merges"},{"line_number":4626,"context_line":"            # we could remove this and let migrate_instance_finish activate the"},{"line_number":4627,"context_line":"            # dest host port binding in finish_snapshot_based_resize_at_dest."},{"line_number":4628,"context_line":"            self.network_api.migrate_instance_start(ctxt, instance, migration)"},{"line_number":4629,"context_line":""},{"line_number":4630,"context_line":"            # Update the migration status from \"migrating\" to \"post-migrating\"."}],"source_content_type":"text/x-python","patch_set":29,"id":"bfb3d3c7_5f7ecf6b","line":4627,"range":{"start_line":4625,"start_character":12,"end_line":4627,"end_character":77},"updated":"2019-05-21 16:34:08.000000000","message":"Should probably remove this because I think we want to activate the dest host port bindings early if for no other reason to make sure we can do it and it doesn\u0027t fail, because if we\u0027re going to fail we want to do it early while we\u0027re on the source host.","commit_id":"0775237daa99b4938d52075d7465ee0d5e72d65e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8fdec522fca8be278c265854358adc5dd694a40b","unresolved":false,"context_lines":[{"line_number":4622,"context_line":""},{"line_number":4623,"context_line":"            # At this point the VIFs are unplugged from this source host."},{"line_number":4624,"context_line":"            # Activate the dest host port bindings created by conductor."},{"line_number":4625,"context_line":"            # TODO(mriedem): If https://review.opendev.org/#/c/653506/ merges"},{"line_number":4626,"context_line":"            # we could remove this and let migrate_instance_finish activate the"},{"line_number":4627,"context_line":"            # dest host port binding in finish_snapshot_based_resize_at_dest."},{"line_number":4628,"context_line":"            self.network_api.migrate_instance_start(ctxt, instance, migration)"},{"line_number":4629,"context_line":""},{"line_number":4630,"context_line":"            # Update the migration status from \"migrating\" to \"post-migrating\"."}],"source_content_type":"text/x-python","patch_set":29,"id":"bfb3d3c7_3ab20696","line":4627,"range":{"start_line":4625,"start_character":12,"end_line":4627,"end_character":77},"in_reply_to":"bfb3d3c7_5f7ecf6b","updated":"2019-05-21 20:50:27.000000000","message":"Done","commit_id":"0775237daa99b4938d52075d7465ee0d5e72d65e"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"bf3f1f231fc452497524ea7621c089ac14a08236","unresolved":false,"context_lines":[{"line_number":4596,"context_line":"                # on the source provider tree back to the instance consumer"},{"line_number":4597,"context_line":"                # and drop the allocations held by the instance on the"},{"line_number":4598,"context_line":"                # destination provider tree."},{"line_number":4599,"context_line":"                self._revert_allocation(ctxt, instance, migration)"},{"line_number":4600,"context_line":""},{"line_number":4601,"context_line":"    @delete_image_on_error"},{"line_number":4602,"context_line":"    def _snapshot_for_resize(self, ctxt, image_id, instance):"}],"source_content_type":"text/x-python","patch_set":36,"id":"9fb8cfa7_02de443d","line":4599,"updated":"2019-06-27 15:34:33.000000000","message":"This feels weird here as the _prep_snapshot_based_resize_at_source() did not create the the dest allocation in the first place. \n\nThe rest of the revert code seems to be in the new conductor tasks so it might be better to move this there as well.","commit_id":"953101e2b07c95c81d52cd47487489a2a349149a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"eeb82dec782b07d09cbbc611bffa889c8ca0ae92","unresolved":false,"context_lines":[{"line_number":4596,"context_line":"                # on the source provider tree back to the instance consumer"},{"line_number":4597,"context_line":"                # and drop the allocations held by the instance on the"},{"line_number":4598,"context_line":"                # destination provider tree."},{"line_number":4599,"context_line":"                self._revert_allocation(ctxt, instance, migration)"},{"line_number":4600,"context_line":""},{"line_number":4601,"context_line":"    @delete_image_on_error"},{"line_number":4602,"context_line":"    def _snapshot_for_resize(self, ctxt, image_id, instance):"}],"source_content_type":"text/x-python","patch_set":36,"id":"9fb8cfa7_50e4d4df","line":4599,"in_reply_to":"9fb8cfa7_02de443d","updated":"2019-07-01 17:52:53.000000000","message":"This is the same thing that resize_instance does on the source host during a traditional resize and prep_snapshot_based_resize_at_source is similar in flow to resize_instance, so that\u0027s why I have this here.\n\n\u003e The rest of the revert code seems to be in the new conductor tasks so it might be better to move this there as well.\n\nDo you mean the rollback stuff per task? You do have a point there. I believe the reason resize_instance for same-cell resize reverts the allocations on failure is because for same-cell resize as soon as conductor has cast to prep_resize on the dest compute, and prep_resize casts to resize_instance on the source compute, we\u0027re far from the revert code in the cold migration task in conductor:\n\nhttps://github.com/openstack/nova/blob/a8d2b24f1f9f801638b04c3140ccadfcd418fd55/nova/conductor/tasks/migrate.py#L321\n\nHowever, for cross-cell resize we\u0027re in a synchronous RPC call from conductor so technically we could drop this code and eventually we should hit the revert in conductor (linked above). One issue would be if we lost the RPC connection so we can\u0027t get back to conductor, but that could cause lots of other issues as well, e.g. the decorators on this method might not work anyway if compute can\u0027t reach conductor to make changes to the DB.\n\nBefore removing this I\u0027d want to make sure we have functional test coverage such that if something fails in this flow we\u0027ll properly revert the allocations.\n\nI have a test like that for finish_snapshot_based_resize_at_dest failing:\n\nhttps://review.opendev.org/#/c/643451/\n\nBut not for prep_snapshot_based_resize_at_source failing. I\u0027ll add a patch for testing this in the functional test class and if it\u0027s cool I\u0027ll remove this.","commit_id":"953101e2b07c95c81d52cd47487489a2a349149a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"38085b49d291b4c6a7c8f44faa4362bb26d308f5","unresolved":false,"context_lines":[{"line_number":4596,"context_line":"                # on the source provider tree back to the instance consumer"},{"line_number":4597,"context_line":"                # and drop the allocations held by the instance on the"},{"line_number":4598,"context_line":"                # destination provider tree."},{"line_number":4599,"context_line":"                self._revert_allocation(ctxt, instance, migration)"},{"line_number":4600,"context_line":""},{"line_number":4601,"context_line":"    @delete_image_on_error"},{"line_number":4602,"context_line":"    def _snapshot_for_resize(self, ctxt, image_id, instance):"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_7be1a3d0","line":4599,"in_reply_to":"9fb8cfa7_50e4d4df","updated":"2019-07-04 01:15:42.000000000","message":"Done - I\u0027ve removed this and added a functional test for it here:\n\nhttps://review.opendev.org/#/c/669013/","commit_id":"953101e2b07c95c81d52cd47487489a2a349149a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"eeb82dec782b07d09cbbc611bffa889c8ca0ae92","unresolved":false,"context_lines":[{"line_number":4713,"context_line":"            self._resize_instance(context, instance, image, migration,"},{"line_number":4714,"context_line":"                                  instance_type, clean_shutdown)"},{"line_number":4715,"context_line":"        except Exception:"},{"line_number":4716,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":4717,"context_line":"                self._revert_allocation(context, instance, migration)"},{"line_number":4718,"context_line":""},{"line_number":4719,"context_line":"    def _resize_instance(self, context, instance, image,"},{"line_number":4720,"context_line":"                         migration, instance_type, clean_shutdown):"}],"source_content_type":"text/x-python","patch_set":36,"id":"9fb8cfa7_7099d86a","line":4717,"range":{"start_line":4716,"start_character":12,"end_line":4717,"end_character":69},"updated":"2019-07-01 17:52:53.000000000","message":"@gibi: this is what I\u0027m following.","commit_id":"953101e2b07c95c81d52cd47487489a2a349149a"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"186c72ed5739262b400b549558c79c23d0466871","unresolved":false,"context_lines":[{"line_number":4665,"context_line":"            \"post-migrating\"."},{"line_number":4666,"context_line":"        :param snapshot_id: ID of the image snapshot to upload if not a"},{"line_number":4667,"context_line":"            volume-backed instance"},{"line_number":4668,"context_line":"        :raises: nova.exception.InstancePowerOffFailure if stopping the"},{"line_number":4669,"context_line":"            instance fails"},{"line_number":4670,"context_line":"        \"\"\""},{"line_number":4671,"context_line":"        # Note that if anything fails here, the migration-based allocations"}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_3ace7e83","line":4668,"updated":"2019-08-12 13:23:53.000000000","message":"It seems there could be a list of other exceptions coming out of _prep_snapshot_based_resize_at_source():\n\n* self.driver.snapshot() could raise driver related exceptions (e.g. libvirt.libvirtError) and InstanceNotFound. Even InstanceNotRunning whose name is incorrect but means the guest is not defined in libvirt. \n* The image upload part of snapshot can raise ImageUnacceptable, Forbidden \n* the migrate_instance_start() can raise PortBindingActivationFailed\n\nand I guess I also missed some of the possible exceptions. \n\nI don\u0027t really know what to do. Listing all of the exceptions is clearly not possible and most probably such list would be outdated soon. So I guess I\u0027m OK with documenting what we clearly raise from this set of codes.\n\nBut. Do you have a good reason listing InstancePowerOffFailure as the only @messaging.expected_exceptions above? Is it somehow a special exception?\n\nDo we need a catch-all except block here to convert all the possible exceptions to something generic, like MigrationError? Or we have such a catch all on the client side already?","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d48cd2163e01494ca6d4090277439c6c1c5f1282","unresolved":false,"context_lines":[{"line_number":4665,"context_line":"            \"post-migrating\"."},{"line_number":4666,"context_line":"        :param snapshot_id: ID of the image snapshot to upload if not a"},{"line_number":4667,"context_line":"            volume-backed instance"},{"line_number":4668,"context_line":"        :raises: nova.exception.InstancePowerOffFailure if stopping the"},{"line_number":4669,"context_line":"            instance fails"},{"line_number":4670,"context_line":"        \"\"\""},{"line_number":4671,"context_line":"        # Note that if anything fails here, the migration-based allocations"}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_508be4be","line":4668,"in_reply_to":"7faddb67_3ace7e83","updated":"2019-08-14 21:32:04.000000000","message":"\u003e I don\u0027t really know what to do. Listing all of the exceptions is\n \u003e clearly not possible and most probably such list would be outdated\n \u003e soon. So I guess I\u0027m OK with documenting what we clearly raise from\n \u003e this set of codes.\n\nYeah I was just documenting what this flow explicitly raises, but for sure there could be lower-level things that bubble up, which is true of most things in the compute manager which don\u0027t even attempt to document themselves (our method documentation is historically pretty terrible).\n\n \u003e But. Do you have a good reason listing InstancePowerOffFailure as\n \u003e the only @messaging.expected_exceptions above? Is it somehow a\n \u003e special exception?\n\nexpected_exceptions is for a list of, well, things you know can happen and shouldn\u0027t be traced by oslo.messaging. Anything not in the list would be considered \"unexpected\" and would get traced by oslo.messaging. If we have other things we hit coming from this frequently that are noise we can list those as well IMO. Having said all that, I seem to remember (it\u0027s been many months now) that at some point I had conductor explicitly handling InstancePowerOffFailure to check the power state and maybe try to recover the power state on the instance, but dropped that for whatever reason - probably because if you hit a failure at that point it\u0027s probably best to just reboot the instance.\n\n \u003e \n \u003e Do we need a catch-all except block here to convert all the\n \u003e possible exceptions to something generic, like MigrationError? Or\n \u003e we have such a catch all on the client side already?\n\nI\u0027d rather not swallow and convert everything unexpected in case the caller wants/needs to do something different with the error handling at some point, but yes there is generic fault handling in the conductor code that will trigger a rollback of the task:\n\nhttps://github.com/openstack/nova/blob/79d1c884b25fccb485bdc31b7dc945c365da8cb0/nova/conductor/tasks/base.py#L38\n\nhttps://github.com/openstack/nova/blob/79d1c884b25fccb485bdc31b7dc945c365da8cb0/nova/conductor/tasks/base.py#L27","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"df69e63767b2d0f78c1e43e6002064b900ab1988","unresolved":false,"context_lines":[{"line_number":4665,"context_line":"            \"post-migrating\"."},{"line_number":4666,"context_line":"        :param snapshot_id: ID of the image snapshot to upload if not a"},{"line_number":4667,"context_line":"            volume-backed instance"},{"line_number":4668,"context_line":"        :raises: nova.exception.InstancePowerOffFailure if stopping the"},{"line_number":4669,"context_line":"            instance fails"},{"line_number":4670,"context_line":"        \"\"\""},{"line_number":4671,"context_line":"        # Note that if anything fails here, the migration-based allocations"}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_839498e9","line":4668,"in_reply_to":"7faddb67_508be4be","updated":"2019-08-16 11:43:53.000000000","message":"Thanks for the explanation. Having a catch all + rollback in the task makes me not too worried about the endless list of exceptions.","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"186c72ed5739262b400b549558c79c23d0466871","unresolved":false,"context_lines":[{"line_number":4733,"context_line":"        # If something fails at this point the instance must go to ERROR"},{"line_number":4734,"context_line":"        # status for operator intervention or to reboot/rebuild the instance."},{"line_number":4735,"context_line":"        with self._error_out_instance_on_exception("},{"line_number":4736,"context_line":"                ctxt, instance, instance_state\u003dvm_states.STOPPED):"},{"line_number":4737,"context_line":""},{"line_number":4738,"context_line":"            # Destroy the guest on the source host which will disconnect"},{"line_number":4739,"context_line":"            # volumes and unplug VIFs. Note that we DO NOT destroy disks since"}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_9a0a121e","line":4736,"range":{"start_line":4736,"start_character":47,"end_line":4736,"end_character":64},"updated":"2019-08-12 13:23:53.000000000","message":"So the instance will be set to STOPPED if NotImplementedError or InstanceFaultRollback raised below, and will be set to ERROR for any other exception. I think the below code does not raise InstanceFaultRollback as it is only raised from _prep_resize() suspend() and driver.migrate_disk_and_power_off() but we don\u0027t call these below. However it is hard to tell if we get NotImplementedError from below. \n\nYou mention just above this context manager that you want to get the instance to ERROR state if something fails. Would it be clearer then to call _error_out_instance_on_exception()  with instance)State\u003dvm_states.ERROR ?","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"df69e63767b2d0f78c1e43e6002064b900ab1988","unresolved":false,"context_lines":[{"line_number":4733,"context_line":"        # If something fails at this point the instance must go to ERROR"},{"line_number":4734,"context_line":"        # status for operator intervention or to reboot/rebuild the instance."},{"line_number":4735,"context_line":"        with self._error_out_instance_on_exception("},{"line_number":4736,"context_line":"                ctxt, instance, instance_state\u003dvm_states.STOPPED):"},{"line_number":4737,"context_line":""},{"line_number":4738,"context_line":"            # Destroy the guest on the source host which will disconnect"},{"line_number":4739,"context_line":"            # volumes and unplug VIFs. Note that we DO NOT destroy disks since"}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_0e552f50","line":4736,"range":{"start_line":4736,"start_character":47,"end_line":4736,"end_character":64},"in_reply_to":"7faddb67_50a4444b","updated":"2019-08-16 11:43:53.000000000","message":"Thanks for the explanation. I agree putting the instance to ACTIVE would be worse. I think adding an explicit ERROR is a bit cleaner but I\u0027m totally OK make that change separately.","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d48cd2163e01494ca6d4090277439c6c1c5f1282","unresolved":false,"context_lines":[{"line_number":4733,"context_line":"        # If something fails at this point the instance must go to ERROR"},{"line_number":4734,"context_line":"        # status for operator intervention or to reboot/rebuild the instance."},{"line_number":4735,"context_line":"        with self._error_out_instance_on_exception("},{"line_number":4736,"context_line":"                ctxt, instance, instance_state\u003dvm_states.STOPPED):"},{"line_number":4737,"context_line":""},{"line_number":4738,"context_line":"            # Destroy the guest on the source host which will disconnect"},{"line_number":4739,"context_line":"            # volumes and unplug VIFs. Note that we DO NOT destroy disks since"}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_50a4444b","line":4736,"range":{"start_line":4736,"start_character":47,"end_line":4736,"end_character":64},"in_reply_to":"7faddb67_9a0a121e","updated":"2019-08-14 21:32:04.000000000","message":"\u003e So the instance will be set to STOPPED if NotImplementedError or\n \u003e InstanceFaultRollback raised below, and will be set to ERROR for\n \u003e any other exception. I think the below code does not raise\n \u003e InstanceFaultRollback as it is only raised from _prep_resize()\n \u003e suspend() and driver.migrate_disk_and_power_off() but we don\u0027t call\n \u003e these below. However it is hard to tell if we get NotImplementedError\n \u003e from below.\n\nI don\u0027t think we\u0027ll get NotImplementedError from anything below - every driver must implement destroy. :)\n\n \u003e \n \u003e You mention just above this context manager that you want to get\n \u003e the instance to ERROR state if something fails. Would it be clearer\n \u003e then to call _error_out_instance_on_exception()  with\n \u003e instance)State\u003dvm_states.ERROR ?\n\nAs you\u0027re aware since you diligently read up on _error_out_instance_on_exception (which I documented at some point because its usage is confusing), anything besides those exception types you pointed out will set the vm_state to error. I\u0027m just passing STOPPED here because we know it\u0027s stopped and _error_out_instance_on_exception defaults to ACTIVE, so if that code eventually changes we don\u0027t have some weird bug where we fail and the instance power state is stopped but the vm_state goes back to ACTIVE (which is probably why I\u0027m being explicit, because I worked on Ie4f9177f4d54cbc7dbcf58bd107fd5f24c60d8bb).\n\nI guess you\u0027re just saying, the comment doesn\u0027t match the behavior so if we fail make sure the vm_status is ERROR regardless of the failure (NotImplementedError, InstanceFaultRollback), then yeah I suppose it should - I can see why the comment and the kwarg are confusing.","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"186c72ed5739262b400b549558c79c23d0466871","unresolved":false,"context_lines":[{"line_number":4742,"context_line":"            # resize is reverted."},{"line_number":4743,"context_line":"            LOG.debug(\u0027Destroying guest on source host but retaining disks.\u0027,"},{"line_number":4744,"context_line":"                      instance\u003dinstance)"},{"line_number":4745,"context_line":"            self.driver.destroy("},{"line_number":4746,"context_line":"                ctxt, instance, network_info,"},{"line_number":4747,"context_line":"                block_device_info\u003dblock_device_info, destroy_disks\u003dFalse)"},{"line_number":4748,"context_line":""}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_ba5eeee6","line":4745,"updated":"2019-08-12 13:23:53.000000000","message":"In the libvirt driver there is an overlapping code path between power_off() (triggered at L4719) and destroy() (called here). Both functions call nova.virt.libvirt.driver.LibvirtDriver._destroy(). I guess you still need both calls as power_off() gives you graceful shutdown of the instance and destroy() gives the possibility to unplug VIFs via cleanup(). \n\nI\u0027m wondering if power_off() and driver.cleanup() would be enough. What do you think?","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"05ca3578d1e4f6791e6bb6c5fa52378b9c399cf9","unresolved":false,"context_lines":[{"line_number":4742,"context_line":"            # resize is reverted."},{"line_number":4743,"context_line":"            LOG.debug(\u0027Destroying guest on source host but retaining disks.\u0027,"},{"line_number":4744,"context_line":"                      instance\u003dinstance)"},{"line_number":4745,"context_line":"            self.driver.destroy("},{"line_number":4746,"context_line":"                ctxt, instance, network_info,"},{"line_number":4747,"context_line":"                block_device_info\u003dblock_device_info, destroy_disks\u003dFalse)"},{"line_number":4748,"context_line":""}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_eb2781b5","line":4745,"in_reply_to":"7faddb67_b036d801","updated":"2019-08-14 21:35:20.000000000","message":"Oh also note that in same-cell resize, resize_instance (on the source compute) will call the driver\u0027s migrate_disk_and_power_off method which under the covers is very similar to what I\u0027m doing here, which is power off the guest and then destroy the domain but retain the disks (in the case of reboot or revert for cross-cell resize). The difference from migrate_disk_and_power_off with cross-cell resize is we aren\u0027t transferring the disks to the dest host directly (via shared storage or ssh/rsync), we\u0027re leveraging the snapshot like shelve/unshelve.","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d48cd2163e01494ca6d4090277439c6c1c5f1282","unresolved":false,"context_lines":[{"line_number":4742,"context_line":"            # resize is reverted."},{"line_number":4743,"context_line":"            LOG.debug(\u0027Destroying guest on source host but retaining disks.\u0027,"},{"line_number":4744,"context_line":"                      instance\u003dinstance)"},{"line_number":4745,"context_line":"            self.driver.destroy("},{"line_number":4746,"context_line":"                ctxt, instance, network_info,"},{"line_number":4747,"context_line":"                block_device_info\u003dblock_device_info, destroy_disks\u003dFalse)"},{"line_number":4748,"context_line":""}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_b036d801","line":4745,"in_reply_to":"7faddb67_ba5eeee6","updated":"2019-08-14 21:32:04.000000000","message":"I\u0027m doing the power off and destroy separately because I want to stop the guest before doing a snapshot, which is similar to how shelve offload works - as noted in the commit message. Plus just because libvirt does different things with destroy doesn\u0027t mean other drivers are the same. Unless I\u0027m misunderstanding what you\u0027re saying I should do.","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"df69e63767b2d0f78c1e43e6002064b900ab1988","unresolved":false,"context_lines":[{"line_number":4742,"context_line":"            # resize is reverted."},{"line_number":4743,"context_line":"            LOG.debug(\u0027Destroying guest on source host but retaining disks.\u0027,"},{"line_number":4744,"context_line":"                      instance\u003dinstance)"},{"line_number":4745,"context_line":"            self.driver.destroy("},{"line_number":4746,"context_line":"                ctxt, instance, network_info,"},{"line_number":4747,"context_line":"                block_device_info\u003dblock_device_info, destroy_disks\u003dFalse)"},{"line_number":4748,"context_line":""}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_aecbbb1b","line":4745,"in_reply_to":"7faddb67_eb2781b5","updated":"2019-08-16 11:43:53.000000000","message":"Thanks. Make sense.","commit_id":"a3d839c05243aec90b49ea45276392e6769a92ef"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"3825a152f4a92f6a2436f8c9872a34f12bb87e96","unresolved":false,"context_lines":[{"line_number":4653,"context_line":"        Performs actions like powering off the guest, upload snapshot data if"},{"line_number":4654,"context_line":"        the instance is not volume-backed, disconnecting volumes, unplugging"},{"line_number":4655,"context_line":"        VIFs and activating the destination host port bindings."},{"line_number":4656,"context_line":""},{"line_number":4657,"context_line":"        :param ctxt: user auth request context targeted at source cell"},{"line_number":4658,"context_line":"        :param instance: nova.objects.Instance; the instance being resized."},{"line_number":4659,"context_line":"            The expected instance.task_state is \"resize_migrating\" when calling"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_861f06b6","line":4656,"updated":"2019-08-28 14:16:55.000000000","message":"Worth noting that we don\u0027t free any resources for the reasons from the commit message?","commit_id":"612063d1f1e65add34aa90f05dbc043ec797a97b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3900bc3c838b46a7f7f3da69af4a2ab76c027d91","unresolved":false,"context_lines":[{"line_number":4653,"context_line":"        Performs actions like powering off the guest, upload snapshot data if"},{"line_number":4654,"context_line":"        the instance is not volume-backed, disconnecting volumes, unplugging"},{"line_number":4655,"context_line":"        VIFs and activating the destination host port bindings."},{"line_number":4656,"context_line":""},{"line_number":4657,"context_line":"        :param ctxt: user auth request context targeted at source cell"},{"line_number":4658,"context_line":"        :param instance: nova.objects.Instance; the instance being resized."},{"line_number":4659,"context_line":"            The expected instance.task_state is \"resize_migrating\" when calling"}],"source_content_type":"text/x-python","patch_set":45,"id":"3fa7e38b_e3c85e56","line":4656,"in_reply_to":"7faddb67_861f06b6","updated":"2019-09-18 20:44:39.000000000","message":"That\u0027s probably worthwhile, can be done in a FUP.","commit_id":"612063d1f1e65add34aa90f05dbc043ec797a97b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"3825a152f4a92f6a2436f8c9872a34f12bb87e96","unresolved":false,"context_lines":[{"line_number":4688,"context_line":"                  instance\u003dinstance)"},{"line_number":4689,"context_line":"        # Note that we do not track the snapshot phase task states"},{"line_number":4690,"context_line":"        # during resize since we do not want to reflect those into the"},{"line_number":4691,"context_line":"        # actual instance.task_state."},{"line_number":4692,"context_line":"        update_task_state \u003d lambda *args, **kwargs: None"},{"line_number":4693,"context_line":"        with timeutils.StopWatch() as timer:"},{"line_number":4694,"context_line":"            self.driver.snapshot(ctxt, instance, image_id, update_task_state)"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_268412d3","line":4691,"updated":"2019-08-28 14:16:55.000000000","message":"++","commit_id":"612063d1f1e65add34aa90f05dbc043ec797a97b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"3825a152f4a92f6a2436f8c9872a34f12bb87e96","unresolved":false,"context_lines":[{"line_number":4717,"context_line":"        LOG.debug(\u0027Stopping instance\u0027, instance\u003dinstance)"},{"line_number":4718,"context_line":"        try:"},{"line_number":4719,"context_line":"            self._power_off_instance(ctxt, instance)"},{"line_number":4720,"context_line":"        except Exception as e:"},{"line_number":4721,"context_line":"            LOG.exception(\u0027Failed to power off instance.\u0027, instance\u003dinstance)"},{"line_number":4722,"context_line":"            raise exception.InstancePowerOffFailure(reason\u003dsix.text_type(e))"},{"line_number":4723,"context_line":"        instance.power_state \u003d self._get_power_state(ctxt, instance)"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_26d2d2d2","line":4720,"range":{"start_line":4720,"start_character":15,"end_line":4720,"end_character":24},"updated":"2019-08-28 14:16:55.000000000","message":"note to self: We can\u0027t make this more narrow since this calls a driver method (\u0027power_off\u0027) and we (unfortunately) don\u0027t say what exceptions are allowed to be raised by a driver\n\nother note to self: it\u0027d be lovely if Python has a \u0027raises\u0027 or \u0027throws\u0027 decorators that we could enforce somehow","commit_id":"612063d1f1e65add34aa90f05dbc043ec797a97b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3900bc3c838b46a7f7f3da69af4a2ab76c027d91","unresolved":false,"context_lines":[{"line_number":4717,"context_line":"        LOG.debug(\u0027Stopping instance\u0027, instance\u003dinstance)"},{"line_number":4718,"context_line":"        try:"},{"line_number":4719,"context_line":"            self._power_off_instance(ctxt, instance)"},{"line_number":4720,"context_line":"        except Exception as e:"},{"line_number":4721,"context_line":"            LOG.exception(\u0027Failed to power off instance.\u0027, instance\u003dinstance)"},{"line_number":4722,"context_line":"            raise exception.InstancePowerOffFailure(reason\u003dsix.text_type(e))"},{"line_number":4723,"context_line":"        instance.power_state \u003d self._get_power_state(ctxt, instance)"}],"source_content_type":"text/x-python","patch_set":45,"id":"3fa7e38b_03b25ae7","line":4720,"range":{"start_line":4720,"start_character":15,"end_line":4720,"end_character":24},"in_reply_to":"7faddb67_26d2d2d2","updated":"2019-09-18 20:44:39.000000000","message":"Java evolved into C++ and eventually Python will evolve into Java so don\u0027t worry, it\u0027ll happen, maybe by Python 5.","commit_id":"612063d1f1e65add34aa90f05dbc043ec797a97b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d8a8133864be39e7f9631271078c83ebeb5185cd","unresolved":false,"context_lines":[{"line_number":4908,"context_line":"        # created in conductor should be reverted by conductor as well,"},{"line_number":4909,"context_line":"        # see MigrationTask.rollback."},{"line_number":4910,"context_line":"        self._prep_snapshot_based_resize_at_source("},{"line_number":4911,"context_line":"            ctxt, instance, migration, snapshot_id\u003dsnapshot_id)"},{"line_number":4912,"context_line":""},{"line_number":4913,"context_line":"    @delete_image_on_error"},{"line_number":4914,"context_line":"    def _snapshot_for_resize(self, ctxt, image_id, instance):"}],"source_content_type":"text/x-python","patch_set":51,"id":"3fa7e38b_4ec77265","line":4911,"updated":"2019-10-15 14:36:56.000000000","message":"I\u0027m not sure I see the point of a single-statement method here. It puts the docstring and decorators here, but the actual work it describes is two methods away, and probably two screens in my editor.","commit_id":"fa487daf093a8b80a6af9aa31957c5c09de5df03"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"32c8dd59560c02456a403d00c6332aed4fda0aaa","unresolved":false,"context_lines":[{"line_number":4908,"context_line":"        # created in conductor should be reverted by conductor as well,"},{"line_number":4909,"context_line":"        # see MigrationTask.rollback."},{"line_number":4910,"context_line":"        self._prep_snapshot_based_resize_at_source("},{"line_number":4911,"context_line":"            ctxt, instance, migration, snapshot_id\u003dsnapshot_id)"},{"line_number":4912,"context_line":""},{"line_number":4913,"context_line":"    @delete_image_on_error"},{"line_number":4914,"context_line":"    def _snapshot_for_resize(self, ctxt, image_id, instance):"}],"source_content_type":"text/x-python","patch_set":51,"id":"3fa7e38b_c6c82223","line":4911,"in_reply_to":"3fa7e38b_4ec77265","updated":"2019-10-15 20:32:03.000000000","message":"It\u0027s mostly just a pattern for the rest of these new methods in this series because the others have try/except handlers around them for doing things like cleaning up allocations or setting the instance to ERROR state on failure, e.g.:\n\nhttps://review.opendev.org/#/c/635080/46/nova/compute/manager.py@5377\n\nIt also makes testing the various decorators separately from the rest of the meat of the operation simpler. In general it\u0027s something I\u0027ve tried doing with this new stuff because of annoyances and cruft built up over the same-cell resize methods over the years, i.e. it is very cumbersome to unit test those existing methods because there is so much going on in one place. Having said that, _prep_snapshot_based_resize_at_source isn\u0027t much better because it still does a ton of crap.","commit_id":"fa487daf093a8b80a6af9aa31957c5c09de5df03"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d8a8133864be39e7f9631271078c83ebeb5185cd","unresolved":false,"context_lines":[{"line_number":4955,"context_line":"            self._power_off_instance(ctxt, instance)"},{"line_number":4956,"context_line":"        except Exception as e:"},{"line_number":4957,"context_line":"            LOG.exception(\u0027Failed to power off instance.\u0027, instance\u003dinstance)"},{"line_number":4958,"context_line":"            raise exception.InstancePowerOffFailure(reason\u003dsix.text_type(e))"},{"line_number":4959,"context_line":"        instance.power_state \u003d self._get_power_state(ctxt, instance)"},{"line_number":4960,"context_line":""},{"line_number":4961,"context_line":"        # If a snapshot image ID was provided, we need to snapshot the guest"}],"source_content_type":"text/x-python","patch_set":51,"id":"3fa7e38b_6ed34e02","line":4958,"updated":"2019-10-15 14:36:56.000000000","message":"I guess this is just to get a consistent exception in the conductor for anything driver-related?","commit_id":"fa487daf093a8b80a6af9aa31957c5c09de5df03"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"32c8dd59560c02456a403d00c6332aed4fda0aaa","unresolved":false,"context_lines":[{"line_number":4955,"context_line":"            self._power_off_instance(ctxt, instance)"},{"line_number":4956,"context_line":"        except Exception as e:"},{"line_number":4957,"context_line":"            LOG.exception(\u0027Failed to power off instance.\u0027, instance\u003dinstance)"},{"line_number":4958,"context_line":"            raise exception.InstancePowerOffFailure(reason\u003dsix.text_type(e))"},{"line_number":4959,"context_line":"        instance.power_state \u003d self._get_power_state(ctxt, instance)"},{"line_number":4960,"context_line":""},{"line_number":4961,"context_line":"        # If a snapshot image ID was provided, we need to snapshot the guest"}],"source_content_type":"text/x-python","patch_set":51,"id":"3fa7e38b_66d7eec0","line":4958,"in_reply_to":"3fa7e38b_6ed34e02","updated":"2019-10-15 20:32:03.000000000","message":"Yeah. Much earlier when I was still hacking on the conductor pieces I toyed with the idea of auto-restarting the instance on rollback if we got here and power_off failed.","commit_id":"fa487daf093a8b80a6af9aa31957c5c09de5df03"}],"nova/compute/rpcapi.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d8a8133864be39e7f9631271078c83ebeb5185cd","unresolved":false,"context_lines":[{"line_number":925,"context_line":"            raise exception.MigrationError(reason\u003d_(\u0027Compute too old\u0027))"},{"line_number":926,"context_line":"        cctxt \u003d client.prepare(server\u003d_compute_host(None, instance),"},{"line_number":927,"context_line":"                               version\u003dversion,"},{"line_number":928,"context_line":"                               call_monitor_timeout\u003dCONF.rpc_response_timeout,"},{"line_number":929,"context_line":"                               timeout\u003dCONF.long_rpc_timeout)"},{"line_number":930,"context_line":"        return cctxt.call("},{"line_number":931,"context_line":"            ctxt, \u0027prep_snapshot_based_resize_at_source\u0027,"}],"source_content_type":"text/x-python","patch_set":51,"id":"3fa7e38b_6eecaeba","line":928,"updated":"2019-10-15 14:36:56.000000000","message":"Makes sense for this one to be a long-call. I guess you didn\u0027t update the conf because you put a \"snapshot-based resize\" umbrella statement in there in the earlier patch...","commit_id":"fa487daf093a8b80a6af9aa31957c5c09de5df03"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"32c8dd59560c02456a403d00c6332aed4fda0aaa","unresolved":false,"context_lines":[{"line_number":925,"context_line":"            raise exception.MigrationError(reason\u003d_(\u0027Compute too old\u0027))"},{"line_number":926,"context_line":"        cctxt \u003d client.prepare(server\u003d_compute_host(None, instance),"},{"line_number":927,"context_line":"                               version\u003dversion,"},{"line_number":928,"context_line":"                               call_monitor_timeout\u003dCONF.rpc_response_timeout,"},{"line_number":929,"context_line":"                               timeout\u003dCONF.long_rpc_timeout)"},{"line_number":930,"context_line":"        return cctxt.call("},{"line_number":931,"context_line":"            ctxt, \u0027prep_snapshot_based_resize_at_source\u0027,"}],"source_content_type":"text/x-python","patch_set":51,"id":"3fa7e38b_86e78aac","line":928,"in_reply_to":"3fa7e38b_6eecaeba","updated":"2019-10-15 20:32:03.000000000","message":"\u003e I guess you didn\u0027t update the conf because you put a \"snapshot-based resize\" umbrella\n \u003e statement in there in the earlier patch...\n\nYup, I intentionally used generic wording for that so I wouldn\u0027t have to update it for every little detailed RPC method in the series.","commit_id":"fa487daf093a8b80a6af9aa31957c5c09de5df03"}],"nova/objects/service.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f3daad04aaf6defd354be0167f2969e93e5924f2","unresolved":false,"context_lines":[{"line_number":153,"context_line":"    {\u0027compute_rpc\u0027: \u00275.1\u0027},"},{"line_number":154,"context_line":"    # Version 38: Compute RPC version 5.1; +prep_snapshot_based_resize_at_dest"},{"line_number":155,"context_line":"    {\u0027compute_rpc\u0027: \u00275.2\u0027},"},{"line_number":156,"context_line":"    # Version 39: Compute RPC version 5.2: prep_snapshot_based_resize_at_source"},{"line_number":157,"context_line":"    {\u0027compute_rpc\u0027: \u00275.3\u0027},"},{"line_number":158,"context_line":")"},{"line_number":159,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"9fdfeff1_541d7ebb","line":156,"range":{"start_line":156,"start_character":38,"end_line":156,"end_character":41},"updated":"2019-02-05 22:16:44.000000000","message":"5.3","commit_id":"f937eb9778cb9c4550aca54e9d1fb33039aaccd8"}]}