Fix failed count for anti-affinity check

The late anti-affinity check runs in the compute manager to avoid
parallel scheduling requests to invalidate the anti-affinity server
group policy. When the check fails the instance is re-scheduled.
However this failure counted as a real instance boot failure of the
compute host and can lead to de-prioritization of the compute host
in the scheduler via BuildFailureWeigher. As the late anti-affinity
check does not indicate any fault of the compute host itself it
should not be counted towards the build failure counter.
This patch adds new build results to handle this case.

Closes-Bug: #1996732
Change-Id: I2ba035c09ace20e9835d9d12a5c5bee17d616718
Signed-off-by: Yusuke Okada <[email protected]>
(cherry picked from commit 56d320a)
(cherry picked from commit 1b56714)
(cherry picked from commit 2f1d657)

Author: yusuke-okada

nova/compute/build_results.py

@@ -24,3 +24,11 @@
 ACTIVE = 'active'  # Instance is running
 FAILED = 'failed'  # Instance failed to build and was not rescheduled
 RESCHEDULED = 'rescheduled'  # Instance failed to build, but was rescheduled
+# Instance failed by policy violation (such as affinity or anti-affinity)
+# and was not rescheduled. In this case, the node's failed count won't be
+# increased.
+FAILED_BY_POLICY = 'failed_by_policy'
+# Instance failed by policy violation (such as affinity or anti-affinity)
+# but was rescheduled. In this case, the node's failed count won't be
+# increased.
+RESCHEDULED_BY_POLICY = 'rescheduled_by_policy'

nova/compute/manager.py

@@ -1804,11 +1804,8 @@ def _do_validation(context, instance, group):
                 else:
                     max_server = 1
                 if len(members_on_host) >= max_server:
-                    msg = _("Anti-affinity instance group policy "
-                            "was violated.")
-                    raise exception.RescheduledException(
-                            instance_uuid=instance.uuid,
-                            reason=msg)
+                    raise exception.GroupAffinityViolation(
+                        instance_uuid=instance.uuid, policy='Anti-affinity')
 
             # NOTE(ganso): The check for affinity below does not work and it
             # can easily be violated because the lock happens in different
@@ -1818,10 +1815,8 @@ def _do_validation(context, instance, group):
             elif group.policy and 'affinity' == group.policy:
                 group_hosts = group.get_hosts(exclude=[instance.uuid])
                 if group_hosts and self.host not in group_hosts:
-                    msg = _("Affinity instance group policy was violated.")
-                    raise exception.RescheduledException(
-                            instance_uuid=instance.uuid,
-                            reason=msg)
+                    raise exception.GroupAffinityViolation(
+                        instance_uuid=instance.uuid, policy='Affinity')
 
         _do_validation(context, instance, group)
 
@@ -2256,6 +2251,9 @@ def _locked_do_build_and_run_instance(*args, **kwargs):
                         self.reportclient.delete_allocation_for_instance(
                             context, instance.uuid, force=True)
 
+                    if result in (build_results.FAILED_BY_POLICY,
+                                  build_results.RESCHEDULED_BY_POLICY):
+                        return
                     if result in (build_results.FAILED,
                                   build_results.RESCHEDULED):
                         self._build_failed(node)
@@ -2354,6 +2352,8 @@ def _do_build_and_run_instance(self, context, instance, image,
                 self._nil_out_instance_obj_host_and_node(instance)
                 self._set_instance_obj_error_state(instance,
                                                    clean_task_state=True)
+                if isinstance(e, exception.RescheduledByPolicyException):
+                    return build_results.FAILED_BY_POLICY
                 return build_results.FAILED
             LOG.debug(e.format_message(), instance=instance)
             # This will be used for logging the exception
@@ -2380,6 +2380,10 @@ def _do_build_and_run_instance(self, context, instance, image,
                     injected_files, requested_networks, security_groups,
                     block_device_mapping, request_spec=request_spec,
                     host_lists=[host_list])
+
+            if isinstance(e, exception.RescheduledByPolicyException):
+                return build_results.RESCHEDULED_BY_POLICY
+
             return build_results.RESCHEDULED
         except (exception.InstanceNotFound,
                 exception.UnexpectedDeletingTaskStateError):
@@ -2597,6 +2601,17 @@ def _build_and_run_instance(self, context, instance, image, injected_files,
                     bdms=block_device_mapping)
             raise exception.BuildAbortException(instance_uuid=instance.uuid,
                     reason=e.format_message())
+        except exception.GroupAffinityViolation as e:
+            LOG.exception('Failed to build and run instance',
+                          instance=instance)
+            self._notify_about_instance_usage(context, instance,
+                    'create.error', fault=e)
+            compute_utils.notify_about_instance_create(
+                    context, instance, self.host,
+                    phase=fields.NotificationPhase.ERROR, exception=e,
+                    bdms=block_device_mapping)
+            raise exception.RescheduledByPolicyException(
+                    instance_uuid=instance.uuid, reason=str(e))
         except Exception as e:
             LOG.exception('Failed to build and run instance',
                           instance=instance)

nova/exception.py

@@ -1477,6 +1477,15 @@ class RescheduledException(NovaException):
                 "%(reason)s")
 
 
