From 7aa2285e724345717a3f333adc13660d7b97dfcd Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Wed, 18 May 2016 22:00:23 +0200 Subject: [PATCH] API change for verifying the scheduler when live migrating After modifying the evacuate action, we now add a new microversion change for modifying the live-migrate call so that the scheduler is called when the admin user provides an hostname unless the force field is provided. APIImpact Implements: blueprint check-destination-on-migrations-newton Change-Id: I212cbb44f46d7cb36b5d8c74a79065d38fc526d8 --- api-ref/source/parameters.yaml | 8 +++ api-ref/source/servers-admin-action.inc | 1 + .../v2.30/live-migrate-server.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/migrate_server.py | 13 ++++- .../compute/schemas/migrate_server.py | 4 ++ .../openstack/rest_api_version_history.rst | 9 +++ nova/compute/api.py | 26 ++++++++- .../v2.30/live-migrate-server.json.tpl | 7 +++ .../api_sample_tests/test_migrate_server.py | 45 +++++++++++++-- .../openstack/compute/test_migrate_server.py | 57 ++++++++++++++++--- nova/tests/unit/compute/test_compute.py | 45 +++++++++++++-- ...ination_when_livemig-e69d32e02d7a18c9.yaml | 11 ++++ 15 files changed, 220 insertions(+), 21 deletions(-) create mode 100644 doc/api_samples/os-migrate-server/v2.30/live-migrate-server.json create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-migrate-server/v2.30/live-migrate-server.json.tpl create mode 100644 releasenotes/notes/check_destination_when_livemig-e69d32e02d7a18c9.yaml diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index eeb4793eddc4..be5d642bc7a1 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -1413,6 +1413,14 @@ force_evacuate: required: false type: boolean min_version: 2.29 +force_live_migrate: + description: | + Force a live-migration by not verifying the provided destination host by + the scheduler. + in: body + required: false + type: boolean + min_version: 2.30 forced_down: in: body required: true diff --git a/api-ref/source/servers-admin-action.inc b/api-ref/source/servers-admin-action.inc index 0cc08265bf99..ca2cbdb5598a 100644 --- a/api-ref/source/servers-admin-action.inc +++ b/api-ref/source/servers-admin-action.inc @@ -170,6 +170,7 @@ Request - host: host_migration - block_migration: block_migration - disk_over_commit: disk_over_commit + - force: force_live_migrate **Example Live-Migrate Server (os-migrateLive Action): JSON request** diff --git a/doc/api_samples/os-migrate-server/v2.30/live-migrate-server.json b/doc/api_samples/os-migrate-server/v2.30/live-migrate-server.json new file mode 100644 index 000000000000..6267fc26c2b0 --- /dev/null +++ b/doc/api_samples/os-migrate-server/v2.30/live-migrate-server.json @@ -0,0 +1,7 @@ +{ + "os-migrateLive": { + "host": "01c0cadef72d47e28a672a76060d492c", + "block_migration": "auto", + "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 424822696ee7..d8e1bb45c7c0 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.29", + "version": "2.30", "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 1a3c5e29c001..78626ded0a56 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.29", + "version": "2.30", "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 987b898589a9..da90b07d3f64 100644 --- a/nova/api/openstack/api_version_request.py +++ b/nova/api/openstack/api_version_request.py @@ -77,6 +77,8 @@ REST_API_VERSION_HISTORY = """REST API Version History: * 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. + * 2.30 - Add a force flag in live-migrate request body and change the + behaviour for the host flag by calling the scheduler. """ # The minimum and maximum versions of the API supported @@ -85,7 +87,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.29" +_MAX_API_VERSION = "2.30" DEFAULT_API_VERSION = _MIN_API_VERSION diff --git a/nova/api/openstack/compute/migrate_server.py b/nova/api/openstack/compute/migrate_server.py index d2633a11e02a..c583f113b0b5 100644 --- a/nova/api/openstack/compute/migrate_server.py +++ b/nova/api/openstack/compute/migrate_server.py @@ -24,6 +24,7 @@ from nova.api.openstack import wsgi from nova.api import validation from nova import compute from nova import exception +from nova.i18n import _ ALIAS = "os-migrate-server" @@ -63,7 +64,8 @@ class MigrateServerController(wsgi.Controller): @extensions.expected_errors((400, 404, 409)) @wsgi.action('os-migrateLive') @validation.schema(migrate_server.migrate_live, "2.1", "2.24") - @validation.schema(migrate_server.migrate_live_v2_25, "2.25") + @validation.schema(migrate_server.migrate_live_v2_25, "2.25", "2.29") + @validation.schema(migrate_server.migrate_live_v2_30, "2.30") def _migrate_live(self, req, id, body): """Permit admins to (live) migrate a server to a new host.""" context = req.environ["nova.context"] @@ -71,7 +73,14 @@ class MigrateServerController(wsgi.Controller): host = body["os-migrateLive"]["host"] block_migration = body["os-migrateLive"]["block_migration"] + force = None + if api_version_request.is_supported(req, min_version='2.30'): + force = body["os-migrateLive"].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.25'): if block_migration == 'auto': block_migration = None @@ -90,7 +99,7 @@ class MigrateServerController(wsgi.Controller): try: instance = common.get_instance(self.compute_api, context, id) self.compute_api.live_migrate(context, instance, block_migration, - disk_over_commit, host) + disk_over_commit, host, force) except exception.InstanceUnknownCell as e: raise exc.HTTPNotFound(explanation=e.format_message()) except (exception.NoValidHost, diff --git a/nova/api/openstack/compute/schemas/migrate_server.py b/nova/api/openstack/compute/schemas/migrate_server.py index 0121fa9db439..f2264d074f4e 100644 --- a/nova/api/openstack/compute/schemas/migrate_server.py +++ b/nova/api/openstack/compute/schemas/migrate_server.py @@ -49,3 +49,7 @@ migrate_live_v2_25['properties']['os-migrateLive']['properties'][ 'block_migration'] = block_migration migrate_live_v2_25['properties']['os-migrateLive']['required'] = ( ['block_migration', 'host']) + +migrate_live_v2_30 = copy.deepcopy(migrate_live_v2_25) +migrate_live_v2_30['properties']['os-migrateLive']['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 77d6127442af..b622c28562f5 100644 --- a/nova/api/openstack/rest_api_version_history.rst +++ b/nova/api/openstack/rest_api_version_history.rst @@ -309,3 +309,12 @@ user documentation. 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. + +2.30 +---- + +Updates the POST request body for the ``live-migrate`` action to include the +optional ``force`` boolean field defaulted to False. +Also changes the live-migrate 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 99fd30aa8e5c..17490a1f4ce7 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -3352,7 +3352,7 @@ class API(base.Base): @check_instance_cell @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.PAUSED]) def live_migrate(self, context, instance, block_migration, - disk_over_commit, host_name): + disk_over_commit, host_name, force=None): """Migrate a server lively to a new host.""" LOG.debug("Going to try to live migrate instance to %s", host_name or "another host", instance=instance) @@ -3369,6 +3369,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_name: + nodes = objects.ComputeNodeList.get_all_by_host(context, host_name) + if not nodes: + raise exception.ComputeHostNotFound(host=host_name) + # NOTE(sbauza): Unset the host to make sure we call the scheduler + host_name = 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 + try: self.compute_task_api.live_migrate_instance(context, instance, host_name, block_migration=block_migration, diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-migrate-server/v2.30/live-migrate-server.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-migrate-server/v2.30/live-migrate-server.json.tpl new file mode 100644 index 000000000000..6ad9b0630a61 --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-migrate-server/v2.30/live-migrate-server.json.tpl @@ -0,0 +1,7 @@ +{ + "os-migrateLive": { + "host": "%(hostname)s", + "block_migration": "auto", + "force": "%(force)s" + } +} diff --git a/nova/tests/functional/api_sample_tests/test_migrate_server.py b/nova/tests/functional/api_sample_tests/test_migrate_server.py index e620e7dbccc8..512ad7061d2a 100644 --- a/nova/tests/functional/api_sample_tests/test_migrate_server.py +++ b/nova/tests/functional/api_sample_tests/test_migrate_server.py @@ -17,6 +17,7 @@ import mock from oslo_utils import versionutils import nova.conf +from nova import objects from nova.tests.functional.api_sample_tests import test_servers CONF = nova.conf.CONF @@ -40,6 +41,7 @@ class MigrateServerSamplesJsonTest(test_servers.ServersSampleBase): """ super(MigrateServerSamplesJsonTest, self).setUp() self.uuid = self._post_server() + self.host_attended = self.compute.host @mock.patch('nova.conductor.manager.ComputeTaskManager._cold_migrate') def test_post_migrate(self, mock_cold_migrate): @@ -48,13 +50,15 @@ class MigrateServerSamplesJsonTest(test_servers.ServersSampleBase): 'migrate-server', {}) self.assertEqual(202, response.status_code) - def test_post_live_migrate_server(self): - # Get api samples to server live migrate request. + def _check_post_live_migrate_server(self, req_subs=None): + if not req_subs: + req_subs = {'hostname': self.compute.host} + def fake_live_migrate(_self, context, instance, scheduler_hint, block_migration, disk_over_commit, request_spec): self.assertEqual(self.uuid, instance["uuid"]) host = scheduler_hint["host"] - self.assertEqual(self.compute.host, host) + self.assertEqual(self.host_attended, host) self.stub_out( 'nova.conductor.manager.ComputeTaskManager._live_migrate', @@ -75,9 +79,13 @@ class MigrateServerSamplesJsonTest(test_servers.ServersSampleBase): response = self._do_post('servers/%s/action' % self.uuid, 'live-migrate-server', - {'hostname': self.compute.host}) + req_subs) self.assertEqual(202, response.status_code) + def test_post_live_migrate_server(self): + # Get api samples to server live migrate request. + self._check_post_live_migrate_server() + class MigrateServerSamplesJsonTestV225(MigrateServerSamplesJsonTest): extension_name = "os-migrate-server" @@ -87,3 +95,32 @@ class MigrateServerSamplesJsonTestV225(MigrateServerSamplesJsonTest): def test_post_migrate(self): # no changes for migrate-server pass + + +class MigrateServerSamplesJsonTestV230(MigrateServerSamplesJsonTest): + extension_name = "os-migrate-server" + microversion = '2.30' + scenarios = [('v2_30', {'api_major_version': 'v2.1'})] + + def test_post_migrate(self): + # no changes for migrate-server + pass + + @mock.patch('nova.objects.ComputeNodeList.get_all_by_host') + def test_post_live_migrate_server(self, compute_node_get_all_by_host): + # Get api samples to server live migrate request. + + fake_computes = objects.ComputeNodeList( + objects=[objects.ComputeNode(host='testHost', + hypervisor_hostname='host')]) + compute_node_get_all_by_host.return_value = fake_computes + self.host_attended = None + self._check_post_live_migrate_server( + req_subs={'hostname': self.compute.host, + 'force': 'False'}) + + def test_post_live_migrate_server_with_force(self): + self.host_attended = self.compute.host + self._check_post_live_migrate_server( + req_subs={'hostname': self.compute.host, + 'force': 'True'}) diff --git a/nova/tests/unit/api/openstack/compute/test_migrate_server.py b/nova/tests/unit/api/openstack/compute/test_migrate_server.py index f56f8ab9c936..fe5e5690f932 100644 --- a/nova/tests/unit/api/openstack/compute/test_migrate_server.py +++ b/nova/tests/unit/api/openstack/compute/test_migrate_server.py @@ -32,6 +32,7 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): validation_error = exception.ValidationError _api_version = '2.1' disk_over_commit = False + force = None def setUp(self): super(MigrateServerTestsV21, self).setUp() @@ -58,7 +59,7 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): '_migrate_live': 'live_migrate'} body_map = {'_migrate_live': self._get_migration_body(host='hostname')} args_map = {'_migrate_live': ((False, self.disk_over_commit, - 'hostname'), {})} + 'hostname', self.force), {})} self._test_actions(['_migrate', '_migrate_live'], body_map=body_map, method_translations=method_translations, args_map=args_map) @@ -67,7 +68,8 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): method_translations = {'_migrate': 'resize', '_migrate_live': 'live_migrate'} body_map = {'_migrate_live': self._get_migration_body(host=None)} - args_map = {'_migrate_live': ((False, self.disk_over_commit, None), + args_map = {'_migrate_live': ((False, self.disk_over_commit, None, + self.force), {})} self._test_actions(['_migrate', '_migrate_live'], body_map=body_map, method_translations=method_translations, @@ -83,7 +85,7 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): '_migrate_live': 'live_migrate'} body_map = self._get_migration_body(host='hostname') args_map = {'_migrate_live': ((False, self.disk_over_commit, - 'hostname'), {})} + 'hostname', self.force), {})} exception_arg = {'_migrate': 'migrate', '_migrate_live': 'os-migrateLive'} self._test_actions_raise_conflict_on_invalid_state( @@ -98,7 +100,7 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): body_map = {'_migrate_live': self._get_migration_body(host='hostname')} args_map = {'_migrate_live': ((False, self.disk_over_commit, - 'hostname'), {})} + 'hostname', self.force), {})} self._test_actions_with_locked_instance( ['_migrate', '_migrate_live'], body_map=body_map, args_map=args_map, method_translations=method_translations) @@ -122,7 +124,8 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): self.mox.StubOutWithMock(self.compute_api, 'live_migrate') instance = self._stub_instance_get() self.compute_api.live_migrate(self.context, instance, False, - self.disk_over_commit, 'hostname') + self.disk_over_commit, 'hostname', + self.force) self.mox.ReplayAll() @@ -201,7 +204,8 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): instance = self._stub_instance_get(uuid=uuid) self.compute_api.live_migrate(self.context, instance, False, self.disk_over_commit, - 'hostname').AndRaise(fake_exc) + 'hostname', self.force + ).AndRaise(fake_exc) self.mox.ReplayAll() @@ -304,7 +308,8 @@ class MigrateServerTestsV225(MigrateServerTestsV21): method_translations = {'_migrate_live': 'live_migrate'} body_map = {'_migrate_live': {'os-migrateLive': {'host': 'hostname', 'block_migration': 'auto'}}} - args_map = {'_migrate_live': ((None, None, 'hostname'), {})} + args_map = {'_migrate_live': ((None, None, 'hostname', self.force), + {})} self._test_actions(['_migrate_live'], body_map=body_map, method_translations=method_translations, args_map=args_map) @@ -323,6 +328,44 @@ class MigrateServerTestsV225(MigrateServerTestsV21): exception.LiveMigrationWithOldNovaNotSupported()) +class MigrateServerTestsV230(MigrateServerTestsV225): + + force = False + + def setUp(self): + super(MigrateServerTestsV230, self).setUp() + self.req.api_version_request = api_version_request.APIVersionRequest( + '2.30') + + def _test_live_migrate(self, force=False): + if force is True: + litteral_force = 'true' + else: + litteral_force = 'false' + method_translations = {'_migrate_live': 'live_migrate'} + body_map = {'_migrate_live': {'os-migrateLive': {'host': 'hostname', + 'block_migration': 'auto', + 'force': litteral_force}}} + args_map = {'_migrate_live': ((None, None, 'hostname', force), + {})} + self._test_actions(['_migrate_live'], body_map=body_map, + method_translations=method_translations, + args_map=args_map) + + def test_live_migrate(self): + self._test_live_migrate() + + def test_live_migrate_with_forced_host(self): + self._test_live_migrate(force=True) + + def test_forced_live_migrate_with_no_provided_host(self): + body = {'os-migrateLive': + {'force': 'true'}} + self.assertRaises(self.validation_error, + self.controller._migrate_live, + self.req, fakes.FAKE_UUID, body=body) + + class MigrateServerPolicyEnforcementV21(test.NoDBTestCase): def setUp(self): diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 655aa9a052cc..19428460bcd7 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -10086,28 +10086,38 @@ class ComputeAPITestCase(BaseTestCase): mock_refresh.assert_called_once_with(mock.sentinel.ctxt, mock.sentinel.instances) - def test_live_migrate(self): + def _test_live_migrate(self, force=None): instance, instance_uuid = self._run_instance() rpcapi = self.compute_api.compute_task_api fake_spec = objects.RequestSpec() @mock.patch.object(rpcapi, 'live_migrate_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, '_record_action_start') def do_test(record_action_start, get_by_instance_uuid, - live_migrate_instance): + get_all_by_host, live_migrate_instance): get_by_instance_uuid.return_value = fake_spec + get_all_by_host.return_value = objects.ComputeNodeList( + objects=[objects.ComputeNode( + host='fake_dest_host', + hypervisor_hostname='fake_dest_node')]) self.compute_api.live_migrate(self.context, instance, block_migration=True, disk_over_commit=True, - host_name='fake_dest_host') + host_name='fake_dest_host', + force=force) record_action_start.assert_called_once_with(self.context, instance, 'live-migration') + if force is False: + host = None + else: + host = 'fake_dest_host' live_migrate_instance.assert_called_once_with( - self.context, instance, 'fake_dest_host', + self.context, instance, host, block_migration=True, disk_over_commit=True, request_spec=fake_spec) @@ -10115,6 +10125,33 @@ class ComputeAPITestCase(BaseTestCase): do_test() instance.refresh() self.assertEqual(instance['task_state'], task_states.MIGRATING) + 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_live_migrate(self): + self._test_live_migrate() + + def test_live_migrate_with_not_forced_host(self): + self._test_live_migrate(force=False) + + def test_live_migrate_with_forced_host(self): + self._test_live_migrate(force=True) + + def test_fail_live_migrate_with_non_existing_destination(self): + instance = self._create_fake_instance_obj(services=True) + self.assertIsNone(instance.task_state) + + self.assertRaises( + exception.ComputeHostNotFound, + self.compute_api.live_migrate, self.context.elevated(), + instance, block_migration=True, + disk_over_commit=True, + host_name='fake_dest_host', + force=False) def _test_evacuate(self, force=None): instance = self._create_fake_instance_obj(services=True) diff --git a/releasenotes/notes/check_destination_when_livemig-e69d32e02d7a18c9.yaml b/releasenotes/notes/check_destination_when_livemig-e69d32e02d7a18c9.yaml new file mode 100644 index 000000000000..ee48ab07ec28 --- /dev/null +++ b/releasenotes/notes/check_destination_when_livemig-e69d32e02d7a18c9.yaml @@ -0,0 +1,11 @@ +--- +features: + - On live-migrate 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 live-migrate operation + to the destination without verifying the scheduler.