Merge "Remove PlacementAPIConnectFailure handling from AggregateAPI"

nova/api/openstack/compute/aggregates.py

@@ -178,7 +178,8 @@ def _remove_host(self, req, id, body):
             msg = _('Cannot remove host %(host)s in aggregate %(id)s') % {
                         'host': host, 'id': id}
             raise exc.HTTPNotFound(explanation=msg)
-        except exception.InvalidAggregateAction:
+        except (exception.InvalidAggregateAction,
+                exception.ResourceProviderUpdateConflict):
             msg = _('Cannot remove host %(host)s in aggregate %(id)s') % {
                         'host': host, 'id': id}
             raise exc.HTTPConflict(explanation=msg)

nova/compute/api.py

@@ -5794,16 +5794,6 @@ def add_host_to_aggregate(self, context, aggregate_id, host_name):
         try:
             self.placement_client.aggregate_add_host(
                 context, aggregate.uuid, host_name=host_name)
-        except exception.PlacementAPIConnectFailure:
-            # NOTE(jaypipes): Rocky should be able to tolerate the nova-api
-            # service not communicating with the Placement API, so just log a
-            # warning here.
-            # TODO(jaypipes): Remove this in Stein, when placement must be able
-            # to be contacted from the nova-api service.
-            LOG.warning("Failed to associate %s with a placement "
-                        "aggregate: %s. There was a failure to communicate "
-                        "with the placement service.",
-                        host_name, aggregate.uuid)
         except (exception.ResourceProviderNotFound,
                 exception.ResourceProviderAggregateRetrievalFailed,
                 exception.ResourceProviderUpdateFailed,
@@ -5855,35 +5845,26 @@ def remove_host_from_aggregate(self, context, aggregate_id, host_name):
             action=fields_obj.NotificationAction.REMOVE_HOST,
             phase=fields_obj.NotificationPhase.START)
 
-        aggregate.delete_host(host_name)
-        self.query_client.update_aggregates(context, [aggregate])
+        # Remove the resource provider from the provider aggregate first before
+        # we change anything on the nova side because if we did the nova stuff
+        # first we can't re-attempt this from the compute API if cleaning up
+        # placement fails.
         try:
+            # Anything else this raises is handled in the route handler as
+            # either a 409 (ResourceProviderUpdateConflict) or 500.
             self.placement_client.aggregate_remove_host(
                 context, aggregate.uuid, host_name)
-        except exception.PlacementAPIConnectFailure:
-            # NOTE(jaypipes): Rocky should be able to tolerate the nova-api
-            # service not communicating with the Placement API, so just log a
-            # warning here.
-            # TODO(jaypipes): Remove this in Stein, when placement must be able
-            # to be contacted from the nova-api service.
-            LOG.warning("Failed to remove association of %s with a placement "
-                        "aggregate: %s. There was a failure to communicate "
-                        "with the placement service.",
-                        host_name, aggregate.uuid)
-        except (exception.ResourceProviderNotFound,
-                exception.ResourceProviderAggregateRetrievalFailed,
-                exception.ResourceProviderUpdateFailed,
-                exception.ResourceProviderUpdateConflict) as err:
-            # NOTE(jaypipes): We don't want a failure perform the mirroring
-            # action in the placement service to be returned to the user (they
-            # probably don't know anything about the placement service and
-            # would just be confused). So, we just log a warning here, noting
-            # that on the next run of nova-manage placement sync_aggregates
-            # things will go back to normal
+        except exception.ResourceProviderNotFound as err:
+            # If the resource provider is not found then it's likely not part
+            # of the aggregate anymore anyway since provider aggregates are
+            # not resources themselves with metadata like nova aggregates, they
+            # are just a grouping concept around resource providers. Log and
+            # continue.
             LOG.warning("Failed to remove association of %s with a placement "
-                        "aggregate: %s. This may be corrected after running "
-                        "nova-manage placement sync_aggregates.",
-                        host_name, err)
+                        "aggregate: %s.", host_name, err)
+
+        aggregate.delete_host(host_name)
+        self.query_client.update_aggregates(context, [aggregate])
         self._update_az_cache_for_host(context, host_name, aggregate.metadata)
         self.compute_rpcapi.remove_aggregate_host(context,
                 aggregate=aggregate, host_param=host_name, host=host_name)

nova/tests/unit/api/openstack/compute/test_aggregates.py

@@ -588,6 +588,18 @@ def test_remove_host_with_missing_host_empty(self):
                           eval(self.remove_host),
                 self.req, "1", body={"remove_host": {}})
 
