API change for verifying the scheduler when evacuating

Adding a new microversion for changing the evacuate action behaviour
to call the scheduler anyway unless the admin user provides a force
flag that then keeps the previous behaviour by forcing the conductor
to call the destination without verifying it.

Implements: blueprint check-destination-on-migrations

APIImpact

Change-Id: I9ecbe3d481bf17b12072511da4bb106ff1b6404e

api-ref/source/parameters.yaml

@@ -1405,6 +1405,14 @@ force:
   in: body
   required: false
   type: boolean
+force_evacuate:
+  description: |
+    Force an evacuation by not verifying the provided destination host by the
+    scheduler.
+  in: body
+  required: false
+  type: boolean
+  min_version: 2.29
 forced_down:
   in: body
   required: true

api-ref/source/servers-action-evacuate.inc

@@ -27,6 +27,7 @@ Request
   - host: host
   - adminPass: adminPass_evacuate_rebuild_request
   - onSharedStorage: on_shared_storage
+  - force: force_evacuate
 
 |
 

doc/api_samples/os-evacuate/v2.29/server-evacuate-req.json

@@ -0,0 +1,7 @@
+{
+    "evacuate": {
+        "host": "testHost",
+        "adminPass": "MySecretPass",
+        "force": false
+    }
+}

doc/api_samples/versions/v21-version-get-resp.json

@@ -19,7 +19,7 @@
             }
         ],
         "status": "CURRENT",
-        "version": "2.28",
+        "version": "2.29",
         "min_version": "2.1",
         "updated": "2013-07-23T11:33:21Z"
     }

doc/api_samples/versions/versions-get-resp.json

@@ -22,7 +22,7 @@
                 }
             ],
             "status": "CURRENT",
-            "version": "2.28",
+            "version": "2.29",
             "min_version": "2.1",
             "updated": "2013-07-23T11:33:21Z"
         }

nova/api/openstack/api_version_request.py

@@ -75,6 +75,8 @@ REST_API_VERSION_HISTORY = """REST API Version History:
     * 2.27 - Adds support for new-style microversion headers while
              keeping support for the original style.
     * 2.28 - Changes compute_node.cpu_info from string to object
+    * 2.29 - Add a force flag in evacuate request body and change the
+             behaviour for the host flag by calling the scheduler.
 """
 
 # The minimum and maximum versions of the API supported
@@ -83,7 +85,7 @@ REST_API_VERSION_HISTORY = """REST API Version History:
 # Note(cyeoh): This only applies for the v2.1 API once microversions
 # support is fully merged. It does not affect the V2 API.
 _MIN_API_VERSION = "2.1"
-_MAX_API_VERSION = "2.28"
+_MAX_API_VERSION = "2.29"
 DEFAULT_API_VERSION = _MIN_API_VERSION
 
 

nova/api/openstack/compute/evacuate.py

@@ -75,7 +75,8 @@ class EvacuateController(wsgi.Controller):
     @extensions.expected_errors((400, 404, 409))
     @wsgi.action('evacuate')
     @validation.schema(evacuate.evacuate, "2.1", "2.12")
-    @validation.schema(evacuate.evacuate_v214, "2.14")
+    @validation.schema(evacuate.evacuate_v214, "2.14", "2.28")
+    @validation.schema(evacuate.evacuate_v2_29, "2.29")
     def _evacuate(self, req, id, body):
         """Permit admins to evacuate a server from a failed host
         to a new one.
@@ -85,9 +86,16 @@ class EvacuateController(wsgi.Controller):
 
         evacuate_body = body["evacuate"]
         host = evacuate_body.get("host")
+        force = None
 
         on_shared_storage = self._get_on_shared_storage(req, evacuate_body)
 
+        if api_version_request.is_supported(req, min_version='2.29'):
+            force = body["evacuate"].get("force", False)
+            force = strutils.bool_from_string(force, strict=True)
+            if force is True and not host:
+                message = _("Can't force to a non-provided destination")
+                raise exc.HTTPBadRequest(explanation=message)
         if api_version_request.is_supported(req, min_version='2.14'):
             password = self._get_password_v214(req, evacuate_body)
         else:
@@ -108,7 +116,7 @@ class EvacuateController(wsgi.Controller):
 
         try:
             self.compute_api.evacuate(context, instance, host,
-                                      on_shared_storage, password)
+                                      on_shared_storage, password, force)
         except exception.InstanceUnknownCell as e:
             raise exc.HTTPNotFound(explanation=e.format_message())
         except exception.InstanceInvalidState as state_error:

nova/api/openstack/compute/schemas/evacuate.py