+class RescheduledByPolicyException(RescheduledException):
+    msg_fmt = _("Build of instance %(instance_uuid)s was re-scheduled: "
+                "%(reason)s")
+
+
+class GroupAffinityViolation(NovaException):
+    msg_fmt = _("%(policy)s instance group policy was violated")
+
+
 class InstanceFaultRollback(NovaException):
     def __init__(self, inner_exception=None):
         message = _("Instance rollback performed due to: %s")

nova/tests/functional/test_server_group.py

@@ -19,6 +19,7 @@
 from nova.compute import instance_actions
 from nova import context
 from nova.db.main import api as db
+from nova import objects
 from nova import test
 from nova.tests import fixtures as nova_fixtures
 from nova.tests.functional.api import client
@@ -494,6 +495,85 @@ def test_soft_affinity_not_supported(self):
         self.assertIn('Invalid input', ex.response.text)
         self.assertIn('soft-affinity', ex.response.text)
 
+    @mock.patch('nova.scheduler.filters.affinity_filter.'
+        'ServerGroupAffinityFilter.host_passes', return_value=True)
+    def test_failed_count_with_affinity_violation(self, mock_host_passes):
+        """Check failed count not incremented after violation of the late
+        affinity check. https://bugs.launchpad.net/nova/+bug/1996732
+        """
+
+        created_group = self.api.post_server_groups(self.affinity)
+        flavor = self.api.get_flavors()[2]
+
+        # Ensure the first instance is on compute1
+        with utils.temporary_mutation(self.admin_api, microversion='2.53'):
+            compute2_service_id = self.admin_api.get_services(
+            host=self.compute2.host, binary='nova-compute')[0]['id']
+            self.admin_api.put_service(compute2_service_id,
+                                        {'status': 'disabled'})
+
+        self._boot_a_server_to_group(created_group, flavor=flavor)
+
+        # Ensure the second instance is on compute2
+        with utils.temporary_mutation(self.admin_api, microversion='2.53'):
+            self.admin_api.put_service(compute2_service_id,
+                                        {'status': 'enabled'})
+            compute1_service_id = self.admin_api.get_services(
+            host=self.compute.host, binary='nova-compute')[0]['id']
+            self.admin_api.put_service(compute1_service_id,
+                                        {'status': 'disabled'})
+
+        # Expects GroupAffinityViolation exception
+        failed_server = self._boot_a_server_to_group(created_group,
+                                                     flavor=flavor,
+                                                     expected_status='ERROR')
+
+        self.assertEqual('Exceeded maximum number of retries. Exhausted all '
+                         'hosts available for retrying build failures for '
+                         'instance %s.' % failed_server['id'],
+                         failed_server['fault']['message'])
+
+        ctxt = context.get_admin_context()
+        computes = objects.ComputeNodeList.get_all(ctxt)
+
+        for node in computes:
+            self.assertEqual(node.stats.get('failed_builds'), '0')
+
+    @mock.patch('nova.scheduler.filters.affinity_filter.'
+        'ServerGroupAntiAffinityFilter.host_passes', return_value=True)
+    def test_failed_count_with_anti_affinity_violation(self, mock_host_passes):
+        """Check failed count after violation of the late affinity check.
+        https://bugs.launchpad.net/nova/+bug/1996732
+        """
+
+        created_group = self.api.post_server_groups(self.anti_affinity)
+        flavor = self.api.get_flavors()[2]
+
+        # Ensure two instances are scheduled on the same host
+        with utils.temporary_mutation(self.admin_api, microversion='2.53'):
+            compute2_service_id = self.admin_api.get_services(
+            host=self.compute2.host, binary='nova-compute')[0]['id']
+            self.admin_api.put_service(compute2_service_id,
+                                        {'status': 'disabled'})
+
+        self._boot_a_server_to_group(created_group, flavor=flavor)
+
+        # Expects GroupAffinityViolation exception
+        failed_server = self._boot_a_server_to_group(created_group,
+                                                     flavor=flavor,
+                                                     expected_status='ERROR')
+
+        self.assertEqual('Exceeded maximum number of retries. Exhausted all '
+                         'hosts available for retrying build failures for '
+                         'instance %s.' % failed_server['id'],
+                         failed_server['fault']['message'])
+
+        ctxt = context.get_admin_context()
+        computes = objects.ComputeNodeList.get_all(ctxt)
+
+        for node in computes:
+            self.assertEqual(node.stats.get('failed_builds'), '0')
+
 
 class ServerGroupAffinityConfTest(ServerGroupTestBase):
     api_major_version = 'v2.1'