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

This reverts commit 19f9b37.

Commit 19f9b37 introduced
a regression and caused errors on Neutron CI jobs which run
on a single node with iptables_hybrid firewall driver.
This is because the nova change made the resize revert flow
wait for neutron-vif-plugged events in the ComputeManager
when the port's host binding changes for iptables_hybrid
ports but for those types of ports, Neutron does not send
the event if the host does not change - which is the case
for a same-host resize being reverted.

Change-Id: I77b3639435c671bacca9cdfc1aa203e44a2fb042
Closes-Bug: #1833902

Author: slawqo

nova/compute/manager.py

@@ -4164,50 +4164,6 @@ def revert_resize(self, context, instance, migration):
             self.compute_rpcapi.finish_revert_resize(context, instance,
                     migration, migration.source_compute)
 
-    def _finish_revert_resize_network_migrate_finish(self, context, instance,
-                                                     migration):
-        """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.
-        :raises: eventlet.timeout.Timeout or
-                 exception.VirtualInterfacePlugException.
-        """
-        network_info = instance.get_network_info()
-        events = []
-        deadline = CONF.vif_plugging_timeout
-        if deadline and utils.is_neutron() and network_info:
-            events = network_info.get_bind_time_events()
-            if events:
-                LOG.debug('Will wait for bind-time events: %s', events,
-                          instance=instance)
-        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)
-        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')
@@ -4255,8 +4211,17 @@ def finish_revert_resize(self, context, instance, migration):
 
             self.network_api.setup_networks_on_host(context, instance,
                                                     migration.source_compute)
-            self._finish_revert_resize_network_migrate_finish(
-                context, instance, migration)
+            # 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)
             network_info = self.network_api.get_instance_nw_info(context,
                                                                  instance)
 
@@ -6474,8 +6439,8 @@ def pre_live_migration(self, context, instance, block_migration, disk,
         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:
@@ -6553,7 +6518,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

@@ -458,16 +458,6 @@ def labeled_ips(self):
                     'ips': ips}
         return []
 
-    def has_bind_time_event(self):
-        """When updating the port binding to a host that already has the
-        instance in a shutoff state (in practice, this currently means
-        reverting a resize or cold migration), the following Neutron/port
-        configurations cause network-vif-plugged events to be sent as soon as
-        the binding is updated:
-        - OVS with hybrid plug
-        """
-        return self.is_hybrid_plug_enabled()
-
     def is_hybrid_plug_enabled(self):
         return self['details'].get(VIF_DETAILS_OVS_HYBRID_PLUG, False)
 
@@ -525,24 +515,6 @@ def wait(self, do_raise=True):
     def json(self):
         return jsonutils.dumps(self)
 