@@ -38,3 +38,7 @@ evacuate = {
 evacuate_v214 = copy.deepcopy(evacuate)
 del evacuate_v214['properties']['evacuate']['properties']['onSharedStorage']
 del evacuate_v214['properties']['evacuate']['required']
+
+evacuate_v2_29 = copy.deepcopy(evacuate_v214)
+evacuate_v2_29['properties']['evacuate']['properties'][
+    'force'] = parameter_types.boolean

nova/api/openstack/rest_api_version_history.rst

@@ -300,3 +300,12 @@ user documentation.
   From this version of the API the hypervisor's 'cpu_info' field will be
   will returned as JSON object (not string) by sending GET request
   to the /v2.1/os-hypervisors/{hypervisor_id}.
+
+2.29
+----
+
+  Updates the POST request body for the ``evacuate`` action to include the
+  optional ``force`` boolean field defaulted to False.
+  Also changes the evacuate action behaviour when providing a ``host`` string
+  field by calling the nova scheduler to verify the provided host unless the
+  ``force`` attribute is set.

nova/compute/api.py

@@ -3445,7 +3445,7 @@ class API(base.Base):
     @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED,
                                     vm_states.ERROR])
     def evacuate(self, context, instance, host, on_shared_storage,
-                 admin_password=None):
+                 admin_password=None, force=None):
         """Running evacuate to target host.
 
         Checking vm compute host state, if the host not in expected_state,
@@ -3455,6 +3455,7 @@ class API(base.Base):
         :param host: Target host. if not set, the scheduler will pick up one
         :param on_shared_storage: True if instance files on shared storage
         :param admin_password: password to set on rebuilt instance
+        :param force: Force the evacuation to the specific host target
 
         """
         LOG.debug('vm evacuation scheduled', instance=instance)
@@ -3491,6 +3492,30 @@ class API(base.Base):
             # Some old instances can still have no RequestSpec object attached
             # to them, we need to support the old way
             request_spec = None
+
+        # NOTE(sbauza): Force is a boolean by the new related API version
+        if force is False and host:
+            nodes = objects.ComputeNodeList.get_all_by_host(context, host)
+            if not nodes:
+                raise exception.ComputeHostNotFound(host=host)
+            # NOTE(sbauza): Unset the host to make sure we call the scheduler
+            host = None
+            # FIXME(sbauza): Since only Ironic driver uses more than one
+            # compute per service but doesn't support evacuations,
+            # let's provide the first one.
+            target = nodes[0]
+            if request_spec:
+                # TODO(sbauza): Hydrate a fake spec for old instances not yet
+                # having a request spec attached to them (particularly true for
+                # cells v1). For the moment, let's keep the same behaviour for
+                # all the instances but provide the destination only if a spec
+                # is found.
+                destination = objects.Destination(
+                    host=target.host,
+                    node=target.hypervisor_hostname
+                )
+                request_spec.requested_destination = destination
+
         return self.compute_task_api.rebuild_instance(context,
                        instance=instance,
                        new_pass=admin_password,

nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.29/server-evacuate-find-host-req.json.tpl

@@ -0,0 +1,5 @@
+{
+    "evacuate": {
+        "adminPass": "%(adminPass)s"
+    }
+}

nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.29/server-evacuate-req.json.tpl

@@ -0,0 +1,7 @@
+{
+    "evacuate": {
+        "host": "%(host)s",
+        "adminPass": "%(adminPass)s",
+        "force": "%(force)s"
+    }
+}

nova/tests/functional/api_sample_tests/test_evacuate.py

@@ -16,6 +16,7 @@
 import mock
 
 import nova.conf
+from nova import objects
 from nova.tests.functional.api_sample_tests import test_servers
 
 CONF = nova.conf.CONF
@@ -144,3 +145,50 @@ class EvacuateJsonTestV214(EvacuateJsonTest):
                 orig_sys_metadata=mock.ANY, bdms=mock.ANY, recreate=mock.ANY,
                 on_shared_storage=None, preserve_ephemeral=mock.ANY,
                 host=None, request_spec=mock.ANY)