+    def test_remove_host_resource_provider_update_conflict(self):
+        """Tests that ResourceProviderUpdateConflict is handled as a 409."""
+        side_effect = exception.ResourceProviderUpdateConflict(
+            uuid=uuidsentinel.provider_id, generation=1, error='try again!')
+        with mock.patch.object(self.controller.api,
+                               'remove_host_from_aggregate',
+                               side_effect=side_effect) as mock_rem:
+            self.assertRaises(exc.HTTPConflict, eval(self.remove_host),
+                              self.req, "1",
+                              body={"remove_host": {"host": "bogushost"}})
+            mock_rem.assert_called_once_with(self.context, "1", "bogushost")
+
     def test_set_metadata(self):
         body = {"set_metadata": {"metadata": {"foo": "bar"}}}
         with mock.patch.object(self.controller.api,

nova/tests/unit/compute/test_host_api.py

@@ -653,42 +653,36 @@ def test_aggregate_add_host_placement_missing_provider(
 
     @mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
                 'aggregate_add_host')
-    @mock.patch.object(compute.LOG, 'warning')
-    def test_aggregate_add_host_bad_placement(
-            self, mock_log, mock_pc_add_host):
+    def test_aggregate_add_host_bad_placement(self, mock_pc_add_host):
         hostname = 'fake-host'
         mock_pc_add_host.side_effect = exception.PlacementAPIConnectFailure
         aggregate = self.aggregate_api.create_aggregate(
             self.ctxt, 'aggregate', None)
         agg_uuid = aggregate.uuid
-        self.aggregate_api.add_host_to_aggregate(
-            self.ctxt, aggregate.id, hostname)
-        # Nothing should blow up in Rocky, but we should get a warning about
-        # placement connectivity failure
-        msg = ("Failed to associate %s with a placement "
-               "aggregate: %s. There was a failure to communicate "
-               "with the placement service.")
-        mock_log.assert_called_with(msg, hostname, agg_uuid)
+        self.assertRaises(exception.PlacementAPIConnectFailure,
+                          self.aggregate_api.add_host_to_aggregate,
+                          self.ctxt, aggregate.id, hostname)
+        mock_pc_add_host.assert_called_once_with(
+            self.ctxt, agg_uuid, host_name=hostname)
 
     @mock.patch('nova.objects.Aggregate.delete_host')
     @mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
                 'aggregate_remove_host')
-    @mock.patch.object(compute.LOG, 'warning')
     def test_aggregate_remove_host_bad_placement(
-            self, mock_log, mock_pc_remove_host, mock_agg_obj_delete_host):
+            self, mock_pc_remove_host, mock_agg_obj_delete_host):
         hostname = 'fake-host'
         mock_pc_remove_host.side_effect = exception.PlacementAPIConnectFailure
         aggregate = self.aggregate_api.create_aggregate(
             self.ctxt, 'aggregate', None)
         agg_uuid = aggregate.uuid
-        self.aggregate_api.remove_host_from_aggregate(
-            self.ctxt, aggregate.id, hostname)
-        # Nothing should blow up in Rocky, but we should get a warning about
-        # placement connectivity failure
-        msg = ("Failed to remove association of %s with a placement "
-               "aggregate: %s. There was a failure to communicate "
-               "with the placement service.")
-        mock_log.assert_called_with(msg, hostname, agg_uuid)
+        self.assertRaises(exception.PlacementAPIConnectFailure,
+                          self.aggregate_api.remove_host_from_aggregate,
+                          self.ctxt, aggregate.id, hostname)
+        mock_pc_remove_host.assert_called_once_with(
+            self.ctxt, agg_uuid, hostname)
+        # Make sure mock_agg_obj_delete_host wasn't called since placement
+        # should be tried first and failed with a server failure.
+        mock_agg_obj_delete_host.assert_not_called()
 
     @mock.patch('nova.objects.Aggregate.delete_host')
     @mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
@@ -705,6 +699,8 @@ def test_aggregate_remove_host_placement_missing_provider(
             self.ctxt, aggregate.id, hostname)
         # Nothing should blow up in Rocky, but we should get a warning
         msg = ("Failed to remove association of %s with a placement "
-               "aggregate: %s. This may be corrected after running "
-               "nova-manage placement sync_aggregates.")
+               "aggregate: %s.")
         mock_log.assert_called_with(msg, hostname, err)
+        # In this case Aggregate.delete_host is still called because the
+        # ResourceProviderNotFound error is just logged.
+        mock_agg_obj_delete_host.assert_called_once_with(hostname)