api: Align availability zone info with forced host

Users can create a server like so:

  $ openstack server create --availability-zone az:host ...

This is a historical way to request that an instance be scheduled to a
specific host and it causes the scheduler to be bypassed. However, no
validation of this availability zone-host combo takes place. The host
could in fact belong to a different availability zone. If it does, we'll
end up in a very odd situation whereby the RequestSpec record for the
instance will record the availability zone requested by the user at
create time, but the Instance record itself will record the availability
zone of the host on which the instance was scheduled. This leads to even
more confusing behavior when we attempt to do something like live
migrate the instance since the RequestSpec record, with its original and
possibly invalid availability zone information, is used. The
'AvailabilityZoneFilter' will fail an error message like the following:

  Availability Zone 'foo' requested. ... has AZs: bar

but the 'openstack server list --long' command will show a non-foo value
for the availability zone column.

The solution is simple: when given an availability zone-host combo, make
sure the availability zone requested matches that of the host (or, more
specifically, the host is a member of the host aggregates that form the
availability zone [1]). If not, simply ignore the requested availability
zone information in favour of using the availability zone of the host,
logging a warning just for record keeping purposes. This is deemed
preferable to failing with HTTP 400 (Bad Request) since what users are
really requesting by using this was to schedule to a specific host: the
availability zone portion of the request is really irrelevant and just
an artifact of this legacy mechanism to request hosts. If users wish to
truly validate a host-availability zone combo, they can use the 'host'
field introduced in microversion 2.74 along with the 'availability_zone'
field:

  $ openstack server create --availability-zone az --host host ...

Conflicts:
  nova/compute/api.py

NOTE(stephenfin): Conflicts are trivial and due to the absence of change
I81fec10535034f3a81d46713a6eda813f90561cf ("Remove references to
'instance_type'") which we don't want to backport here.

[1] https://docs.openstack.org/nova/latest/admin/aggregates.html

Change-Id: Iac0e634e66cd4e150a50935cf635f626fc11b70e
Signed-off-by: Stephen Finucane <[email protected]>
Closes-Bug: #1934770
(cherry picked from commit 8f21ee4)

Author: stephenfin

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