From 3057989714e5e510e275c454258e0f167ed551d2 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Wed, 15 May 2019 17:57:29 -0400 Subject: [PATCH] Deprecate openstack server migrate --host option Per the discussion at the Train Forum [1] this deprecates the problematic --live option on the server migrate command which, depending on the compute API version used, forcefully bypasses the scheduler and also does not allow you to live migrate a server and let the scheduler pick a host. The --live option is replaced here with two new options: * --live-migration: this simply tells the command you want to perform a live rather than cold migration; if specified with --live the --live-migration option takes priority. * --host: when specified, this will request a target host for the live migration and will be validated by the scheduler; if not specified, the scheduler will pick a host. This option is mutually exclusive with --live. We can build on the --host option by supporting cold migrations with a specified host when using compute API version 2.56 or greater but that will come in a separate change. If the --live option is ever used we log a warning. Note there are several related changes for this issue: - https://review.openstack.org/#/c/628334/ - https://review.openstack.org/#/c/626949/ - https://review.openstack.org/#/c/627801/ - https://review.openstack.org/#/c/589012/ - https://review.openstack.org/#/c/460059/ This change allows us to deprecate the --live option and provide a replacement which is backward compatible without having to use something potentially error-prone like nargs='?'. Closes-Bug: #1411190 [1] https://etherpad.openstack.org/p/DEN-osc-compute-api-gaps Change-Id: I95d3d588e4abeb6848bdccf6915f7b5da40b5d4f --- openstackclient/compute/v2/server.py | 78 ++++++++- .../tests/unit/compute/v2/test_server.py | 158 +++++++++++++++++- ...-live-migration-host-655ae6befa6a3de2.yaml | 27 +++ 3 files changed, 255 insertions(+), 8 deletions(-) create mode 100644 releasenotes/notes/bug-1411190-live-migration-host-655ae6befa6a3de2.yaml diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index cb9f8d43eb..fefd4b3773 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -1407,9 +1407,38 @@ class MigrateServer(command.Command): help=_('Server (name or ID)'), ) parser.add_argument( + '--live-migration', + dest='live_migration', + action='store_true', + help=_('Live migrate the server. Use the ``--host`` option to ' + 'specify a target host for the migration which will be ' + 'validated by the scheduler.'), + ) + # The --live and --host options are mutually exclusive ways of asking + # for a target host during a live migration. + host_group = parser.add_mutually_exclusive_group() + # TODO(mriedem): Remove --live in the next major version bump after + # the Train release. + host_group.add_argument( '--live', metavar='', - help=_('Target hostname'), + help=_('**Deprecated** This option is problematic in that it ' + 'requires a host and prior to compute API version 2.30, ' + 'specifying a host during live migration will bypass ' + 'validation by the scheduler which could result in ' + 'failures to actually migrate the server to the specified ' + 'host or over-subscribe the host. Use the ' + '``--live-migration`` option instead. If both this option ' + 'and ``--live-migration`` are used, ``--live-migration`` ' + 'takes priority.'), + ) + # TODO(mriedem): Add support for --os-compute-api-version >= 2.56 where + # you can cold migrate to a specified target host. + host_group.add_argument( + '--host', + metavar='', + help=_('Live migrate the server to the specified host. Requires ' + '``--os-compute-api-version`` 2.30 or greater.'), ) migration_group = parser.add_mutually_exclusive_group() migration_group.add_argument( @@ -1447,6 +1476,15 @@ class MigrateServer(command.Command): ) return parser + def _log_warning_for_live(self, parsed_args): + if parsed_args.live: + # NOTE(mriedem): The --live option requires a host and if + # --os-compute-api-version is less than 2.30 it will forcefully + # bypass the scheduler which is dangerous. + self.log.warning(_( + 'The --live option has been deprecated. Please use the ' + '--live-migration option instead.')) + def take_action(self, parsed_args): def _show_progress(progress): @@ -1460,19 +1498,45 @@ class MigrateServer(command.Command): compute_client.servers, parsed_args.server, ) - if parsed_args.live: + # Check for live migration. + if parsed_args.live or parsed_args.live_migration: + # Always log a warning if --live is used. + self._log_warning_for_live(parsed_args) kwargs = { - 'host': parsed_args.live, 'block_migration': parsed_args.block_migration } + # Prefer --live-migration over --live if both are specified. + if parsed_args.live_migration: + # Technically we could pass a non-None host with + # --os-compute-api-version < 2.30 but that is the same thing + # as the --live option bypassing the scheduler which we don't + # want to support, so if the user is using --live-migration + # and --host, we want to enforce that they are using version + # 2.30 or greater. + if (parsed_args.host and + compute_client.api_version < + api_versions.APIVersion('2.30')): + raise exceptions.CommandError( + '--os-compute-api-version 2.30 or greater is required ' + 'when using --host') + # The host parameter is required in the API even if None. + kwargs['host'] = parsed_args.host + else: + kwargs['host'] = parsed_args.live + if compute_client.api_version < api_versions.APIVersion('2.25'): kwargs['disk_over_commit'] = parsed_args.disk_overcommit server.live_migrate(**kwargs) else: - if parsed_args.block_migration or parsed_args.disk_overcommit: - raise exceptions.CommandError("--live must be specified if " - "--block-migration or " - "--disk-overcommit is specified") + if (parsed_args.block_migration or parsed_args.disk_overcommit or + parsed_args.host): + # TODO(mriedem): Allow --host for cold migration if + # --os-compute-api-version >= 2.56. + raise exceptions.CommandError( + "--live-migration must be specified if " + "--block-migration, --disk-overcommit or --host is " + "specified") + server.migrate() if parsed_args.wait: diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index c30af8fb8e..2a301b4541 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -23,6 +23,7 @@ from openstack import exceptions as sdk_exceptions from osc_lib import exceptions from osc_lib import utils as common_utils from oslo_utils import timeutils +import six from openstackclient.compute.v2 import server from openstackclient.tests.unit.compute.v2 import fakes as compute_fakes @@ -2415,12 +2416,40 @@ class TestServerMigrate(TestServer): self.assertNotCalled(self.servers_mock.live_migrate) self.assertNotCalled(self.servers_mock.migrate) + def test_server_migrate_with_host(self): + # Tests that --host is not allowed for a cold migration. + arglist = [ + '--host', 'fakehost', self.server.id, + ] + verifylist = [ + ('live', None), + ('live_migration', False), + ('host', 'fakehost'), + ('block_migration', False), + ('disk_overcommit', False), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + ex = self.assertRaises(exceptions.CommandError, self.cmd.take_action, + parsed_args) + + # Make sure it's the error we expect. + self.assertIn("--live-migration must be specified if " + "--block-migration, --disk-overcommit or --host is " + "specified", six.text_type(ex)) + self.servers_mock.get.assert_called_with(self.server.id) + self.assertNotCalled(self.servers_mock.live_migrate) + self.assertNotCalled(self.servers_mock.migrate) + def test_server_live_migrate(self): arglist = [ '--live', 'fakehost', self.server.id, ] verifylist = [ ('live', 'fakehost'), + ('live_migration', False), + ('host', None), ('block_migration', False), ('disk_overcommit', False), ('wait', False), @@ -2430,7 +2459,8 @@ class TestServerMigrate(TestServer): self.app.client_manager.compute.api_version = \ api_versions.APIVersion('2.24') - result = self.cmd.take_action(parsed_args) + with mock.patch.object(self.cmd.log, 'warning') as mock_warning: + result = self.cmd.take_action(parsed_args) self.servers_mock.get.assert_called_with(self.server.id) self.server.live_migrate.assert_called_with(block_migration=False, @@ -2438,6 +2468,132 @@ class TestServerMigrate(TestServer): host='fakehost') self.assertNotCalled(self.servers_mock.migrate) self.assertIsNone(result) + # A warning should have been logged for using --live. + mock_warning.assert_called_once() + self.assertIn('The --live option has been deprecated.', + six.text_type(mock_warning.call_args[0][0])) + + def test_server_live_migrate_host_pre_2_30(self): + # Tests that the --host option is not supported for --live-migration + # before microversion 2.30 (the test defaults to 2.1). + arglist = [ + '--live-migration', '--host', 'fakehost', self.server.id, + ] + verifylist = [ + ('live', None), + ('live_migration', True), + ('host', 'fakehost'), + ('block_migration', False), + ('disk_overcommit', False), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + ex = self.assertRaises(exceptions.CommandError, self.cmd.take_action, + parsed_args) + + # Make sure it's the error we expect. + self.assertIn('--os-compute-api-version 2.30 or greater is required ' + 'when using --host', six.text_type(ex)) + + self.servers_mock.get.assert_called_with(self.server.id) + self.assertNotCalled(self.servers_mock.live_migrate) + self.assertNotCalled(self.servers_mock.migrate) + + def test_server_live_migrate_no_host(self): + # Tests the --live-migration option without --host or --live. + arglist = [ + '--live-migration', self.server.id, + ] + verifylist = [ + ('live', None), + ('live_migration', True), + ('host', None), + ('block_migration', False), + ('disk_overcommit', False), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + with mock.patch.object(self.cmd.log, 'warning') as mock_warning: + result = self.cmd.take_action(parsed_args) + + self.servers_mock.get.assert_called_with(self.server.id) + self.server.live_migrate.assert_called_with(block_migration=False, + disk_over_commit=False, + host=None) + self.assertNotCalled(self.servers_mock.migrate) + self.assertIsNone(result) + # Since --live wasn't used a warning shouldn't have been logged. + mock_warning.assert_not_called() + + def test_server_live_migrate_with_host(self): + # Tests the --live-migration option with --host but no --live. + # This requires --os-compute-api-version >= 2.30 so the test uses 2.30. + arglist = [ + '--live-migration', '--host', 'fakehost', self.server.id, + ] + verifylist = [ + ('live', None), + ('live_migration', True), + ('host', 'fakehost'), + ('block_migration', False), + ('disk_overcommit', False), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.app.client_manager.compute.api_version = \ + api_versions.APIVersion('2.30') + + result = self.cmd.take_action(parsed_args) + + self.servers_mock.get.assert_called_with(self.server.id) + # No disk_overcommit with microversion >= 2.25. + self.server.live_migrate.assert_called_with(block_migration=False, + host='fakehost') + self.assertNotCalled(self.servers_mock.migrate) + self.assertIsNone(result) + + def test_server_live_migrate_without_host_override_live(self): + # Tests the --live-migration option without --host and with --live. + # The --live-migration option will take precedence and a warning is + # logged for using --live. + arglist = [ + '--live', 'fakehost', '--live-migration', self.server.id, + ] + verifylist = [ + ('live', 'fakehost'), + ('live_migration', True), + ('host', None), + ('block_migration', False), + ('disk_overcommit', False), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + with mock.patch.object(self.cmd.log, 'warning') as mock_warning: + result = self.cmd.take_action(parsed_args) + + self.servers_mock.get.assert_called_with(self.server.id) + self.server.live_migrate.assert_called_with(block_migration=False, + disk_over_commit=False, + host=None) + self.assertNotCalled(self.servers_mock.migrate) + self.assertIsNone(result) + # A warning should have been logged for using --live. + mock_warning.assert_called_once() + self.assertIn('The --live option has been deprecated.', + six.text_type(mock_warning.call_args[0][0])) + + def test_server_live_migrate_live_and_host_mutex(self): + # Tests specifying both the --live and --host options which are in a + # mutex group so argparse should fail. + arglist = [ + '--live', 'fakehost', '--host', 'fakehost', self.server.id, + ] + self.assertRaises(utils.ParserException, + self.check_parser, self.cmd, arglist, verify_args=[]) def test_server_block_live_migrate(self): arglist = [ diff --git a/releasenotes/notes/bug-1411190-live-migration-host-655ae6befa6a3de2.yaml b/releasenotes/notes/bug-1411190-live-migration-host-655ae6befa6a3de2.yaml new file mode 100644 index 0000000000..927d6a6451 --- /dev/null +++ b/releasenotes/notes/bug-1411190-live-migration-host-655ae6befa6a3de2.yaml @@ -0,0 +1,27 @@ +--- +deprecations: + - | + The ``--live`` option on the ``openstack server migrate`` command has + been deprecated and is being replaced with two new options: + + * ``--live-migration``: This will signal that the migration is a live + migration. + * ``--host``: This can be used to request a target host for the live + migration but requires ``--os-compute-api-version`` 2.30 or greater + so the requested host can be validated by the scheduler. + + The ``--live`` option is problematic in that it requires a host and + prior to compute API version 2.30, specifying a host during live migration + will bypass validation by the scheduler which could result in failures to + actually migrate the server to the specified host or over-subscribe the + host. + + The ``--live`` and ``--host`` options are mutually exclusive. Furthermore, + if both the ``--live`` and ``--live-migration`` options are used the + ``--live-migration`` option takes priority. +fixes: + - | + `Bug 1411190`_ has been fixed by providing a ``--live-migration`` and + ``--host`` option to the ``openstack server migrate`` command. + + .. _Bug 1411190: https://bugs.launchpad.net/python-openstackclient/+bug/1411190