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

doc/api_samples/os-evacuate/v2.14/server-evacuate-find-host-req.json

@@ -0,0 +1,5 @@
+{
+    "evacuate": {
+        "adminPass": "MySecretPass"
+    }
+}

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

@@ -0,0 +1,6 @@
+{
+    "evacuate": {
+        "host": "testHost",
+        "adminPass": "MySecretPass"
+    }
+}

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"
     }

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"
         }

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
 
 

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):

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']

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.

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"
+    }
+}

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"
+    }
+}

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"
     }

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"
         }

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)

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'})

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": [

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.