From c01d16e81af6cd9453ffe7133bdc6a4c82e4f6d5 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Tue, 10 Nov 2015 09:17:06 +0100 Subject: [PATCH] Remove onSharedStorage from evacuate API The patch I4217bd00d8c253db522241885dba2847a26af6df made the on_shared_storge flag optional in the compute api in Liberty. This patch removes the corresponding onSharedStorage flag from the REST API as nova can easily detect this information. APIImpact DocImpact Implements: bp remove-shared-storage-flag-in-evacuate-api Change-Id: I54bfa1275e188573c1b95d770d89160a86cdf52c --- .../v2.14/server-evacuate-find-host-req.json | 5 + .../v2.14/server-evacuate-req.json | 6 ++ .../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 | 67 +++++++++---- .../api/openstack/compute/schemas/evacuate.py | 6 ++ .../openstack/rest_api_version_history.rst | 7 ++ .../server-evacuate-find-host-req.json.tpl | 5 + .../v2.14/server-evacuate-req.json.tpl | 6 ++ .../versions/v21-version-get-resp.json.tpl | 2 +- .../versions/versions-get-resp.json.tpl | 2 +- .../api_sample_tests/test_evacuate.py | 47 ++++++++- .../api/openstack/compute/test_evacuate.py | 99 +++++++++++++++++++ .../api/openstack/compute/test_versions.py | 4 +- ...ag-from-evacuate-api-76a3d58616479fe9.yaml | 7 ++ 16 files changed, 243 insertions(+), 28 deletions(-) create mode 100644 doc/api_samples/os-evacuate/v2.14/server-evacuate-find-host-req.json create mode 100644 doc/api_samples/os-evacuate/v2.14/server-evacuate-req.json create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.14/server-evacuate-find-host-req.json.tpl create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.14/server-evacuate-req.json.tpl create mode 100644 releasenotes/notes/remove-on-shared-storage-flag-from-evacuate-api-76a3d58616479fe9.yaml diff --git a/doc/api_samples/os-evacuate/v2.14/server-evacuate-find-host-req.json b/doc/api_samples/os-evacuate/v2.14/server-evacuate-find-host-req.json new file mode 100644 index 0000000000..e8147c7a60 --- /dev/null +++ b/doc/api_samples/os-evacuate/v2.14/server-evacuate-find-host-req.json @@ -0,0 +1,5 @@ +{ + "evacuate": { + "adminPass": "MySecretPass" + } +} diff --git a/doc/api_samples/os-evacuate/v2.14/server-evacuate-req.json b/doc/api_samples/os-evacuate/v2.14/server-evacuate-req.json new file mode 100644 index 0000000000..5eea1aa67d --- /dev/null +++ b/doc/api_samples/os-evacuate/v2.14/server-evacuate-req.json @@ -0,0 +1,6 @@ +{ + "evacuate": { + "host": "testHost", + "adminPass": "MySecretPass" + } +} diff --git a/doc/api_samples/versions/v21-version-get-resp.json b/doc/api_samples/versions/v21-version-get-resp.json index 30d00dc1ce..c96aa6c877 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.13", + "version": "2.14", "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 901d39238d..45615c7278 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.13", + "version": "2.14", "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 905984971e..f90fd33fe5 100644 --- a/nova/api/openstack/api_version_request.py +++ b/nova/api/openstack/api_version_request.py @@ -54,6 +54,8 @@ REST_API_VERSION_HISTORY = """REST API Version History: * 2.11 - Exposes forced_down attribute for os-services * 2.12 - Exposes VIF net-id in os-virtual-interfaces * 2.13 - Add project id and user id information for os-server-groups API + * 2.14 - Remove onSharedStorage from evacuate request body and remove + adminPass from the response body """ # The minimum and maximum versions of the API supported @@ -62,7 +64,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.13" +_MAX_API_VERSION = "2.14" DEFAULT_API_VERSION = _MIN_API_VERSION diff --git a/nova/api/openstack/compute/evacuate.py b/nova/api/openstack/compute/evacuate.py index 7b42982110..f18ecace15 100644 --- a/nova/api/openstack/compute/evacuate.py +++ b/nova/api/openstack/compute/evacuate.py @@ -17,6 +17,7 @@ from oslo_config import cfg from oslo_utils import strutils from webob import exc +from nova.api.openstack import api_version_request from nova.api.openstack import common from nova.api.openstack.compute.schemas import evacuate from nova.api.openstack import extensions @@ -41,24 +42,14 @@ class EvacuateController(wsgi.Controller): self.compute_api = compute.API(skip_policy_check=True) self.host_api = compute.HostAPI() - # TODO(eliqiao): Should be responding here with 202 Accept - # because evacuate is an async call, but keep to 200 for - # backwards compatibility reasons. - @extensions.expected_errors((400, 404, 409)) - @wsgi.action('evacuate') - @validation.schema(evacuate.evacuate) - def _evacuate(self, req, id, body): - """Permit admins to evacuate a server from a failed host - to a new one. - """ - context = req.environ["nova.context"] - authorize(context) - - evacuate_body = body["evacuate"] - host = evacuate_body.get("host") - on_shared_storage = strutils.bool_from_string( - evacuate_body["onSharedStorage"]) + def _get_on_shared_storage(self, req, evacuate_body): + if (req.api_version_request < + api_version_request.APIVersionRequest("2.14")): + return strutils.bool_from_string(evacuate_body["onSharedStorage"]) + else: + return None + def _get_password(self, req, evacuate_body, on_shared_storage): password = None if 'adminPass' in evacuate_body: # check that if requested to evacuate server on shared storage @@ -71,6 +62,42 @@ class EvacuateController(wsgi.Controller): elif not on_shared_storage: password = utils.generate_password() + return password + + def _get_password_v214(self, req, evacuate_body): + if 'adminPass' in evacuate_body: + password = evacuate_body['adminPass'] + else: + password = utils.generate_password() + + return password + + # TODO(eliqiao): Should be responding here with 202 Accept + # because evacuate is an async call, but keep to 200 for + # backwards compatibility reasons. + @extensions.expected_errors((400, 404, 409)) + @wsgi.action('evacuate') + @validation.schema(evacuate.evacuate, "2.1", "2.12") + @validation.schema(evacuate.evacuate_v214, "2.14") + def _evacuate(self, req, id, body): + """Permit admins to evacuate a server from a failed host + to a new one. + """ + context = req.environ["nova.context"] + authorize(context) + + evacuate_body = body["evacuate"] + host = evacuate_body.get("host") + + on_shared_storage = self._get_on_shared_storage(req, evacuate_body) + + if (req.api_version_request < + api_version_request.APIVersionRequest("2.14")): + password = self._get_password(req, evacuate_body, + on_shared_storage) + else: + password = self._get_password_v214(req, evacuate_body) + if host is not None: try: self.host_api.service_get_by_compute_host(context, host) @@ -94,10 +121,12 @@ class EvacuateController(wsgi.Controller): except exception.ComputeServiceInUse as e: raise exc.HTTPBadRequest(explanation=e.format_message()) - if CONF.enable_instance_password: + if (req.api_version_request < + api_version_request.APIVersionRequest("2.14") and + CONF.enable_instance_password): return {'adminPass': password} else: - return {} + return None class Evacuate(extensions.V21APIExtensionBase): diff --git a/nova/api/openstack/compute/schemas/evacuate.py b/nova/api/openstack/compute/schemas/evacuate.py index ac0aac5e1f..51fb05410f 100644 --- a/nova/api/openstack/compute/schemas/evacuate.py +++ b/nova/api/openstack/compute/schemas/evacuate.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +import copy + from nova.api.validation import parameter_types @@ -32,3 +34,7 @@ evacuate = { 'required': ['evacuate'], 'additionalProperties': False, } + +evacuate_v214 = copy.deepcopy(evacuate) +del evacuate_v214['properties']['evacuate']['properties']['onSharedStorage'] +del evacuate_v214['properties']['evacuate']['required'] diff --git a/nova/api/openstack/rest_api_version_history.rst b/nova/api/openstack/rest_api_version_history.rst index 6f55fe0ac0..4ab5b846a7 100644 --- a/nova/api/openstack/rest_api_version_history.rst +++ b/nova/api/openstack/rest_api_version_history.rst @@ -137,3 +137,10 @@ user documentation. Add information ``project_id`` and ``user_id`` to ``os-server-groups`` API response data. +2.14 +---- + + Remove ``onSharedStorage`` parameter from server's evacuate action. Nova will + automatically detect if the instance is on shared storage. + Also adminPass is removed from the response body. The user can get the + password with the server's os-server-password action. diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.14/server-evacuate-find-host-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.14/server-evacuate-find-host-req.json.tpl new file mode 100644 index 0000000000..8abf0b4e18 --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.14/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.14/server-evacuate-req.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.14/server-evacuate-req.json.tpl new file mode 100644 index 0000000000..37eb2fe119 --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-evacuate/v2.14/server-evacuate-req.json.tpl @@ -0,0 +1,6 @@ +{ + "evacuate": { + "host": "%(host)s", + "adminPass": "%(adminPass)s" + } +} diff --git a/nova/tests/functional/api_sample_tests/api_samples/versions/v21-version-get-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/versions/v21-version-get-resp.json.tpl index 8f3628e1a4..2b74f0c274 100644 --- a/nova/tests/functional/api_sample_tests/api_samples/versions/v21-version-get-resp.json.tpl +++ b/nova/tests/functional/api_sample_tests/api_samples/versions/v21-version-get-resp.json.tpl @@ -19,7 +19,7 @@ } ], "status": "CURRENT", - "version": "2.13", + "version": "2.14", "min_version": "2.1", "updated": "2013-07-23T11:33:21Z" } diff --git a/nova/tests/functional/api_sample_tests/api_samples/versions/versions-get-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/versions/versions-get-resp.json.tpl index f87977b01e..423e3b4c1c 100644 --- a/nova/tests/functional/api_sample_tests/api_samples/versions/versions-get-resp.json.tpl +++ b/nova/tests/functional/api_sample_tests/api_samples/versions/versions-get-resp.json.tpl @@ -22,7 +22,7 @@ } ], "status": "CURRENT", - "version": "2.13", + "version": "2.14", "min_version": "2.1", "updated": "2013-07-23T11:33:21Z" } diff --git a/nova/tests/functional/api_sample_tests/test_evacuate.py b/nova/tests/functional/api_sample_tests/test_evacuate.py index 895fc09c46..a76fceefef 100644 --- a/nova/tests/functional/api_sample_tests/test_evacuate.py +++ b/nova/tests/functional/api_sample_tests/test_evacuate.py @@ -70,8 +70,15 @@ class EvacuateJsonTest(test_servers.ServersSampleBase): response = self._do_post('servers/%s/action' % self.uuid, server_req, req_subs) - subs = self._get_regexes() - self._verify_response(server_resp, subs, response, expected_resp_code) + if server_resp: + subs = self._get_regexes() + self._verify_response(server_resp, subs, response, + expected_resp_code) + else: + # NOTE(gibi): no server_resp means we expect empty body as + # a response + self.assertEqual(expected_resp_code, response.status_code) + self.assertEqual('', response.content) @mock.patch('nova.conductor.manager.ComputeTaskManager.rebuild_instance') def test_server_evacuate(self, rebuild_mock): @@ -105,3 +112,39 @@ class EvacuateJsonTest(test_servers.ServersSampleBase): orig_sys_metadata=mock.ANY, bdms=mock.ANY, recreate=mock.ANY, on_shared_storage=False, preserve_ephemeral=mock.ANY, host=None) + + +class EvacuateJsonTestV214(EvacuateJsonTest): + microversion = '2.14' + scenarios = [('v2_14', {'api_major_version': 'v2.1'})] + + @mock.patch('nova.conductor.manager.ComputeTaskManager.rebuild_instance') + def test_server_evacuate(self, rebuild_mock): + # Note (wingwj): The host can't be the same one + req_subs = { + 'host': 'testHost', + "adminPass": "MySecretPass", + } + 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='testHost') + + @mock.patch('nova.conductor.manager.ComputeTaskManager.rebuild_instance') + def test_server_evacuate_find_host(self, rebuild_mock): + req_subs = { + "adminPass": "MySecretPass", + } + self._test_evacuate(req_subs, 'server-evacuate-find-host-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) diff --git a/nova/tests/unit/api/openstack/compute/test_evacuate.py b/nova/tests/unit/api/openstack/compute/test_evacuate.py index 12333f287f..b365944d1c 100644 --- a/nova/tests/unit/api/openstack/compute/test_evacuate.py +++ b/nova/tests/unit/api/openstack/compute/test_evacuate.py @@ -16,6 +16,7 @@ import uuid import mock from oslo_config import cfg +import unittest import webob from nova.api.openstack.compute import evacuate as evacuate_v21 @@ -293,3 +294,101 @@ class EvacuatePolicyEnforcementv21(test.NoDBTestCase): self.assertEqual( "Policy doesn't allow %s to be performed." % rule_name, exc.format_message()) + + +class EvacuateTestV214(EvacuateTestV21): + def setUp(self): + super(EvacuateTestV214, self).setUp() + self.admin_req = fakes.HTTPRequest.blank('', use_admin_context=True, + version='2.14') + self.req = fakes.HTTPRequest.blank('', version='2.14') + + def _get_evacuate_response(self, json_load, uuid=None): + json_load.pop('onSharedStorage', None) + base_json_load = {'evacuate': json_load} + response = self.controller._evacuate(self.admin_req, uuid or self.UUID, + body=base_json_load) + + return response + + def _check_evacuate_failure(self, exception, body, uuid=None, + controller=None): + controller = controller or self.controller + body.pop('onSharedStorage', None) + body = {'evacuate': body} + self.assertRaises(exception, + controller._evacuate, + self.admin_req, uuid or self.UUID, body=body) + + @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] + self.assertEqual(CONF.password_length, len(admin_pass)) + self.assertIsNone(on_shared_storage) + + def test_evacuate_with_valid_instance(self): + admin_pass = 'MyNewPass' + res = self._get_evacuate_response({'host': 'my-host', + 'adminPass': admin_pass}) + + self.assertIsNone(res) + + @unittest.skip('Password is not returned from Microversion 2.14') + def test_evacuate_disable_password_return(self): + pass + + @unittest.skip('Password is not returned from Microversion 2.14') + def test_evacuate_enable_password_return(self): + pass + + @unittest.skip('onSharedStorage was removed from Microversion 2.14') + def test_evacuate_instance_with_invalid_on_shared_storage(self): + pass + + @unittest.skip('onSharedStorage was removed from Microversion 2.14') + @mock.patch('nova.objects.Instance.save') + def test_evacuate_not_shared_pass_generated(self, mock_save): + pass + + @mock.patch.object(compute_api.API, 'evacuate') + @mock.patch('nova.objects.Instance.save') + def test_evacuate_pass_generated(self, mock_save, mock_evacuate): + self._get_evacuate_response({'host': 'my-host'}) + self.assertEqual(CONF.password_length, + len(mock_evacuate.call_args_list[0][0][4])) + + def test_evacuate_instance_without_on_shared_storage(self): + self._get_evacuate_response({'host': 'my-host', + 'adminPass': 'MyNewPass'}) + + def test_evacuate_instance_with_no_target(self): + admin_pass = 'MyNewPass' + with mock.patch.object(compute_api.API, 'evacuate') as mock_evacuate: + self._get_evacuate_response({'adminPass': admin_pass}) + self.assertEqual(admin_pass, + mock_evacuate.call_args_list[0][0][4]) + + def test_not_admin(self): + body = {'evacuate': {'host': 'my-host'}} + self.assertRaises(exception.PolicyNotAuthorized, + self.controller._evacuate, + self.req, self.UUID, body=body) + + @unittest.skip('onSharedStorage was removed from Microversion 2.14') + @mock.patch('nova.objects.Instance.save') + def test_evacuate_shared_and_pass(self, mock_save): + pass + + @unittest.skip('from Microversion 2.14 it is covered with ' + 'test_evacuate_pass_generated') + def test_evacuate_instance_with_target(self): + pass + + @mock.patch('nova.objects.Instance.save') + def test_evacuate_instance_with_underscore_in_hostname(self, mock_save): + # NOTE: The hostname grammar in RFC952 does not allow for + # 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'}) diff --git a/nova/tests/unit/api/openstack/compute/test_versions.py b/nova/tests/unit/api/openstack/compute/test_versions.py index 5e306cabaa..86fe053b9e 100644 --- a/nova/tests/unit/api/openstack/compute/test_versions.py +++ b/nova/tests/unit/api/openstack/compute/test_versions.py @@ -66,7 +66,7 @@ EXP_VERSIONS = { "v2.1": { "id": "v2.1", "status": "CURRENT", - "version": "2.13", + "version": "2.14", "min_version": "2.1", "updated": "2013-07-23T11:33:21Z", "links": [ @@ -128,7 +128,7 @@ class VersionsTestV20(test.NoDBTestCase): { "id": "v2.1", "status": "CURRENT", - "version": "2.13", + "version": "2.14", "min_version": "2.1", "updated": "2013-07-23T11:33:21Z", "links": [ diff --git a/releasenotes/notes/remove-on-shared-storage-flag-from-evacuate-api-76a3d58616479fe9.yaml b/releasenotes/notes/remove-on-shared-storage-flag-from-evacuate-api-76a3d58616479fe9.yaml new file mode 100644 index 0000000000..f9bcfd64be --- /dev/null +++ b/releasenotes/notes/remove-on-shared-storage-flag-from-evacuate-api-76a3d58616479fe9.yaml @@ -0,0 +1,7 @@ +--- +features: + - Remove ``onSharedStorage`` parameter from server's evacuate action in + microversion 2.14. Nova will automatically detect if the instance is on + shared storage. Also adminPass is removed from the response body which + makes the response body empty. The user can get the password with the + server's os-server-password action.