Merge pull request #13 from stackhpc/upstream/wallaby-2023-01-25

Synchronise wallaby with upstream

Author: markgoddard

.zuul.yaml

@@ -124,6 +124,66 @@
 #       Verifiy the evacuation of instances with ceph ephemeral disks
 #       from down compute hosts.
 
+- job:
+    name: nova-ovs-hybrid-plug
+    parent: tempest-multinode-full-py3
+    description: |
+      Run move operations, reboot, and evacuation (via the same post-run hook
+      as the nova-live-migration job) tests with the OVS network backend and
+      the "iptables_hybrid" securitygroup firewall driver, aka "hybrid plug".
+      The external events interactions between Nova and Neutron in these
+      situations has historically been fragile. This job exercises them.
+    irrelevant-files: *dsvm-irrelevant-files
+    vars:
+      tox_envlist: all
+      tempest_test_regex: (^tempest\..*compute\..*(migration|resize|reboot).*)
+      devstack_localrc:
+        Q_AGENT: openvswitch
+        Q_ML2_TENANT_NETWORK_TYPE: vxlan
+        Q_ML2_PLUGIN_MECHANISM_DRIVERS: openvswitch,linuxbridge
+        ML2_L3_PLUGIN: router
+      devstack_services:
+        # Disable OVN services
+        br-ex-tcpdump: false
+        br-int-flows: false
+        ovn-controller: false
+        ovn-northd: false
+        q-ovn-metadata-agent: false
+        # Neutron services
+        q-agt: true
+        q-dhcp: true
+        q-l3: true
+        q-meta: true
+      devstack_local_conf:
+        post-config:
+          "/$NEUTRON_CORE_PLUGIN_CONF":
+            securitygroup:
+              firewall_driver: iptables_hybrid
+    group-vars:
+      subnode:
+        devstack_localrc:
+          Q_AGENT: openvswitch
+          Q_ML2_TENANT_NETWORK_TYPE: vxlan
+          Q_ML2_PLUGIN_MECHANISM_DRIVERS: openvswitch,linuxbridge
+          ML2_L3_PLUGIN: router
+        devstack_services:
+          # Disable OVN services
+          br-ex-tcpdump: false
+          br-int-flows: false
+          ovn-controller: false
+          ovn-northd: false
+          ovs-vswitchd: false
+          ovsdb-server: false
+          q-ovn-metadata-agent: false
+          # Neutron services
+          q-agt: true
+        devstack_local_conf:
+          post-config:
+            "/$NEUTRON_CORE_PLUGIN_CONF":
+              securitygroup:
+                firewall_driver: iptables_hybrid
+    post-run: playbooks/nova-live-migration/post-run.yaml
+
 - job:
     name: nova-lvm
     parent: devstack-tempest
@@ -477,6 +537,7 @@
         - nova-lvm
         - nova-multi-cell
         - nova-next
+        - nova-ovs-hybrid-plug
         - nova-tox-validate-backport:
             voting: false
         - nova-tox-functional-py38

nova/compute/manager.py

@@ -4779,8 +4779,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
@@ -4872,50 +4882,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')
@@ -4973,8 +4939,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)
 
@@ -5051,8 +5027,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
 
