From 86706785ff251b841dff3590dc60f6b4834d7b7e Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Wed, 18 May 2016 21:58:06 +0200 Subject: [PATCH] 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 | 8 +++ api-ref/source/servers-action-evacuate.inc | 1 + .../v2.29/server-evacuate-req.json | 7 +++ .../versions/v21-version-get-resp.json | 2 +- .../versions/versions-get-resp.json | 2 +- nova/api/openstack/api_version_request.py | 4 +- nova/api/openstack/compute/evacuate.py | 12 ++++- .../api/openstack/compute/schemas/evacuate.py | 4 ++ .../openstack/rest_api_version_history.rst | 9 ++++ nova/compute/api.py | 27 +++++++++- .../server-evacuate-find-host-req.json.tpl | 5 ++ .../v2.29/server-evacuate-req.json.tpl | 7 +++ .../api_sample_tests/test_evacuate.py | 48 ++++++++++++++++++ .../api/openstack/compute/test_evacuate.py | 36 +++++++++++++ nova/tests/unit/compute/test_compute.py | 50 ++++++++++++++++--- nova/tests/unit/compute/test_compute_cells.py | 7 +-- ...tion_when_evacuating-37b52ebe8b5b086c.yaml | 11 ++++ 17 files changed, 225 insertions(+), 15 deletions(-) create mode 100644 doc/api_samples/os-evacuate/v2.29/server-evacuate-req.json create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.29/server-evacuate-find-host-req.json.tpl create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.29/server-evacuate-req.json.tpl create mode 100644 releasenotes/notes/check_destination_when_evacuating-37b52ebe8b5b086c.yaml diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index 2be7a9dcf308..6e64acae220a 100644 --- a/api-ref/source/parameters.yaml +++ b/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 diff --git a/api-ref/source/servers-action-evacuate.inc b/api-ref/source/servers-action-evacuate.inc index 5a5b5f16e8fb..47b593dc0d04 100644 --- a/api-ref/source/servers-action-evacuate.inc +++ b/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 | diff --git a/doc/api_samples/os-evacuate/v2.29/server-evacuate-req.json b/doc/api_samples/os-evacuate/v2.29/server-evacuate-req.json new file mode 100644 index 000000000000..c01a0a555b4c --- /dev/null +++ b/doc/api_samples/os-evacuate/v2.29/server-evacuate-req.json @@ -0,0 +1,7 @@ +{ + "evacuate": { + "host": "testHost", + "adminPass": "MySecretPass", + "force": false + } +} diff --git a/doc/api_samples/versions/v21-version-get-resp.json b/doc/api_samples/versions/v21-version-get-resp.json index fd39e1faa39b..424822696ee7 100644 --- a/doc/api_samples/versions/v21-version-get-resp.json +++ b/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" } diff --git a/doc/api_samples/versions/versions-get-resp.json b/doc/api_samples/versions/versions-get-resp.json index 432ff2f87ad0..1a3c5e29c001 100644 --- a/doc/api_samples/versions/versions-get-resp.json +++ b/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" } diff --git a/nova/api/openstack/api_version_request.py b/nova/api/openstack/api_version_request.py index 889a5e8b5c9d..987b898589a9 100644 --- a/nova/api/openstack/api_version_request.py +++ b/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 diff --git a/nova/api/openstack/compute/evacuate.py b/nova/api/openstack/compute/evacuate.py index 26cf69bb92d8..e3b8f519768b 100644 --- a/nova/api/openstack/compute/evacuate.py +++ b/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: diff --git a/nova/api/openstack/compute/schemas/evacuate.py b/nova/api/openstack/compute/schemas/evacuate.py index 51fb05410fc6..439ab2d3fd17 100644 --- a/nova/api/openstack/compute/schemas/evacuate.py +++ b/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 diff --git a/nova/api/openstack/rest_api_version_history.rst b/nova/api/openstack/rest_api_version_history.rst index df792fa26c43..77d6127442af 100644 --- a/nova/api/openstack/rest_api_version_history.rst +++ b/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. diff --git a/nova/compute/api.py b/nova/compute/api.py index 95d9d254418f..99fd30aa8e5c 100644 --- a/nova/compute/api.py +++ b/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, diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.29/server-evacuate-find-host-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.29/server-evacuate-find-host-req.json.tpl new file mode 100644 index 000000000000..8abf0b4e180f --- /dev/null +++ b/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" + } +} diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.29/server-evacuate-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.29/server-evacuate-req.json.tpl new file mode 100644 index 000000000000..bce9b83a994a --- /dev/null +++ b/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" + } +} diff --git a/nova/tests/functional/api_sample_tests/test_evacuate.py b/nova/tests/functional/api_sample_tests/test_evacuate.py index 21ba5b5ff544..de8b616b26ea 100644 --- a/nova/tests/functional/api_sample_tests/test_evacuate.py +++ b/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) diff --git a/nova/tests/unit/api/openstack/compute/test_evacuate.py b/nova/tests/unit/api/openstack/compute/test_evacuate.py index 27dc5f219996..51afbc6608b4 100644 --- a/nova/tests/unit/api/openstack/compute/test_evacuate.py +++ b/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'}) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 68fc74dc6a42..f30d6998b905 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/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 = {} diff --git a/nova/tests/unit/compute/test_compute_cells.py b/nova/tests/unit/compute/test_compute_cells.py index 1c9d3f7a111c..78e9bbacb7a3 100644 --- a/nova/tests/unit/compute/test_compute_cells.py +++ b/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() diff --git a/releasenotes/notes/check_destination_when_evacuating-37b52ebe8b5b086c.yaml b/releasenotes/notes/check_destination_when_evacuating-37b52ebe8b5b086c.yaml new file mode 100644 index 000000000000..058e25c12402 --- /dev/null +++ b/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.