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 <sfinucan@redhat.com>

2 files changed, 84 insertions, 48 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index b1a86a23..c42486d9 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='<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.'),
+            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='<hostname>',
-            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 0548924d..5de6d006 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),
         ]