@@ -8255,8 +8230,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:
@@ -8352,7 +8327,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
@@ -8460,8 +8435,9 @@ def _do_live_migration(self, context, dest, instance, block_migration,
         # host attachment. We fetch BDMs before that to retain connection_info
         # and attachment_id relating to the source host for post migration
         # cleanup.
-        post_live_migration = functools.partial(self._post_live_migration,
-                                                source_bdms=source_bdms)
+        post_live_migration = functools.partial(
+            self._post_live_migration_update_host, source_bdms=source_bdms
+            )
         rollback_live_migration = functools.partial(
             self._rollback_live_migration, source_bdms=source_bdms)
 
@@ -8588,15 +8564,41 @@ def live_migration_abort(self, context, instance, migration_id):
             migration, future = (
                 self._waiting_live_migrations.pop(instance.uuid))
             if future and future.cancel():
-                # If we got here, we've successfully aborted the queued
-                # migration and _do_live_migration won't run so we need
-                # to set the migration status to cancelled and send the
-                # notification. If Future.cancel() fails, it means
-                # _do_live_migration is running and the migration status
-                # is preparing, and _do_live_migration() itself will attempt
-                # to pop the queued migration, hit a KeyError, and rollback,
-                # set the migration to cancelled and send the
-                # live.migration.abort.end notification.
+                # If we got here, we've successfully dropped a queued
+                # migration from the queue, so _do_live_migration won't run
+                # and we only need to revert minor changes introduced by Nova
+                # control plane (port bindings, resource allocations and
+                # instance's PCI devices), restore VM's state, set the
+                # migration's status to cancelled and send the notification.
+                # If Future.cancel() fails, it means _do_live_migration is
+                # running and the migration status is preparing, and
+                # _do_live_migration() itself will attempt to pop the queued
+                # migration, hit a KeyError, and rollback, set the migration
+                # to cancelled and send the live.migration.abort.end
+                # notification.
+                self._revert_allocation(context, instance, migration)
+                try:
+                    # This call will delete any inactive destination host
+                    # port bindings.
+                    self.network_api.setup_networks_on_host(
+                        context, instance, host=migration.dest_compute,
+                        teardown=True)
+                except exception.PortBindingDeletionFailed as e:
+                    # Removing the inactive port bindings from the destination
+                    # host is not critical so just log an error but don't fail.
+                    LOG.error(
+                        'Network cleanup failed for destination host %s '
+                        'during live migration rollback. You may need to '
+                        'manually clean up resources in the network service. '
+                        'Error: %s', migration.dest_compute, str(e))
+                except Exception:
+                    with excutils.save_and_reraise_exception():
+                        LOG.exception(
+                            'An error occurred while cleaning up networking '
+                            'during live migration rollback.',
+                            instance=instance)
+                instance.task_state = None
+                instance.save(expected_task_state=[task_states.MIGRATING])
                 self._set_migration_status(migration, 'cancelled')
         except KeyError:
             migration = objects.Migration.get_by_id(context, migration_id)
@@ -8707,6 +8709,42 @@ def _post_live_migration_remove_source_vol_connections(
                                   bdm.attachment_id, self.host,
                                   str(e), instance=instance)
 
+    # TODO(sean-k-mooney): add typing
+    def _post_live_migration_update_host(
+        self, ctxt, instance, dest, block_migration=False,
+        migrate_data=None, source_bdms=None
+    ):
+        try:
+            self._post_live_migration(
+                ctxt, instance, dest, block_migration, migrate_data,
+                source_bdms)
+        except Exception:
+            # Restore the instance object
+            node_name = None
+            try:
+                # get node name of compute, where instance will be
+                # running after migration, that is destination host
+                compute_node = self._get_compute_info(ctxt, dest)
+                node_name = compute_node.hypervisor_hostname
+            except exception.ComputeHostNotFound:
+                LOG.exception('Failed to get compute_info for %s', dest)
+
+            # we can never rollback from post live migration and we can only
+            # get here if the instance is running on the dest so we ensure
+            # the instance.host is set correctly and reraise the original
+            # exception unmodified.
+            if instance.host != dest:
+                # apply saves the new fields while drop actually removes the
+                # migration context from the instance, so migration persists.
+                instance.apply_migration_context()
+                instance.drop_migration_context()
+                instance.host = dest
+                instance.task_state = None
+                instance.node = node_name
+                instance.progress = 0
+                instance.save()
+            raise
+
     @wrap_exception()
     @wrap_instance_fault
     def _post_live_migration(self, ctxt, instance, dest,
@@ -8718,7 +8756,7 @@ def _post_live_migration(self, ctxt, instance, dest,
         and mainly updating database record.
 
         :param ctxt: security context
-        :param instance: instance dict
+        :param instance: instance object
         :param dest: destination host
         :param block_migration: if true, prepare for block migration
         :param migrate_data: if not None, it is a dict which has data

nova/network/model.py

@@ -459,17 +459,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
@@ -538,27 +527,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

@@ -200,9 +200,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/fixtures.py

@@ -1844,13 +1844,16 @@ def delete_port_binding(self, context, client, port_id, host):
 
         return fake_requests.FakeResponse(204)
 
-    def _activate_port_binding(self, port_id, host):
+    def _activate_port_binding(self, port_id, host, modify_port=False):
         # It makes sure that only one binding is active for a port
         for h, binding in self._port_bindings[port_id].items():
             if h == host:
                 # NOTE(gibi): neutron returns 409 if this binding is already
                 # active but nova does not depend on this behaviour yet.
                 binding['status'] = 'ACTIVE'
+                if modify_port:
+                    # We need to ensure that port's binding:host_id is valid
+                    self._merge_in_active_binding(self._ports[port_id])
             else:
                 binding['status'] = 'INACTIVE'
 
@@ -1860,7 +1863,7 @@ def activate_port_binding(self, context, client, port_id, host):
         if failure is not None:
             return failure
 
-        self._activate_port_binding(port_id, host)
+        self._activate_port_binding(port_id, host, modify_port=True)
 
         return fake_requests.FakeResponse(200)