Revert "Revert resize: wait for events according to hybrid plug"

This reverts commit 7a7a223.

That commit was added because - tl'dr - upon revert resize, Neutron
with the OVS backend and the iptables security group driver would send
us the network-vif-plugged event as soon as we updated the port
binding.

That behaviour has changed with commit 66c7f00. With that commit,
we started unplugging the vifs on the source compute host when doing a
resize. When reverting the resize, the vifs had to be re-plugged again,
regarldess of the networking backend in use. This renders commit
7a7a223. pointless, and it can be
reverted.

Conflicts - most have to do with context around this commit's code:

nova/compute/manager.py

    a2984b6 added provider_mappings to
    _finish_revert_resize_network_migrate_finish()'s signature

    750aef5 started using
    _finish_revert_resize_network_migrate_finish() in
    _finish_revert_snapshot_based_resize_at_source()

nova/network/model.py

    8b33ac0 added get_live_migration_plug_time_events() and
    has_live_migration_plug_time_event()

    7da9444 added has_port_with_allocation()

nova/objects/migration.py

    f203da3 added is_resize() and is_live_migration()

nova/tests/unit/compute/test_compute.py

    a0e60fe added request_spec to the test

nova/tests/unit/compute/test_compute_mgr.py

    be27800 added unit tests below ours

nova/tests/unit/network/test_network_info.py

    7da9444 (again) added tests for has_port_with_allocation()

nova/tests/unit/virt/libvirt/test_driver.py and
nova/virt/libvirt/driver.py are different in that attempting to
identify individual conflicts is a pointless exercise, as so much has
changed (mdev, vtmp, the recent wait for events during hard reboot
workaround config option, etc). They can be treated as
manual removal of any code that had to do with the bind-time events
logic (though guided by the conflict markers in git).

TODO(artom) There was a follow up commit,
78a08d4, that added the migration
parameter to finish_revert_migration(). This is no longer needed, as
the migration was only used to obtain plug-time events. We'll have to
undo that as well.

Closes-bug: 1952003
Change-Id: I3cb39a9ec2c260f422b3c48122b9db512cdd799b

Author: notartom

.zuul.yaml

@@ -605,8 +605,7 @@
         - nova-lvm
         - nova-multi-cell
         - nova-next
-        - nova-ovs-hybrid-plug:
-            voting: false
+        - nova-ovs-hybrid-plug
         - nova-tox-validate-backport:
             voting: false
         - nova-tox-functional-centos8-py36

nova/compute/manager.py

@@ -4809,8 +4809,18 @@ def _finish_revert_snapshot_based_resize_at_source(
                   self.host, instance=instance)
         # TODO(mriedem): Calculate provider mappings when we support
         # cross-cell resize/migrate with ports having resource requests.
-        self._finish_revert_resize_network_migrate_finish(
-            ctxt, instance, migration, provider_mappings=None)
+        # NOTE(hanrong): we need to change migration.dest_compute to
+        # source host temporarily.
+        # "network_api.migrate_instance_finish" will setup the network
+        # for the instance on the destination host. For revert resize,
+        # the instance will back to the source host, the setup of the
+        # network for instance should be on the source host. So set
+        # the migration.dest_compute to source host at here.
+        with utils.temporary_mutation(
+            migration, dest_compute=migration.source_compute
+        ):
+            self.network_api.migrate_instance_finish(
+                ctxt, instance, migration, provider_mappings=None)
         network_info = self.network_api.get_instance_nw_info(ctxt, instance)
 
         # Remember that prep_snapshot_based_resize_at_source destroyed the
@@ -4902,50 +4912,6 @@ def revert_resize(self, context, instance, migration, request_spec):
             self.compute_rpcapi.finish_revert_resize(context, instance,
                     migration, migration.source_compute, request_spec)
 
-    def _finish_revert_resize_network_migrate_finish(
-            self, context, instance, migration, provider_mappings):
-        """Causes port binding to be updated. In some Neutron or port
-        configurations - see NetworkModel.get_bind_time_events() - we
-        expect the vif-plugged event from Neutron immediately and wait for it.
-        The rest of the time, the event is expected further along in the
-        virt driver, so we don't wait here.
-
-        :param context: The request context.
-        :param instance: The instance undergoing the revert resize.
-        :param migration: The Migration object of the resize being reverted.
-        :param provider_mappings: a dict of list of resource provider uuids
-            keyed by port uuid
-        :raises: eventlet.timeout.Timeout or
-                 exception.VirtualInterfacePlugException.
-        """
-        network_info = instance.get_network_info()
-        events = []
-        deadline = CONF.vif_plugging_timeout
-        if deadline and network_info:
-            events = network_info.get_bind_time_events(migration)
-            if events:
-                LOG.debug('Will wait for bind-time events: %s', events)
-        error_cb = self._neutron_failed_migration_callback
-        try:
-            with self.virtapi.wait_for_instance_event(instance, events,
-                                                      deadline=deadline,
-                                                      error_callback=error_cb):
-                # NOTE(hanrong): we need to change migration.dest_compute to
-                # source host temporarily.
-                # "network_api.migrate_instance_finish" will setup the network
-                # for the instance on the destination host. For revert resize,
-                # the instance will back to the source host, the setup of the
-                # network for instance should be on the source host. So set
-                # the migration.dest_compute to source host at here.
-                with utils.temporary_mutation(
-                        migration, dest_compute=migration.source_compute):
-                    self.network_api.migrate_instance_finish(
-                        context, instance, migration, provider_mappings)
-        except eventlet.timeout.Timeout:
-            with excutils.save_and_reraise_exception():
-                LOG.error('Timeout waiting for Neutron events: %s', events,
-                          instance=instance)
-
     @wrap_exception()
     @reverts_task_state
     @wrap_instance_event(prefix='compute')