+
+
+class EvacuateJsonTestV229(EvacuateJsonTestV214):
+    microversion = '2.29'
+    scenarios = [('v2_29', {'api_major_version': 'v2.1'})]
+
+    @mock.patch('nova.conductor.manager.ComputeTaskManager.rebuild_instance')
+    @mock.patch('nova.objects.ComputeNodeList.get_all_by_host')
+    def test_server_evacuate(self, compute_node_get_all_by_host, rebuild_mock):
+        # Note (wingwj): The host can't be the same one
+        req_subs = {
+            'host': 'testHost',
+            "adminPass": "MySecretPass",
+            "force": "false",
+        }
+        fake_computes = objects.ComputeNodeList(
+            objects=[objects.ComputeNode(host='testHost',
+                                         hypervisor_hostname='host')])
+        compute_node_get_all_by_host.return_value = fake_computes
+        self._test_evacuate(req_subs, 'server-evacuate-req',
+                            server_resp=None, expected_resp_code=200)
+        rebuild_mock.assert_called_once_with(mock.ANY, instance=mock.ANY,
+                orig_image_ref=mock.ANY, image_ref=mock.ANY,
+                injected_files=mock.ANY, new_pass="MySecretPass",
+                orig_sys_metadata=mock.ANY, bdms=mock.ANY, recreate=mock.ANY,
+                on_shared_storage=None, preserve_ephemeral=mock.ANY,
+                host=None, request_spec=mock.ANY)
+
+    @mock.patch('nova.conductor.manager.ComputeTaskManager.rebuild_instance')
+    @mock.patch('nova.objects.ComputeNodeList.get_all_by_host')
+    def test_server_evacuate_with_force(self, compute_node_get_all_by_host,
+                                        rebuild_mock):
+        # Note (wingwj): The host can't be the same one
+        req_subs = {
+            'host': 'testHost',
+            "adminPass": "MySecretPass",
+            "force": "True",
+        }
+        self._test_evacuate(req_subs, 'server-evacuate-req',
+                            server_resp=None, expected_resp_code=200)
+        self.assertEqual(0, compute_node_get_all_by_host.call_count)
+        rebuild_mock.assert_called_once_with(mock.ANY, instance=mock.ANY,
+                orig_image_ref=mock.ANY, image_ref=mock.ANY,
+                injected_files=mock.ANY, new_pass="MySecretPass",
+                orig_sys_metadata=mock.ANY, bdms=mock.ANY, recreate=mock.ANY,
+                on_shared_storage=None, preserve_ephemeral=mock.ANY,
+                host='testHost', request_spec=mock.ANY)

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

@@ -351,3 +351,39 @@ class EvacuateTestV214(EvacuateTestV21):
         # underscores in hostnames. However, we should test that it
         # is supported because it sometimes occurs in real systems.
         self._get_evacuate_response({'host': 'underscore_hostname'})
+
+
+class EvacuateTestV229(EvacuateTestV214):
+    def setUp(self):
+        super(EvacuateTestV229, self).setUp()
+        self.admin_req = fakes.HTTPRequest.blank('', use_admin_context=True,
+                                                 version='2.29')
+        self.req = fakes.HTTPRequest.blank('', version='2.29')
+
+    @mock.patch.object(compute_api.API, 'evacuate')
+    def test_evacuate_instance(self, mock_evacuate):
+        self._get_evacuate_response({})
+        admin_pass = mock_evacuate.call_args_list[0][0][4]
+        on_shared_storage = mock_evacuate.call_args_list[0][0][3]
+        force = mock_evacuate.call_args_list[0][0][5]
+        self.assertEqual(CONF.password_length, len(admin_pass))
+        self.assertIsNone(on_shared_storage)
+        self.assertEqual(False, force)
+
+    def test_evacuate_with_valid_instance(self):
+        admin_pass = 'MyNewPass'
+        res = self._get_evacuate_response({'host': 'my-host',
+                                           'adminPass': admin_pass,
+                                           'force': 'false'})
+        self.assertIsNone(res)
+
+    @mock.patch.object(compute_api.API, 'evacuate')
+    def test_evacuate_instance_with_forced_host(self, mock_evacuate):
+        self._get_evacuate_response({'host': 'my-host',
+                                     'force': 'true'})
+        force = mock_evacuate.call_args_list[0][0][5]
+        self.assertEqual(True, force)
+
+    def test_forced_evacuate_with_no_host_provided(self):
+        self._check_evacuate_failure(webob.exc.HTTPBadRequest,
+                                     {'force': 'true'})

nova/tests/unit/compute/test_compute.py

@@ -10035,7 +10035,7 @@ class ComputeAPITestCase(BaseTestCase):
         instance.refresh()
         self.assertEqual(instance['task_state'], task_states.MIGRATING)
 
-    def test_evacuate(self):
+    def _test_evacuate(self, force=None):
         instance = self._create_fake_instance_obj(services=True)
         self.assertIsNone(instance.task_state)
 
@@ -10044,25 +10044,37 @@ class ComputeAPITestCase(BaseTestCase):
         fake_spec = objects.RequestSpec()
 
         def fake_rebuild_instance(*args, **kwargs):
-            instance.host = kwargs['host']
+            # NOTE(sbauza): Host can be set to None, we need to fake a correct
+            # destination if this is the case.
+            instance.host = kwargs['host'] or 'fake_dest_host'
             instance.save()
 
         @mock.patch.object(self.compute_api.compute_task_api,
                            'rebuild_instance')
