Merge "api: Align availability zone info with forced host" into stable/wallaby

Author: Zuul

nova/api/openstack/compute/servers.py

@@ -545,6 +545,84 @@ def _process_networks_for_create(
 
         create_kwargs['requested_networks'] = requested_networks
 
+    @staticmethod
+    def _validate_host_availability_zone(context, availability_zone, host):
+        """Ensure the host belongs in the availability zone.
+
+        This is slightly tricky and it's probably worth recapping how host
+        aggregates and availability zones are related before reading. Hosts can
+        belong to zero or more host aggregates, but they will always belong to
+        exactly one availability zone. If the user has set the availability
+        zone key on one of the host aggregates that the host is a member of
+        then the host will belong to this availability zone. If the user has
+        not set the availability zone key on any of the host aggregates that
+        the host is a member of or the host is not a member of any host
+        aggregates, then the host will belong to the default availability zone.
+        Setting the availability zone key on more than one of host aggregates
+        that the host is a member of is an error and will be rejected by the
+        API.
+
+        Given the above, our host-availability zone check needs to vary
+        behavior based on whether we're requesting the default availability
+        zone or not. If we are not, then we simply ask "does this host belong
+        to a host aggregate and, if so, do any of the host aggregates have the
+        requested availability zone metadata set". By comparison, if we *are*
+        requesting the default availability zone then we want to ask the
+        inverse, or "does this host not belong to a host aggregate or, if it
+        does, is the availability zone information unset (or, naughty naughty,
+        set to the default) for each of the host aggregates". If both cases, if
+        the answer is no then we warn about the mismatch and then use the
+        actual availability zone of the host to avoid mismatches.
+
+        :param context: The nova auth request context
+        :param availability_zone: The name of the requested availability zone
+        :param host: The name of the requested host
+        :returns: The availability zone that should actually be used for the
+            request
+        """
+        aggregates = objects.AggregateList.get_by_host(context, host=host)
+        if not aggregates:
+            # a host is assigned to the default availability zone if it is not
+            # a member of any host aggregates
+            if availability_zone == CONF.default_availability_zone:
+                return availability_zone
+
+            LOG.warning(
+                "Requested availability zone '%s' but forced host '%s' "
+                "does not belong to any availability zones; ignoring "
+                "requested availability zone to avoid bug #1934770",
+                availability_zone, host,
+            )
+            return None
+
+        # only one host aggregate will have the availability_zone field set so
+        # use the first non-null value
+        host_availability_zone = next(
+            (a.availability_zone for a in aggregates if a.availability_zone),
+            None,
+        )
+
+        if availability_zone == host_availability_zone:
+            # if there's an exact match, use what the user requested
+            return availability_zone
+
+        if (
+            availability_zone == CONF.default_availability_zone and
+            host_availability_zone is None
+        ):
+            # special case the default availability zone since this won't (or
+            # rather shouldn't) be explicitly stored on any host aggregate
+            return availability_zone
+
+        # no match, so use the host's availability zone information, if any
+        LOG.warning(
+            "Requested availability zone '%s' but forced host '%s' "
+            "does not belong to this availability zone; overwriting "
+            "requested availability zone to avoid bug #1934770",
+            availability_zone, host,
+        )
+        return None
+
     @staticmethod
     def _process_hosts_for_create(
             context, target, server_dict, create_kwargs, host, node):
@@ -665,6 +743,8 @@ def create(self, req, body):
         if host or node:
             context.can(server_policies.SERVERS % 'create:forced_host',
                         target=target)
+            availability_zone = self._validate_host_availability_zone(
+                context, availability_zone, host)
 
         if api_version_request.is_supported(req, min_version='2.74'):
             self._process_hosts_for_create(context, target, server_dict,

nova/compute/api.py

@@ -2036,17 +2036,15 @@ def create(self, context, instance_type,
             self._check_multiple_instances_with_neutron_ports(
                 requested_networks)
 
-        if availability_zone:
-            available_zones = availability_zones.\
-                get_availability_zones(context.elevated(), self.host_api,
-                                       get_only_available=True)
-            if forced_host is None and availability_zone not in \
-                    available_zones:
+        if availability_zone and forced_host is None:
+            azs = availability_zones.get_availability_zones(
+                context.elevated(), self.host_api, get_only_available=True)
+            if availability_zone not in azs:
                 msg = _('The requested availability zone is not available')
                 raise exception.InvalidRequest(msg)
 
         filter_properties = scheduler_utils.build_filter_properties(
-                scheduler_hints, forced_host, forced_node, instance_type)
+            scheduler_hints, forced_host, forced_node, instance_type)
 
         return self._create_instance(
             context, instance_type,

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

@@ -4563,21 +4563,144 @@ def test_create_instance_name_all_blank_spaces(self):
         self.assertRaises(exception.ValidationError,
                           self.controller.create, req, body=body)
 
-    def test_create_az_with_leading_trailing_spaces(self):
+    def test_create_instance_az_with_leading_trailing_spaces(self):
         self.body['server']['availability_zone'] = '  zone1  '
         self.req.body = jsonutils.dump_as_bytes(self.body)
         self.assertRaises(exception.ValidationError,
                           self.controller.create, self.req, body=self.body)
 
-    def test_create_az_with_leading_trailing_spaces_in_compat_mode(
-            self):
+    def test_create_instance_az_with_leading_trailing_spaces_compat_mode(self):
         self.body['server']['name'] = '   abc    def   '
-        self.body['server']['availability_zones'] = '  zone1  '
+        self.body['server']['availability_zone'] = '  zone1  '
         self.req.body = jsonutils.dump_as_bytes(self.body)
         self.req.set_legacy_v2()
-        with mock.patch.object(availability_zones, 'get_availability_zones',
-                               return_value=['  zone1  ']):
+        with mock.patch.object(
+            availability_zones, 'get_availability_zones',
+            return_value=['  zone1  '],
+        ) as mock_get_azs:
             self.controller.create(self.req, body=self.body)
+            mock_get_azs.assert_called_once()
+
+    def test_create_instance_invalid_az(self):
+        self.body['server']['availability_zone'] = 'zone1'
+        self.req.body = jsonutils.dump_as_bytes(self.body)
+        with mock.patch.object(
+            availability_zones, 'get_availability_zones',
+            return_value=['zone2'],
+        ) as mock_get_azs:
+            self.assertRaises(
+                webob.exc.HTTPBadRequest,
+                self.controller.create,
+                self.req, body=self.body)
+            mock_get_azs.assert_called_once()
+
+    @mock.patch.object(objects.AggregateList, 'get_by_host')
+    @mock.patch.object(servers, 'LOG')
+    def test_create_instance_az_host(self, mock_log, mock_get_host_aggs):
+        """Ensure we handle az:host format for 'availability_zone'."""
+        mock_get_host_aggs.return_value = objects.AggregateList(
+            objects=[
+                objects.Aggregate(metadata={'availability_zone': 'zone1'}),
+            ],
+        )
+
+        self.body['server']['availability_zone'] = 'zone1:host1'
+        self.req.body = jsonutils.dump_as_bytes(self.body)
+
+        self.controller.create(self.req, body=self.body)
+
+        mock_get_host_aggs.assert_called_once()
+        mock_log.warning.assert_not_called()
+
+    @mock.patch.object(objects.AggregateList, 'get_by_host')
+    @mock.patch.object(servers, 'LOG')
+    def test_create_instance_az_host_mismatch_without_aggs(
+        self, mock_log, mock_get_host_aggs,
+    ):
+        """User requests an AZ but the host doesn't have one"""
+        mock_get_host_aggs.return_value = objects.AggregateList(objects=[])
+
+        self.body['server']['availability_zone'] = 'zone1:host1'
+        self.req.body = jsonutils.dump_as_bytes(self.body)
+
+        self.controller.create(self.req, body=self.body)
+
+        mock_get_host_aggs.assert_called_once()
+        # we should see a log since the host doesn't belong to the requested AZ
+        self.assertIn('bug #1934770', mock_log.warning.call_args[0][0])
+
+    @mock.patch.object(objects.AggregateList, 'get_by_host')
+    @mock.patch.object(servers, 'LOG')
+    def test_create_instance_az_host_mismatch_without_aggs_in_default_az(
+        self, mock_log, mock_get_host_aggs,
+    ):
+        """User requests the default AZ and host isn't in any explicit AZ"""
+        self.flags(default_availability_zone='zone1')
+        mock_get_host_aggs.return_value = objects.AggregateList(objects=[])
+
+        self.body['server']['availability_zone'] = 'zone1:host1'
+        self.req.body = jsonutils.dump_as_bytes(self.body)
+
+        self.controller.create(self.req, body=self.body)
+
+        mock_get_host_aggs.assert_called_once()
+        # we shouldn't see a log since the host is not in any aggregate and
+        # therefore is in the default AZ
+        mock_log.warning.assert_not_called()
+
+    @mock.patch.object(objects.AggregateList, 'get_by_host')
+    @mock.patch.object(servers, 'LOG')
+    def test_create_instance_az_host_mismatch_with_aggs(
+        self, mock_log, mock_get_host_aggs,
+    ):
+        """User requests an AZ but the host has a different one."""
+        mock_get_host_aggs.return_value = objects.AggregateList(
+            objects=[
+                objects.Aggregate(metadata={'availability_zone': 'zone2'}),
+            ],
+        )
+
+        self.body['server']['availability_zone'] = 'zone1:host1'
+        self.req.body = jsonutils.dump_as_bytes(self.body)
+
+        self.controller.create(self.req, body=self.body)
+
+        mock_get_host_aggs.assert_called_once()
+        # we should see a log since the host belongs to a different AZ
+        self.assertIn('bug #1934770', mock_log.warning.call_args[0][0])
+
+    @mock.patch.object(objects.AggregateList, 'get_by_host')
+    @mock.patch.object(servers, 'LOG')
+    def test_create_instance_az_host_mismatch_with_aggs_in_default_az(
+        self, mock_log, mock_get_host_aggs,
+    ):
+        """User requests the default AZ and host is in aggregates without AZ"""
+        self.flags(default_availability_zone='zone1')
+        mock_get_host_aggs.return_value = objects.AggregateList(
+            objects=[objects.Aggregate(metadata={})],
+        )
+
+        self.body['server']['availability_zone'] = 'zone1:host1'
+        self.req.body = jsonutils.dump_as_bytes(self.body)
+
+        self.controller.create(self.req, body=self.body)
+
+        mock_get_host_aggs.assert_called_once()
+        # we shouldn't see a log since none of the host aggregates have an
+        # explicit AZ set and the host is therefore in the default AZ
+        mock_log.warning.assert_not_called()
+
+    def test_create_instance_invalid_az_format(self):
+        self.body['server']['availability_zone'] = 'invalid::::zone'
+        self.assertRaises(webob.exc.HTTPBadRequest,
+                          self.controller.create,
+                          self.req, body=self.body)
+
+    def test_create_instance_invalid_az_as_int(self):
+        self.body['server']['availability_zone'] = 123
+        self.assertRaises(exception.ValidationError,
+                          self.controller.create,
+                          self.req, body=self.body)
 
     def test_create_instance(self):
         self.stub_out('uuid.uuid4', lambda: FAKE_UUID)
@@ -6181,18 +6304,6 @@ def test_create_instance_raise_image_bad_request(self, mock_create):
                           self.controller.create,
                           self.req, body=self.body)
 
-    def test_create_instance_invalid_availability_zone(self):
-        self.body['server']['availability_zone'] = 'invalid::::zone'
-        self.assertRaises(webob.exc.HTTPBadRequest,
-                          self.controller.create,
-                          self.req, body=self.body)
-
-    def test_create_instance_invalid_availability_zone_as_int(self):
-        self.body['server']['availability_zone'] = 123
-        self.assertRaises(exception.ValidationError,
-                          self.controller.create,
-                          self.req, body=self.body)
-
     @mock.patch.object(compute_api.API, 'create',
                        side_effect=exception.FixedIpNotFoundForAddress(
                         address='dummy'))

nova/tests/unit/policies/test_servers.py

@@ -420,6 +420,9 @@ def test_create_server_policy(self, mock_create):
 
     @mock.patch('nova.compute.api.API.create')
     @mock.patch('nova.compute.api.API.parse_availability_zone')
+    @mock.patch.object(
+        servers.ServersController, '_validate_host_availability_zone',
+        new=mock.Mock(return_value=None))
     def test_create_forced_host_server_policy(self, mock_az, mock_create):
         # 'create' policy is checked before 'create:forced_host' so
         # we have to allow it for everyone otherwise it will