Volume status management for volume migration

This patch proposes the following changes
to the 'available' volume:
* Add the --lock-volume flag to cinder migrate
The default value is False, which means the migration is aborted
if the owner of the volume issues commands like attach
or detach during the volume migration. The volume will be in
'available' during migration.
If it is set to True, the migration of this volume will not be
aborted by other commands. The volume will be in 'maintenance'
during migration.

* List migration status for all the volumes
The attribute migration_status will be listed, if the request
is issued by an admin. Otherwise, the migration_status will not
be listed. The option migration_status is added for the admin
to filter the volumes returned via 'cinder list' command.

DocImpact
APIImpact

Partial-implements: blueprint migration-improvement
Change-Id: I5a1a717d1d08f550b45836d958a51f1f3fba5ced
Depends-On: Ia86421f2d6fce61dcfeb073f8e7b9c9dde517373

5 files changed, 101 insertions, 19 deletions

diff --git a/cinderclient/tests/unit/v2/test_shell.py b/cinderclient/tests/unit/v2/test_shell.py
index 6ba1a03..7bd692d 100644
--- a/cinderclient/tests/unit/v2/test_shell.py
+++ b/cinderclient/tests/unit/v2/test_shell.py
@@ -276,7 +276,8 @@ class ShellTest(utils.TestCase):
             with mock.patch('cinderclient.utils.print_list') as mock_print:
                 self.run_command(cmd)
                 mock_print.assert_called_once_with(
-                    mock.ANY, mock.ANY, sortby_index=None)
+                    mock.ANY, mock.ANY, exclude_unavailable=True,
+                    sortby_index=None)
 
     def test_list_reorder_without_sort(self):
         # sortby_index is 0 without sort information
@@ -284,7 +285,8 @@ class ShellTest(utils.TestCase):
             with mock.patch('cinderclient.utils.print_list') as mock_print:
                 self.run_command(cmd)
                 mock_print.assert_called_once_with(
-                    mock.ANY, mock.ANY, sortby_index=0)
+                    mock.ANY, mock.ANY, exclude_unavailable=True,
+                    sortby_index=0)
 
     def test_list_availability_zone(self):
         self.run_command('availability-zone-list')
@@ -710,17 +712,38 @@ class ShellTest(utils.TestCase):
         self.assert_called_anytime('GET', '/types/1')
 
     def test_migrate_volume(self):
-        self.run_command('migrate 1234 fakehost --force-host-copy=True')
+        self.run_command('migrate 1234 fakehost --force-host-copy=True '
+                         '--lock-volume=True')
         expected = {'os-migrate_volume': {'force_host_copy': 'True',
+                                          'lock_volume': 'True',
                                           'host': 'fakehost'}}
         self.assert_called('POST', '/volumes/1234/action', body=expected)
 
     def test_migrate_volume_bool_force(self):
-        self.run_command('migrate 1234 fakehost --force-host-copy')
+        self.run_command('migrate 1234 fakehost --force-host-copy '
+                         '--lock-volume')
         expected = {'os-migrate_volume': {'force_host_copy': True,
+                                          'lock_volume': True,
                                           'host': 'fakehost'}}
         self.assert_called('POST', '/volumes/1234/action', body=expected)
 