@@ -5003,8 +4969,18 @@ def _finish_revert_resize(
 
             self.network_api.setup_networks_on_host(context, instance,
                                                     migration.source_compute)
-            self._finish_revert_resize_network_migrate_finish(
-                context, instance, migration, provider_mappings)
+            # NOTE(hanrong): we need to change migration.dest_compute to
+            # source host temporarily. "network_api.migrate_instance_finish"
+            # will setup the network for the instance on the destination host.
+            # For revert resize, the instance will back to the source host, the
+            # setup of the network for instance should be on the source host.
+            # So set the migration.dest_compute to source host at here.
+            with utils.temporary_mutation(
+                    migration, dest_compute=migration.source_compute):
+                self.network_api.migrate_instance_finish(context,
+                                                         instance,
+                                                         migration,
+                                                         provider_mappings)
             network_info = self.network_api.get_instance_nw_info(context,
                                                                  instance)
 
@@ -5081,8 +5057,7 @@ def _fill_provider_mapping_based_on_allocs(
             # the provider mappings. If the instance has ports with
             # resource request then the port update will fail in
             # _update_port_binding_for_instance() called via
-            # _finish_revert_resize_network_migrate_finish() in
-            # finish_revert_resize.
+            # migrate_instance_finish() in finish_revert_resize.
             provider_mappings = None
         return provider_mappings
 
@@ -8306,8 +8281,8 @@ def pre_live_migration(self, context, instance, disk, migrate_data):
         return migrate_data
 
     @staticmethod
-    def _neutron_failed_migration_callback(event_name, instance):
-        msg = ('Neutron reported failure during migration '
+    def _neutron_failed_live_migration_callback(event_name, instance):
+        msg = ('Neutron reported failure during live migration '
                'with %(event)s for instance %(uuid)s')
         msg_args = {'event': event_name, 'uuid': instance.uuid}
         if CONF.vif_plugging_is_fatal:
@@ -8403,7 +8378,7 @@ class _BreakWaitForInstanceEvent(Exception):
                 disk = None
 
             deadline = CONF.vif_plugging_timeout
-            error_cb = self._neutron_failed_migration_callback
+            error_cb = self._neutron_failed_live_migration_callback
             # In order to avoid a race with the vif plugging that the virt
             # driver does on the destination host, we register our events
             # to wait for before calling pre_live_migration. Then if the

nova/network/model.py

@@ -485,17 +485,6 @@ def labeled_ips(self):
                     'ips': ips}
         return []
 
-    def has_bind_time_event(self, migration):
-        """Returns whether this VIF's network-vif-plugged external event will
-        be sent by Neutron at "bind-time" - in other words, as soon as the port
-        binding is updated. This is in the context of updating the port binding
-        to a host that already has the instance in a shutoff state - in
-        practice, this means reverting either a cold migration or a
-        non-same-host resize.
-        """
-        return (self.is_hybrid_plug_enabled() and not
-                migration.is_same_host())
-
     @property
     def has_live_migration_plug_time_event(self):
         """Returns whether this VIF's network-vif-plugged external event will
@@ -564,27 +553,13 @@ def wait(self, do_raise=True):
     def json(self):
         return jsonutils.dumps(self)
 
-    def get_bind_time_events(self, migration):
-        """Returns a list of external events for any VIFs that have
-        "bind-time" events during cold migration.
-        """
-        return [('network-vif-plugged', vif['id'])
-                for vif in self if vif.has_bind_time_event(migration)]
-
     def get_live_migration_plug_time_events(self):
         """Returns a list of external events for any VIFs that have
         "plug-time" events during live migration.
         """
         return [('network-vif-plugged', vif['id'])
                 for vif in self if vif.has_live_migration_plug_time_event]
 
-    def get_plug_time_events(self, migration):
-        """Returns a list of external events for any VIFs that have
-        "plug-time" events during cold migration.
-        """
-        return [('network-vif-plugged', vif['id'])
-                for vif in self if not vif.has_bind_time_event(migration)]
-
     def has_port_with_allocation(self):
         return any(vif.has_allocation() for vif in self)
 

nova/objects/migration.py

@@ -202,9 +202,6 @@ def instance(self):
     def instance(self, instance):
         self._cached_instance = instance
 
-    def is_same_host(self):
-        return self.source_compute == self.dest_compute
-
     @property
     def is_live_migration(self):
         return self.migration_type == fields.MigrationType.LIVE_MIGRATION

nova/tests/unit/compute/test_compute.py

@@ -5801,9 +5801,7 @@ def fake_finish_revert_migration_driver(*args, **kwargs):
             old_vm_state = vm_states.ACTIVE
         else:
             old_vm_state = vm_states.STOPPED
-        params = {'vm_state': old_vm_state,
-                  'info_cache': objects.InstanceInfoCache(
-                      network_info=network_model.NetworkInfo([]))}
+        params = {'vm_state': old_vm_state}
         instance = self._create_fake_instance_obj(params)
         request_spec = objects.RequestSpec()
 
@@ -5956,9 +5954,7 @@ def test_finish_revert_resize_validate_source_compute(self):
         def fake(*args, **kwargs):
             pass
 
-        params = {'info_cache': objects.InstanceInfoCache(
-                      network_info=network_model.NetworkInfo([]))}
-        instance = self._create_fake_instance_obj(params)
+        instance = self._create_fake_instance_obj()
         request_spec = objects.RequestSpec()
 
         self.stub_out('nova.virt.fake.FakeDriver.finish_migration', fake)