openstack/python-openstackclient
Code Issues Proposed changes

Deprecate openstack server migrate --host option

Browse Source 
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
This commit is contained in:
Matt Riedemann 2019-05-15 17:57:29 -04:00
parent 1bc44fcdc6
commit 3057989714
3 changed files with 255 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

78
openstackclient/compute/v2/server.py
View File

@ -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='<hostname>',
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='<hostname>',
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:

158
openstackclient/tests/unit/compute/v2/test_server.py
View File

@ -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 = [

27
releasenotes/notes/bug-1411190-live-migration-host-655ae6befa6a3de2.yaml Normal file
View File

@ -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