+    def test_migrate_volume_bool_force_false(self):
+        # Set both --force-host-copy and --lock-volume to False.
+        self.run_command('migrate 1234 fakehost --force-host-copy=False '
+                         '--lock-volume=False')
+        expected = {'os-migrate_volume': {'force_host_copy': 'False',
+                                          'lock_volume': 'False',
+                                          'host': 'fakehost'}}
+        self.assert_called('POST', '/volumes/1234/action', body=expected)
+
+        # Do not set the values to --force-host-copy and --lock-volume.
+        self.run_command('migrate 1234 fakehost')
+        expected = {'os-migrate_volume': {'force_host_copy': False,
+                                          'lock_volume': False,
+                                          'host': 'fakehost'}}
+        self.assert_called('POST', '/volumes/1234/action',
+                           body=expected)
+
     def test_snapshot_metadata_set(self):
         self.run_command('snapshot-metadata 1234 set key1=val1 key2=val2')
         self.assert_called('POST', '/snapshots/1234/metadata',
diff --git a/cinderclient/tests/unit/v2/test_volumes.py b/cinderclient/tests/unit/v2/test_volumes.py
index b56d908..08b9678 100644
--- a/cinderclient/tests/unit/v2/test_volumes.py
+++ b/cinderclient/tests/unit/v2/test_volumes.py
@@ -166,8 +166,19 @@ class VolumesTest(utils.TestCase):
 
     def test_migrate(self):
         v = cs.volumes.get('1234')
-        cs.volumes.migrate_volume(v, 'dest', False)
-        cs.assert_called('POST', '/volumes/1234/action')
+        cs.volumes.migrate_volume(v, 'dest', False, False)
+        cs.assert_called('POST', '/volumes/1234/action',
+                         {'os-migrate_volume': {'host': 'dest',
+                                                'force_host_copy': False,
+                                                'lock_volume': False}})
+
+    def test_migrate_with_lock_volume(self):
+        v = cs.volumes.get('1234')
+        cs.volumes.migrate_volume(v, 'dest', False, True)
+        cs.assert_called('POST', '/volumes/1234/action',
+                         {'os-migrate_volume': {'host': 'dest',
+                                                'force_host_copy': False,
+                                                'lock_volume': True}})
 
     def test_metadata_update_all(self):
         cs.volumes.update_all_metadata(1234, {'k1': 'v1'})
diff --git a/cinderclient/utils.py b/cinderclient/utils.py
index 89478bd..13baf11 100644
--- a/cinderclient/utils.py
+++ b/cinderclient/utils.py
@@ -110,11 +110,14 @@ def _print(pt, order):
         print(strutils.safe_encode(pt.get_string(sortby=order)))
 
 
-def print_list(objs, fields, formatters=None, sortby_index=0):
+def print_list(objs, fields, exclude_unavailable=False, formatters=None,
+               sortby_index=0):
     '''Prints a list of objects.
 
     @param objs: Objects to print
     @param fields: Fields on each object to be printed
+    @param exclude_unavailable: Boolean to decide if unavailable fields are
+                                removed
     @param formatters: Custom field formatters
     @param sortby_index: Results sorted against the key in the fields list at
                          this index; if None then the object order is not
@@ -122,12 +125,14 @@ def print_list(objs, fields, formatters=None, sortby_index=0):
     '''
     formatters = formatters or {}
     mixed_case_fields = ['serverId']
-    pt = prettytable.PrettyTable([f for f in fields], caching=False)
-    pt.aligns = ['l' for f in fields]
+    removed_fields = []
+    rows = []
 
     for o in objs:
         row = []
         for field in fields:
+            if field in removed_fields:
+                continue
             if field in formatters:
                 row.append(formatters[field](o))
             else:
@@ -138,12 +143,24 @@ def print_list(objs, fields, formatters=None, sortby_index=0):
                 if type(o) == dict and field in o:
                     data = o[field]
                 else:
-                    data = getattr(o, field_name, '')
+                    if not hasattr(o, field_name) and exclude_unavailable:
+                        removed_fields.append(field)
+                        continue
+                    else:
+                        data = getattr(o, field_name, '')
                 if data is None:
                     data = '-'
                 if isinstance(data, six.string_types) and "\r" in data:
                     data = data.replace("\r", " ")
                 row.append(data)
+        rows.append(row)
+
+    for f in removed_fields:
+        fields.remove(f)
+
+    pt = prettytable.PrettyTable((f for f in fields), caching=False)
+    pt.aligns = ['l' for f in fields]
+    for row in rows:
         pt.add_row(row)
 
     if sortby_index is None:
diff --git a/cinderclient/v2/shell.py b/cinderclient/v2/shell.py
index 7087f9f..b0caf9d 100644
--- a/cinderclient/v2/shell.py
+++ b/cinderclient/v2/shell.py
@@ -160,6 +160,11 @@ def _extract_metadata(args):
            metavar='<status>',
            default=None,
            help='Filters results by a status. Default=None.')
+@utils.arg('--migration_status',
+           metavar='<migration_status>',
+           default=None,
+           help='Filters results by a migration status. Default=None. '
+                'Admin only.')
 @utils.arg('--metadata',
            type=str,
            nargs='*',
@@ -212,6 +217,7 @@ def do_list(cs, args):
         'project_id': args.tenant,
         'name': args.name,
         'status': args.status,
+        'migration_status': args.migration_status,
         'metadata': _extract_metadata(args) if args.metadata else None,
     }
 
@@ -233,18 +239,19 @@ def do_list(cs, args):
         setattr(vol, 'attached_to', ','.join(map(str, servers)))
 
     if all_tenants:
-        key_list = ['ID', 'Tenant ID', 'Status', 'Name',
+        key_list = ['ID', 'Tenant ID', 'Status', 'Migration Status', 'Name',
                     'Size', 'Volume Type', 'Bootable', 'Multiattach',
                     'Attached to']
     else:
-        key_list = ['ID', 'Status', 'Name',
+        key_list = ['ID', 'Status', 'Migration Status', 'Name',
                     'Size', 'Volume Type', 'Bootable',
                     'Multiattach', 'Attached to']
     if args.sort_key or args.sort_dir or args.sort:
         sortby_index = None
     else:
         sortby_index = 0
-    utils.print_list(volumes, key_list, sortby_index=sortby_index)
+    utils.print_list(volumes, key_list, exclude_unavailable=True,
+                     sortby_index=sortby_index)
 
 
 @utils.arg('volume',
@@ -452,7 +459,8 @@ def do_force_delete(cs, args):
 @utils.arg('--state', metavar='<state>', default='available',
            help=('The state to assign to the volume. Valid values are '
                  '"available", "error", "creating", "deleting", "in-use", '
-                 '"attaching", "detaching" and "error_deleting". '
+                 '"attaching", "detaching", "error_deleting" and '
+                 '"maintenance". '
                  'NOTE: This command simply changes the state of the '
                  'Volume in the DataBase with no regard to actual status, '
                  'exercise caution when using. Default=available.'))
@@ -1153,11 +1161,31 @@ def do_upload_to_image(cs, args):
            help='Enables or disables generic host-based '
            'force-migration, which bypasses driver '
            'optimizations. Default=False.')
+@utils.arg('--lock-volume', metavar='<True|False>',
+           choices=['True', 'False'],
+           required=False,
+           const=True,
+           nargs='?',
+           default=False,
+           help='Enables or disables the termination of volume migration '
+           'caused by other commands. This option applies to the '
+           'available volume. True means it locks the volume '
+           'state and does not allow the migration to be aborted. The '
+           'volume status will be in maintenance during the '
+           'migration. False means it allows the volume migration '
+           'to be aborted. The volume status is still in the original '
+           'status. Default=False.')
 @utils.service_type('volumev2')
 def do_migrate(cs, args):
     """Migrates volume to a new host."""
     volume = utils.find_volume(cs, args.volume)
-    volume.migrate_volume(args.host, args.force_host_copy)
+    try:
+        volume.migrate_volume(args.host, args.force_host_copy,
+                              args.lock_volume)
+        print("Request to migrate volume %s has been accepted." % (volume))
+    except Exception as e:
+        print("Migration for volume %s failed: %s." % (volume,
+                                                       six.text_type(e)))
 
 
 @utils.arg('volume', metavar='<volume>',
diff --git a/cinderclient/v2/volumes.py b/cinderclient/v2/volumes.py
index 3fdf986..76bf293 100644
--- a/cinderclient/v2/volumes.py
+++ b/cinderclient/v2/volumes.py
@@ -139,9 +139,9 @@ class Volume(base.Resource):
         """
         self.manager.extend(self, new_size)
 
-    def migrate_volume(self, host, force_host_copy):
+    def migrate_volume(self, host, force_host_copy, lock_volume):
         """Migrate the volume to a new host."""
-        self.manager.migrate_volume(self, host, force_host_copy)
+        self.manager.migrate_volume(self, host, force_host_copy, lock_volume)
 
     def retype(self, volume_type, policy):
         """Change a volume's type."""
@@ -554,16 +554,19 @@ class VolumeManager(base.ManagerWithFind):
         """
         return self._get("/volumes/%s/encryption" % volume_id)._info
 
-    def migrate_volume(self, volume, host, force_host_copy):
+    def migrate_volume(self, volume, host, force_host_copy, lock_volume):
         """Migrate volume to new host.
 
         :param volume: The :class:`Volume` to migrate
         :param host: The destination host
         :param force_host_copy: Skip driver optimizations
+        :param lock_volume: Lock the volume and guarantee the migration
+                            to finish
         """
         return self._action('os-migrate_volume',
                             volume,
-                            {'host': host, 'force_host_copy': force_host_copy})
+                            {'host': host, 'force_host_copy': force_host_copy,
+                             'lock_volume': lock_volume})
 
     def migrate_volume_completion(self, old_volume, new_volume, error):
         """Complete the migration from the old volume to the temp new one.