+        @mock.patch.object(objects.ComputeNodeList, 'get_all_by_host')
         @mock.patch.object(objects.RequestSpec,
                            'get_by_instance_uuid')
         @mock.patch.object(self.compute_api.servicegroup_api, 'service_is_up')
-        def do_test(service_is_up, get_by_instance_uuid, rebuild_instance):
+        def do_test(service_is_up, get_by_instance_uuid, get_all_by_host,
+                    rebuild_instance):
             service_is_up.return_value = False
             get_by_instance_uuid.return_value = fake_spec
             rebuild_instance.side_effect = fake_rebuild_instance
+            get_all_by_host.return_value = objects.ComputeNodeList(
+                objects=[objects.ComputeNode(
+                    host='fake_dest_host',
+                    hypervisor_hostname='fake_dest_node')])
 
             self.compute_api.evacuate(ctxt,
                                       instance,
                                       host='fake_dest_host',
                                       on_shared_storage=True,
-                                      admin_password=None)
-
+                                      admin_password=None,
+                                      force=force)
+            if force is False:
+                host = None
+            else:
+                host = 'fake_dest_host'
             rebuild_instance.assert_called_once_with(
                 ctxt,
                 instance=instance,
@@ -10075,7 +10087,7 @@ class ComputeAPITestCase(BaseTestCase):
                 recreate=True,
                 on_shared_storage=True,
                 request_spec=fake_spec,
-                host='fake_dest_host')
+                host=host)
         do_test()
 
         instance.refresh()
@@ -10088,6 +10100,32 @@ class ComputeAPITestCase(BaseTestCase):
         self.assertEqual('accepted', migs[0].status)
         self.assertEqual('compute.instance.evacuate',
                          fake_notifier.NOTIFICATIONS[0].event_type)
+        if force is False:
+            req_dest = fake_spec.requested_destination
+            self.assertIsNotNone(req_dest)
+            self.assertIsInstance(req_dest, objects.Destination)
+            self.assertEqual('fake_dest_host', req_dest.host)
+            self.assertEqual('fake_dest_node', req_dest.node)
+
+    def test_evacuate(self):
+        self._test_evacuate()
+
+    def test_evacuate_with_not_forced_host(self):
+        self._test_evacuate(force=False)
+
+    def test_evacuate_with_forced_host(self):
+        self._test_evacuate(force=True)
+
+    @mock.patch('nova.servicegroup.api.API.service_is_up',
+                return_value=False)
+    def test_fail_evacuate_with_non_existing_destination(self, _service_is_up):
+        instance = self._create_fake_instance_obj(services=True)
+        self.assertIsNone(instance.task_state)
+
+        self.assertRaises(exception.ComputeHostNotFound,
+                self.compute_api.evacuate, self.context.elevated(), instance,
+                host='fake_dest_host', on_shared_storage=True,
+                admin_password=None, force=False)
 
     def test_fail_evacuate_from_non_existing_host(self):
         inst = {}

nova/tests/unit/compute/test_compute_cells.py

@@ -127,15 +127,16 @@ class CellsComputeAPITestCase(test_compute.ComputeAPITestCase):
     def test_instance_metadata(self):
         self.skipTest("Test is incompatible with cells.")
 
-    def test_evacuate(self):
+    def _test_evacuate(self, force=None):
         @mock.patch.object(compute_api.API, 'evacuate')
         def _test(mock_evacuate):
             instance = objects.Instance(uuid=uuids.evacuate_instance,
                                         cell_name='fake_cell_name')
             dest_host = 'fake_cell_name@fakenode2'
-            self.compute_api.evacuate(self.context, instance, host=dest_host)
+            self.compute_api.evacuate(self.context, instance, host=dest_host,
+                                      force=force)
             mock_evacuate.assert_called_once_with(
-                self.context, instance, 'fakenode2')
+                self.context, instance, 'fakenode2', force=force)
 
         _test()
 

releasenotes/notes/check_destination_when_evacuating-37b52ebe8b5b086c.yaml

@@ -0,0 +1,11 @@
+---
+features:
+  - On evacuate actions, the default behaviour when providing a host in
+    the request body changed. Now, instead of bypassing the scheduler when
+    asking for a destination, it will instead call it with the requested
+    destination to make sure the proposed host is accepted by all the filters
+    and the original request.
+    In case the administrator doesn't want to call the scheduler when providing
+    a destination, a new request body field called ``force`` (defaulted to
+    False) will modify that new behaviour by forcing the evacuate operation
+    to the destination without verifying the scheduler.