From 2bdf34dcc3f8957ff78709467197b5fcb44e07c3 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 21 Jan 2021 11:25:08 +0000 Subject: [PATCH] compute: Auto-configure shared/block live migration API microversion 2.25 introduced the 'block_migration=auto' value for the os-migrateLive server action. This is a sensible default that we should use, allowing users to avoid stating one of the '--block-migration' or '--shared-migration' parameters explicitly. While we're here, we take the opportunity to fix up some formatting in the function, which is really rather messy. Change-Id: Ieedc77d6dc3d4a3cd93b29672faa97dd4e8c1185 Signed-off-by: Stephen Finucane --- openstackclient/compute/v2/server.py | 87 +++++++++++++------ .../tests/unit/compute/v2/test_server.py | 45 +++++----- ...block-live-migration-437d461c914f8f2f.yaml | 6 ++ 3 files changed, 90 insertions(+), 48 deletions(-) create mode 100644 releasenotes/notes/auto-configure-block-live-migration-437d461c914f8f2f.yaml diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index b1a86a23df..c42486d9c7 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -2457,9 +2457,11 @@ revert to release the new server and restart the old one.""") '--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.'), + 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. @@ -2469,37 +2471,48 @@ revert to release the new server and restart the old one.""") host_group.add_argument( '--live', metavar='', - 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.'), + 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.' + ), ) host_group.add_argument( '--host', metavar='', - help=_('Migrate the server to the specified host. Requires ' - '``--os-compute-api-version`` 2.30 or greater when used ' - 'with the ``--live-migration`` option, otherwise requires ' - '``--os-compute-api-version`` 2.56 or greater.'), + help=_( + 'Migrate the server to the specified host. ' + '(supported with --os-compute-api-version 2.30 or above ' + 'when used with the --live-migration option) ' + '(supported with --os-compute-api-version 2.56 or above ' + 'when used without the --live-migration option)' + ), ) migration_group = parser.add_mutually_exclusive_group() migration_group.add_argument( '--shared-migration', dest='block_migration', action='store_false', - default=False, - help=_('Perform a shared live migration (default)'), + default=None, + help=_( + 'Perform a shared live migration ' + '(default before --os-compute-api-version 2.25, auto after)' + ), ) migration_group.add_argument( '--block-migration', dest='block_migration', action='store_true', - help=_('Perform a block live migration'), + help=_( + 'Perform a block live migration ' + '(auto-configured from --os-compute-api-version 2.25)' + ), ) disk_group = parser.add_mutually_exclusive_group() disk_group.add_argument( @@ -2513,8 +2526,9 @@ revert to release the new server and restart the old one.""") dest='disk_overcommit', action='store_false', default=False, - help=_('Do not over-commit disk on the' - ' destination host (default)'), + help=_( + 'Do not over-commit disk on the destination host (default)' + ), ) parser.add_argument( '--wait', @@ -2549,9 +2563,21 @@ revert to release the new server and restart the old one.""") 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 = { - 'block_migration': parsed_args.block_migration - } + + kwargs = {} + + block_migration = parsed_args.block_migration + if block_migration is None: + if ( + compute_client.api_version < + api_versions.APIVersion('2.25') + ): + block_migration = False + else: + block_migration = 'auto' + + kwargs['block_migration'] = 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 @@ -2560,12 +2586,16 @@ revert to release the new server and restart the old one.""") # 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')): + 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') + 'when using --host' + ) + # The host parameter is required in the API even if None. kwargs['host'] = parsed_args.host else: @@ -2573,6 +2603,7 @@ revert to release the new server and restart the old one.""") 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: diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 0548924d27..5de6d006ca 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -4524,7 +4524,7 @@ class TestServerMigrate(TestServer): ] verifylist = [ ('live', None), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', False), ] @@ -4547,7 +4547,7 @@ class TestServerMigrate(TestServer): ('live', None), ('live_migration', False), ('host', 'fakehost'), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', False), ] @@ -4588,7 +4588,7 @@ class TestServerMigrate(TestServer): ] verifylist = [ ('live', None), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', True), ('wait', False), ] @@ -4611,7 +4611,7 @@ class TestServerMigrate(TestServer): ('live', None), ('live_migration', False), ('host', 'fakehost'), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', False), ] @@ -4637,7 +4637,7 @@ class TestServerMigrate(TestServer): ('live', 'fakehost'), ('live_migration', False), ('host', None), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', False), ] @@ -4670,7 +4670,7 @@ class TestServerMigrate(TestServer): ('live', None), ('live_migration', True), ('host', 'fakehost'), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', False), ] @@ -4696,7 +4696,7 @@ class TestServerMigrate(TestServer): ('live', None), ('live_migration', True), ('host', None), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', False), ] @@ -4724,7 +4724,7 @@ class TestServerMigrate(TestServer): ('live', None), ('live_migration', True), ('host', 'fakehost'), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', False), ] @@ -4736,9 +4736,10 @@ class TestServerMigrate(TestServer): 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') + # No disk_overcommit and block_migration defaults to auto with + # microversion >= 2.25 + self.server.live_migrate.assert_called_with( + block_migration='auto', host='fakehost') self.assertNotCalled(self.servers_mock.migrate) self.assertIsNone(result) @@ -4753,7 +4754,7 @@ class TestServerMigrate(TestServer): ('live', 'fakehost'), ('live_migration', True), ('host', None), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', False), ] @@ -4779,8 +4780,9 @@ class TestServerMigrate(TestServer): arglist = [ '--live', 'fakehost', '--host', 'fakehost', self.server.id, ] - self.assertRaises(utils.ParserException, - self.check_parser, self.cmd, arglist, verify_args=[]) + self.assertRaises( + utils.ParserException, + self.check_parser, self.cmd, arglist, verify_args=[]) def test_server_block_live_migrate(self): arglist = [ @@ -4812,7 +4814,7 @@ class TestServerMigrate(TestServer): ] verifylist = [ ('live', 'fakehost'), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', True), ('wait', False), ] @@ -4861,7 +4863,7 @@ class TestServerMigrate(TestServer): ] verifylist = [ ('live', 'fakehost'), - ('block_migration', False), + ('block_migration', None), ('wait', False), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -4872,8 +4874,11 @@ class TestServerMigrate(TestServer): 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, - host='fakehost') + # No disk_overcommit and block_migration defaults to auto with + # microversion >= 2.25 + self.server.live_migrate.assert_called_with( + block_migration='auto', + host='fakehost') self.assertNotCalled(self.servers_mock.migrate) self.assertIsNone(result) @@ -4884,7 +4889,7 @@ class TestServerMigrate(TestServer): ] verifylist = [ ('live', None), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', True), ] @@ -4904,7 +4909,7 @@ class TestServerMigrate(TestServer): ] verifylist = [ ('live', None), - ('block_migration', False), + ('block_migration', None), ('disk_overcommit', False), ('wait', True), ] diff --git a/releasenotes/notes/auto-configure-block-live-migration-437d461c914f8f2f.yaml b/releasenotes/notes/auto-configure-block-live-migration-437d461c914f8f2f.yaml new file mode 100644 index 0000000000..e1ff187e62 --- /dev/null +++ b/releasenotes/notes/auto-configure-block-live-migration-437d461c914f8f2f.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + The ``server migrate`` command will now automatically determine whether + to use block or shared migration during a live migration operation. This + requires Compute API microversion 2.25 or greater.