)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"9980f53698704cb35a113e04cdeee75760e9e57a","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     LuyaoZhong \u003cluyao.zhong@intel.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2020-03-25 03:00:30 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"bug-fix: set do_cleanup always True for libvirt driver"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"For libvirt, do_cleanup is just a flag to mark if the instance_path is shared."},{"line_number":10,"context_line":"If the instance path is shared, that means we shouldn\u0027t cleanup the target or"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df33271e_59d7acc0","line":7,"range":{"start_line":7,"start_character":0,"end_line":7,"end_character":7},"updated":"2020-03-25 10:30:58.000000000","message":"If this is a bugfix can you write up a bug report with a full description of the issue at hand here?","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"},{"author":{"_account_id":23598,"name":"Zhong Luyao","email":"luyao.zhong@intel.com","username":"ZhongLuyao"},"change_message_id":"711461143996e9a7fe916652623dce69a31674bd","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     LuyaoZhong \u003cluyao.zhong@intel.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2020-03-25 03:00:30 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"bug-fix: set do_cleanup always True for libvirt driver"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"For libvirt, do_cleanup is just a flag to mark if the instance_path is shared."},{"line_number":10,"context_line":"If the instance path is shared, that means we shouldn\u0027t cleanup the target or"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df33271e_3f81e790","line":7,"range":{"start_line":7,"start_character":0,"end_line":7,"end_character":7},"in_reply_to":"df33271e_59d7acc0","updated":"2020-03-25 15:11:45.000000000","message":"According to your comments, I look into the code again, it might not be a bug I think, just some confusing codes which need refactoring.","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"9980f53698704cb35a113e04cdeee75760e9e57a","unresolved":false,"context_lines":[{"line_number":12,"context_line":"flag."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"We have two operations based on this do_cleanup flag:"},{"line_number":15,"context_line":"   a) rpc call \u0027rollback_live_migration_at_destination\u0027 on source host when"},{"line_number":16,"context_line":"      rollback live migration"},{"line_number":17,"context_line":"   b) driver.cleanup on source host when post live migration"},{"line_number":18,"context_line":"Both a) and b) have other logic like undefine the domain, disconnect volume or"},{"line_number":19,"context_line":"destory vif except for cleanup instance path. These operations shouldn\u0027t be"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df33271e_f9bde068","line":16,"range":{"start_line":15,"start_character":0,"end_line":16,"end_character":29},"updated":"2020-03-25 10:30:58.000000000","message":"As the name suggests that\u0027s used on the destination after a failure not the source.","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"},{"author":{"_account_id":23598,"name":"Zhong Luyao","email":"luyao.zhong@intel.com","username":"ZhongLuyao"},"change_message_id":"711461143996e9a7fe916652623dce69a31674bd","unresolved":false,"context_lines":[{"line_number":12,"context_line":"flag."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"We have two operations based on this do_cleanup flag:"},{"line_number":15,"context_line":"   a) rpc call \u0027rollback_live_migration_at_destination\u0027 on source host when"},{"line_number":16,"context_line":"      rollback live migration"},{"line_number":17,"context_line":"   b) driver.cleanup on source host when post live migration"},{"line_number":18,"context_line":"Both a) and b) have other logic like undefine the domain, disconnect volume or"},{"line_number":19,"context_line":"destory vif except for cleanup instance path. These operations shouldn\u0027t be"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df33271e_9c73e1b1","line":16,"range":{"start_line":15,"start_character":0,"end_line":16,"end_character":29},"in_reply_to":"df33271e_f9bde068","updated":"2020-03-25 15:11:45.000000000","message":"I mean this rpc call is sent out from source host, sorry I didn\u0027t make it clear.","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"9980f53698704cb35a113e04cdeee75760e9e57a","unresolved":false,"context_lines":[{"line_number":15,"context_line":"   a) rpc call \u0027rollback_live_migration_at_destination\u0027 on source host when"},{"line_number":16,"context_line":"      rollback live migration"},{"line_number":17,"context_line":"   b) driver.cleanup on source host when post live migration"},{"line_number":18,"context_line":"Both a) and b) have other logic like undefine the domain, disconnect volume or"},{"line_number":19,"context_line":"destory vif except for cleanup instance path. These operations shouldn\u0027t be"},{"line_number":20,"context_line":"controlled by only do_cleanup flag."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"So set do_cleanup always True for libvirt driver to complete other cleanup"},{"line_number":23,"context_line":"except for deleting instance path."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df33271e_f94b2084","line":20,"range":{"start_line":18,"start_character":0,"end_line":20,"end_character":35},"updated":"2020-03-25 10:30:58.000000000","message":"I agree this needs to be cleaned up but I don\u0027t think defaulting do_cleanup to True is the way to do this.\n\nWe are duplicating logic in cleanup that\u0027s also present in other parts of the migration path.\n\nI\u0027d rather see a set live_migration_cleanup_source|destination methods that explicitly cleanup things given if the instance path or disks are shared.","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"},{"author":{"_account_id":23598,"name":"Zhong Luyao","email":"luyao.zhong@intel.com","username":"ZhongLuyao"},"change_message_id":"711461143996e9a7fe916652623dce69a31674bd","unresolved":false,"context_lines":[{"line_number":15,"context_line":"   a) rpc call \u0027rollback_live_migration_at_destination\u0027 on source host when"},{"line_number":16,"context_line":"      rollback live migration"},{"line_number":17,"context_line":"   b) driver.cleanup on source host when post live migration"},{"line_number":18,"context_line":"Both a) and b) have other logic like undefine the domain, disconnect volume or"},{"line_number":19,"context_line":"destory vif except for cleanup instance path. These operations shouldn\u0027t be"},{"line_number":20,"context_line":"controlled by only do_cleanup flag."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"So set do_cleanup always True for libvirt driver to complete other cleanup"},{"line_number":23,"context_line":"except for deleting instance path."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df33271e_ada82ddb","line":20,"range":{"start_line":18,"start_character":0,"end_line":20,"end_character":35},"in_reply_to":"df33271e_f94b2084","updated":"2020-03-25 15:11:45.000000000","message":"yes there are duplicating logic, if set do_cleanup to True (I go into some code pathes)\nfor a), those duplicating logic might be called, like volume disconnection, but I think it will not break anything(exception will be captured and print some logs)\nfor b), those duplicating logic will not be called again, since we have several params like \u0027destroy_vifs\u0027 to control if we need unplug vifs and \u0027block_device_info\u0027 to control volume disconnection.\n\nif set do_cleanup to False, a) will not be called, _rollback_live_migration is lack of some logic like free_pci_device_claims_for_instance[1] which a) has. I\u0027m not sure if this can be called a bug.\n\nYour suggestion is reasonable, we should put other cleanup tasks inside another separate method but not make them into such chaos or confusing or unreadable. \n\nThis change/refactor must be huge~~~, and need many tests.\n\n[1]https://github.com/openstack/nova/blob/f454e1dec9580abf4605e071bdd678a40f492a49/nova/compute/manager.py#L8785","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"}],"doc/notification_samples/instance-live_migration_post-end.json":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"9980f53698704cb35a113e04cdeee75760e9e57a","unresolved":false,"context_lines":[{"line_number":3,"context_line":"    \"payload\": {"},{"line_number":4,"context_line":"        \"$ref\": \"common_payloads/InstanceActionPayload.json#\","},{"line_number":5,"context_line":"        \"nova_object.data\": {"},{"line_number":6,"context_line":"            \"host\": \"host2\","},{"line_number":7,"context_line":"            \"node\": \"host2\","},{"line_number":8,"context_line":"            \"action_initiator_user\": \"admin\""},{"line_number":9,"context_line":"        }"},{"line_number":10,"context_line":"    },"},{"line_number":11,"context_line":"    \"priority\": \"INFO\","}],"source_content_type":"application/json","patch_set":3,"id":"df33271e_d9f45c5e","line":8,"range":{"start_line":6,"start_character":0,"end_line":8,"end_character":44},"updated":"2020-03-25 10:30:58.000000000","message":"Why are we changing this?","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"},{"author":{"_account_id":23598,"name":"Zhong Luyao","email":"luyao.zhong@intel.com","username":"ZhongLuyao"},"change_message_id":"711461143996e9a7fe916652623dce69a31674bd","unresolved":false,"context_lines":[{"line_number":3,"context_line":"    \"payload\": {"},{"line_number":4,"context_line":"        \"$ref\": \"common_payloads/InstanceActionPayload.json#\","},{"line_number":5,"context_line":"        \"nova_object.data\": {"},{"line_number":6,"context_line":"            \"host\": \"host2\","},{"line_number":7,"context_line":"            \"node\": \"host2\","},{"line_number":8,"context_line":"            \"action_initiator_user\": \"admin\""},{"line_number":9,"context_line":"        }"},{"line_number":10,"context_line":"    },"},{"line_number":11,"context_line":"    \"priority\": \"INFO\","}],"source_content_type":"application/json","patch_set":3,"id":"df33271e_0d74b967","line":8,"range":{"start_line":6,"start_character":0,"end_line":8,"end_character":44},"in_reply_to":"df33271e_d9f45c5e","updated":"2020-03-25 15:11:45.000000000","message":"We will get different results when setting do_cleanup to True/False. Obviously, current functional test don\u0027t go to here[1] since do_cleanup is False. instance.host is updated to dest host when post_live_migration_at_destination[2], so refresh instance will get the instance from db, ortherwise from memory cache.\n\n[1]https://github.com/openstack/nova/blob/f454e1dec9580abf4605e071bdd678a40f492a49/nova/compute/manager.py#L8372\n[2]https://github.com/openstack/nova/blob/f454e1dec9580abf4605e071bdd678a40f492a49/nova/compute/manager.py#L8350","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"}],"nova/compute/manager.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"9980f53698704cb35a113e04cdeee75760e9e57a","unresolved":false,"context_lines":[{"line_number":8187,"context_line":"        do_cleanup \u003d False"},{"line_number":8188,"context_line":"        destroy_disks \u003d False"},{"line_number":8189,"context_line":"        if isinstance(migrate_data, migrate_data_obj.LibvirtLiveMigrateData):"},{"line_number":8190,"context_line":"            # NOTE(luyao): We need to cleanup any zombie Planned VM"},{"line_number":8191,"context_line":"            do_cleanup \u003d True"},{"line_number":8192,"context_line":"            destroy_disks \u003d not migrate_data.is_shared_block_storage"},{"line_number":8193,"context_line":"        elif isinstance(migrate_data, migrate_data_obj.XenapiLiveMigrateData):"},{"line_number":8194,"context_line":"            do_cleanup \u003d migrate_data.block_migration"}],"source_content_type":"text/x-python","patch_set":3,"id":"df33271e_b9fd183c","line":8191,"range":{"start_line":8190,"start_character":0,"end_line":8191,"end_character":29},"updated":"2020-03-25 10:30:58.000000000","message":"I don\u0027t follow this logic, in both cases where this is used by the libvirt driver libvirtd itself will destroy the domains for us. Either on the source after a successful migration or on the destination after a failure.","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"}],"nova/tests/functional/notification_sample_tests/test_instance.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"9980f53698704cb35a113e04cdeee75760e9e57a","unresolved":false,"context_lines":[{"line_number":209,"context_line":"        # 5. instance.live_migration_abort.end"},{"line_number":210,"context_line":"        # 6. instance.live_migration_rollback.start"},{"line_number":211,"context_line":"        # 7. instance.live_migration_rollback.end"},{"line_number":212,"context_line":"        # 8. instance.live_migration_rollback_dest.start"},{"line_number":213,"context_line":"        # 9. instance.live_migration_rollback_dest.end"},{"line_number":214,"context_line":"        self.assertEqual(10, len(fake_notifier.VERSIONED_NOTIFICATIONS),"},{"line_number":215,"context_line":"                         [x[\u0027event_type\u0027] for x in"},{"line_number":216,"context_line":"                          fake_notifier.VERSIONED_NOTIFICATIONS])"},{"line_number":217,"context_line":"        self._verify_notification("}],"source_content_type":"text/x-python","patch_set":3,"id":"df33271e_f994c0ea","line":214,"range":{"start_line":212,"start_character":0,"end_line":214,"end_character":72},"updated":"2020-03-25 10:30:58.000000000","message":"Why are we changing this?","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"},{"author":{"_account_id":23598,"name":"Zhong Luyao","email":"luyao.zhong@intel.com","username":"ZhongLuyao"},"change_message_id":"711461143996e9a7fe916652623dce69a31674bd","unresolved":false,"context_lines":[{"line_number":209,"context_line":"        # 5. instance.live_migration_abort.end"},{"line_number":210,"context_line":"        # 6. instance.live_migration_rollback.start"},{"line_number":211,"context_line":"        # 7. instance.live_migration_rollback.end"},{"line_number":212,"context_line":"        # 8. instance.live_migration_rollback_dest.start"},{"line_number":213,"context_line":"        # 9. instance.live_migration_rollback_dest.end"},{"line_number":214,"context_line":"        self.assertEqual(10, len(fake_notifier.VERSIONED_NOTIFICATIONS),"},{"line_number":215,"context_line":"                         [x[\u0027event_type\u0027] for x in"},{"line_number":216,"context_line":"                          fake_notifier.VERSIONED_NOTIFICATIONS])"},{"line_number":217,"context_line":"        self._verify_notification("}],"source_content_type":"text/x-python","patch_set":3,"id":"df33271e_edcb5547","line":214,"range":{"start_line":212,"start_character":0,"end_line":214,"end_character":72},"in_reply_to":"df33271e_f994c0ea","updated":"2020-03-25 15:11:45.000000000","message":"if settting do_cleanup always True, it will invoke rollback_live_migration_at_destination.","commit_id":"a5aa4dbf4140389d1e02eb4bb010504142604f94"}],"nova/virt/fake.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"9980f53698704cb35a113e04cdeee75760e9e57a","unresolved":false,"context_lines":[{"line_number":320,"context_line":"    def cleanup(self, context, instance, network_info, block_device_info\u003dNone,"},{"line_number":321,"context_line":"                destroy_disks\u003dTrue, migrate_data\u003dNone, destroy_vifs\u003dTrue):"},{"line_number":322,"context_line":"        # cleanup() should not be called when the guest has not been destroyed."},{"line_number":323,"context_line":"        if instance.uuid in self.instances:"},{"line_number":324,"context_line":"            raise exception.InstanceExists("},{"line_number":325,"context_line":"                \"Instance %s has not been destroyed.\" % instance.uuid)"},{"line_number":326,"context_line":""},{"line_number":327,"context_line":"    def attach_volume(self, context, connection_info, instance, mountpoint,"},{"line_number":328,"context_line":"                      disk_bus\u003dNone, device_type\u003dNone, encryption\u003dNone):"}],"source_content_type":"text/x-python","patch_set":3,"id":"df33271e_5924cc0d","side":"PARENT","line":325,"range":{"start_line":323,"start_character":0,"end_line":325,"end_character":70},"updated":"2020-03-25 10:30:58.000000000","message":"This is a valid check during post live migration FWIW.\n\nIn both cases where this is used libvirtd has already removed the domain for us in this case so we shouldn\u0027t find instance.uuid in self.instances.","commit_id":"f454e1dec9580abf4605e071bdd678a40f492a49"},{"author":{"_account_id":23598,"name":"Zhong Luyao","email":"luyao.zhong@intel.com","username":"ZhongLuyao"},"change_message_id":"711461143996e9a7fe916652623dce69a31674bd","unresolved":false,"context_lines":[{"line_number":320,"context_line":"    def cleanup(self, context, instance, network_info, block_device_info\u003dNone,"},{"line_number":321,"context_line":"                destroy_disks\u003dTrue, migrate_data\u003dNone, destroy_vifs\u003dTrue):"},{"line_number":322,"context_line":"        # cleanup() should not be called when the guest has not been destroyed."},{"line_number":323,"context_line":"        if instance.uuid in self.instances:"},{"line_number":324,"context_line":"            raise exception.InstanceExists("},{"line_number":325,"context_line":"                \"Instance %s has not been destroyed.\" % instance.uuid)"},{"line_number":326,"context_line":""},{"line_number":327,"context_line":"    def attach_volume(self, context, connection_info, instance, mountpoint,"},{"line_number":328,"context_line":"                      disk_bus\u003dNone, device_type\u003dNone, encryption\u003dNone):"}],"source_content_type":"text/x-python","patch_set":3,"id":"df33271e_2d1d3dbd","side":"PARENT","line":325,"range":{"start_line":323,"start_character":0,"end_line":325,"end_character":70},"in_reply_to":"df33271e_5924cc0d","updated":"2020-03-25 15:11:45.000000000","message":"This is a fake driver, not a real libvirt. So instance will still be there.","commit_id":"f454e1dec9580abf4605e071bdd678a40f492a49"}]}