-    def get_bind_time_events(self):
-        """When updating the port binding to a host that already has the
-        instance in a shutoff state (in practice, this currently means
-        reverting a resize or cold migration), return external events that are
-        sent as soon as the binding is updated.
-        """
-        return [('network-vif-plugged', vif['id'])
-                for vif in self if vif.has_bind_time_event()]
-
-    def get_plug_time_events(self):
-        """When updating the port binding to a host that already has the
-        instance in a shutoff state (in practice, this currently means
-        reverting a resize or cold migration), return external events that are
-        sent when the VIF is plugged.
-        """
-        return [('network-vif-plugged', vif['id'])
-                for vif in self if not vif.has_bind_time_event()]
-
 
 class NetworkInfoAsyncWrapper(NetworkInfo):
     """Wrapper around NetworkInfo that allows retrieving NetworkInfo

nova/tests/unit/compute/test_compute.py

@@ -5891,9 +5891,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)
 
         self.stub_out('nova.virt.fake.FakeDriver.finish_migration', fake)
@@ -6043,9 +6041,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()
 
         self.stub_out('nova.virt.fake.FakeDriver.finish_migration', fake)
         self.stub_out('nova.virt.fake.FakeDriver.finish_revert_migration',

nova/tests/unit/compute/test_compute_mgr.py

@@ -5054,81 +5054,6 @@ def test_notify_volume_usage_detach_no_block_stats(self):
                     self.context, fake_instance, fake_bdm)
         block_stats.assert_called_once_with(fake_instance, 'vda')
 
-    def _test_finish_revert_resize_network_migrate_finish(self, vifs, events):
-        instance = fake_instance.fake_instance_obj(self.context)
-        instance.info_cache = objects.InstanceInfoCache(
-            network_info=network_model.NetworkInfo(vifs))
-        migration = objects.Migration(
-            source_compute='fake-source',
-            dest_compute='fake-dest')
-
-        def fake_migrate_instance_finish(context, instance, migration):
-            self.assertEqual(migration.source_compute, 'fake-source')
-            # NOTE(artom) This looks weird, but it's checking that the
-            # temporaty_mutation() context manager did its job.
-            self.assertEqual(migration.dest_compute, 'fake-source')
-
-        with test.nested(
-            mock.patch.object(self.compute.virtapi,
-                              'wait_for_instance_event'),
-            mock.patch.object(self.compute.network_api,
-                              'migrate_instance_finish',
-                              side_effect=fake_migrate_instance_finish)
-        ) as (mock_wait, mock_migrate_instance_finish):
-            self.compute._finish_revert_resize_network_migrate_finish(
-                self.context, instance, migration)
-            mock_wait.assert_called_once_with(
-                instance, events, deadline=CONF.vif_plugging_timeout,
-                error_callback=self.compute._neutron_failed_migration_callback)
-            mock_migrate_instance_finish.assert_called_once_with(
-                self.context, instance, migration)
-
-    def test_finish_revert_resize_network_migrate_finish_wait(self):
-        """Test that we wait for bind-time events if we have a hybrid-plugged
-        VIF.
-        """
-        self._test_finish_revert_resize_network_migrate_finish(
-            [network_model.VIF(id=uuids.hybrid_vif,
-                               details={'ovs_hybrid_plug': True}),
-             network_model.VIF(id=uuids.normal_vif,
-                               details={'ovs_hybrid_plug': False})],
-            [('network-vif-plugged', uuids.hybrid_vif)])
-
-    def test_finish_revert_resize_network_migrate_finish_dont_wait(self):
-        """Test that we're not waiting for any events if we don't have any
-        hybrid-plugged VIFs.
-        """
-        self._test_finish_revert_resize_network_migrate_finish(
-            [network_model.VIF(id=uuids.hybrid_vif,
-                               details={'ovs_hybrid_plug': False}),
-             network_model.VIF(id=uuids.normal_vif,
-                               details={'ovs_hybrid_plug': False})],
-            [])
-
-    def test_finish_revert_resize_network_migrate_finish_no_vif_timeout(self):
-        """Test that we're not waiting for any events if vif_plugging_timeout
-        is 0.
-        """
-        self.flags(vif_plugging_timeout=0)
-        self._test_finish_revert_resize_network_migrate_finish(
-            [network_model.VIF(id=uuids.hybrid_vif,
-                               details={'ovs_hybrid_plug': True}),
-             network_model.VIF(id=uuids.normal_vif,
-                               details={'ovs_hybrid_plug': True})],
-            [])
-
-    @mock.patch.object(utils, 'is_neutron', return_value=False)
-    def test_finish_revert_resize_network_migrate_finish_not_neutron(self, _):
-        """Test that we're not waiting for any events if we're not using
-        Neutron.
-        """
-        self._test_finish_revert_resize_network_migrate_finish(
-            [network_model.VIF(id=uuids.hybrid_vif,
-                               details={'ovs_hybrid_plug': True}),
-             network_model.VIF(id=uuids.normal_vif,
-                               details={'ovs_hybrid_plug': True})],
-            [])
-
 
 class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
     def setUp(self):

nova/tests/unit/network/test_network_info.py

@@ -15,7 +15,6 @@
 #    under the License.
 
 from oslo_config import cfg
-from oslo_utils.fixture import uuidsentinel as uuids
 
 from nova import exception
 from nova.network import model
@@ -858,21 +857,6 @@ def test_injection_ipv6_with_lxc_no_gateway(self):
                 libvirt_virt_type='lxc')
         self.assertEqual(expected, template)
 
-    def test_get_events(self):
-        network_info = model.NetworkInfo([
-            model.VIF(
-                id=uuids.hybrid_vif,
-                details={'ovs_hybrid_plug': True}),
-            model.VIF(
-                id=uuids.normal_vif,
-                details={'ovs_hybrid_plug': False})])
-        self.assertEqual(
-            [('network-vif-plugged', uuids.hybrid_vif)],
-            network_info.get_bind_time_events())
-        self.assertEqual(
-            [('network-vif-plugged', uuids.normal_vif)],
-            network_info.get_plug_time_events())
-
 
 class TestNetworkMetadata(test.NoDBTestCase):
     def setUp(self):