functional: Add reproducer for #1907775

You can currently remove a host that has instances scheduled to it from
an aggregate. If the aggregate is configured as part of an availability
zone (AZ), this would in turn remove the host from the AZ, leaving
instances originally scheduled to that AZ stranded on a host that is no
longer a member of the AZ. This is clearly undesirable and should be
blocked at the API level.

You can also add a host to an aggregate where it wasn't in one before.
Because nova provides a default AZ for hosts that don't belong to an
aggregate, adding a host to an aggregate doesn't just assign it to an
AZ, it removes it from the default 'nova' one (or whatever you've
configured via '[DEFAULT] default_availability_zone'). As noted in the
docs [1], people should not rely on scheduling to the default AZ, but if
they had, we'd end up in the same situation as above.

Add tests for both, with a fix coming after.

[1] https://docs.openstack.org/nova/latest/admin/availability-zones.html

Change-Id: I21f7f93ee0ec0cd3a290afba59342b31d074cf2f
Signed-off-by: Stephen Finucane <[email protected]>
Related-Bug: #1907775

Author: stephenfin

nova/tests/functional/test_aggregates.py

@@ -197,14 +197,24 @@ def _add_host_to_aggregate(self, agg, host):
         agg = self.aggregates[agg]
         self.admin_api.add_host_to_aggregate(agg['id'], host)
 
-    def _boot_server(self, az=None, flavor_id=None, image_id=None,
-                     end_status='ACTIVE'):
+    def _remove_host_from_aggregate(self, agg, host):
+        """Remove a compute host from both nova and placement aggregates.
+
+        :param agg: Name of the nova aggregate
+        :param host: Name of the compute host
+        """
+        agg = self.aggregates[agg]
+        self.admin_api.remove_host_from_aggregate(agg['id'], host)
+
+    def _boot_server(
+        self, az=None, host=None, flavor_id=None, image_id=None,
+        end_status='ACTIVE',
+    ):
         flavor_id = flavor_id or self.flavors[0]['id']
         image_uuid = image_id or '155d900f-4e14-4e4c-a73d-069cbf4541e6'
         server_req = self._build_server(
-            image_uuid=image_uuid,
-            flavor_id=flavor_id,
-            networks='none', az=az)
+            image_uuid=image_uuid, flavor_id=flavor_id,
+            networks='none', az=az, host=host)
 
         created_server = self.api.post_server({'server': server_req})
         server = self._wait_for_state_change(created_server, end_status)
@@ -284,7 +294,7 @@ def _set_traits_on_aggregate(self, agg, traits):
 
 class AggregatePostTest(AggregateRequestFiltersTest):
 
-    def test_set_az_for_aggreate_no_instances(self):
+    def test_set_az_for_aggregate_no_instances(self):
         """Should be possible to update AZ for an empty aggregate.
 
         Check you can change the AZ name of an aggregate when it does
@@ -343,6 +353,105 @@ def test_cannot_delete_az(self):
         self.assertEqual(az, agg['availability_zone'])
 
 
+class AggregateHostMoveTestCase(AggregateRequestFiltersTest):
+
+    def setUp(self):
+        super().setUp()
+        # keep it separate from the parent class' setup
+        self.host = 'host3'
+        az = 'custom-az'
+        self._start_compute(self.host)
+        self._create_aggregate('ag1-no-az')
+        self._create_aggregate('ag2-no-az')
+        self._create_aggregate('ag3-custom-az')
+        self._set_az_aggregate('ag3-custom-az', az)
+        self._create_aggregate('ag4-custom-az')
+        self._set_az_aggregate('ag4-custom-az', az)
+
+    def test_add_host_with_instances_default_az_doesnt_change(self):
+        # server will be in default AZ
+        self._boot_server(host=self.host)
+
+        # the AZ of the server does not change as the aggregate is also in
+        # the default AZ.
+        self._add_host_to_aggregate('ag1-no-az', self.host)
+
+    def test_add_host_with_instances_custom_az_doesnt_change(self):
+        self._add_host_to_aggregate('ag3-custom-az', self.host)
+        # server will be in custom AZ
+        self._boot_server(host=self.host)
+
+        # as the new aggregate also in the custom-az this does not need the
+        # server to move between AZs, so this is OK.
+        self._add_host_to_aggregate('ag4-custom-az', self.host)
+
+    def test_cannot_add_host_with_instances_default_az_then_custom_az(self):
+        # server will be in default AZ
+        self._boot_server(host=self.host)
+
+        # FIXME(stephenfin): This is bug #1907775, where we should reject the
+        # request to add a host to an aggregate when and instance would need
+        # to move between AZs
+
+        # The server would need to move from default AZ to custom AZ, that
+        # is not OK
+        self._add_host_to_aggregate('ag3-custom-az', self.host)
+
+    def test_add_host_with_instances_custom_az_then_default(self):
+        self._add_host_to_aggregate('ag3-custom-az', self.host)
+        # server will be in custom AZ
+        self._boot_server(host=self.host)
+
+        # The server is still in the custom AZ and that is OK as the host is
+        # also in that AZ even after added to an aggregate without AZ.
+        self._add_host_to_aggregate('ag1-no-az', self.host)
+
+    def test_remove_host_with_instances_default_az_doesnt_change(self):
+        self._add_host_to_aggregate('ag1-no-az', self.host)
+        self._add_host_to_aggregate('ag2-no-az', self.host)
+        # server will be in default AZ
+        self._boot_server(host=self.host)
+
+        # The server still remains in default AZ so no AZ change, this is OK
+        self._remove_host_from_aggregate('ag1-no-az', self.host)
+        # still OK as the host not in any aggregate means instance is in
+        # default AZ.
+        self._remove_host_from_aggregate('ag2-no-az', self.host)
+
+    def test_remove_host_with_instances_custom_az_doesnt_change(self):
+        self._add_host_to_aggregate('ag3-custom-az', self.host)
+        self._add_host_to_aggregate('ag4-custom-az', self.host)
+        # server will be in custom AZ
+        self._boot_server(host=self.host)
+
+        # The server still remains in custom AZ so no AZ change, it is OK.
+        self._remove_host_from_aggregate('ag3-custom-az', self.host)
+
+    def test_cannot_remove_host_with_instances_custom_az_to_default(self):
+        self._add_host_to_aggregate('ag3-custom-az', self.host)
+        # server will be in custom AZ
+        self._boot_server(host=self.host)
+
+        # FIXME(stephenfin): This is bug #1907775, where we should reject the
+        # request to remove a host from the aggregate when there are instances
+        # on said host
+
+        # The server would need to move to the default AZ, that is not OK.
+        self._remove_host_from_aggregate('ag3-custom-az', self.host)
+
+    def test_moving_host_around_az_without_instances(self):
+        # host moving from default to custom AZ
+        self._add_host_to_aggregate('ag3-custom-az', self.host)
+        # host still in custom AZ
+        self._add_host_to_aggregate('ag1-no-az', self.host)
+        # host moves from custom to default AZ
+        self._remove_host_from_aggregate('ag3-custom-az', self.host)
+        # host still in default AZ
+        self._remove_host_from_aggregate('ag1-no-az', self.host)
+        # host still in default AZ
+        self._add_host_to_aggregate('ag1-no-az', self.host)
+
+
 # NOTE: this test case has the same test methods as AggregatePostTest
 # but for the AZ update it uses PUT /os-aggregates/{aggregate_id} method
 class AggregatePutTest(AggregatePostTest):