Implement Share Migration Ocata improvements

- Added 'preserve-snapshots' parameter
- Driver-assisted parameters are now mandatory (thus
compatibility with previous versions is broken)
- Updated parameter descriptions

Change-Id: Icfdefdee88040464d19ab95dba50b88c8f90a1d3
Depends-On: I764b389816319ed0ac5178cadbf809cb632035b4
Implements: blueprint ocata-migration-improvements

contrib/ci/post_test_hook.sh

@@ -63,6 +63,9 @@ iniset $MANILACLIENT_CONF DEFAULT suppress_errors_in_cleanup $SUPPRESS_ERRORS
 # Set access type usage specific to dummy driver that we are using in CI
 iniset $MANILACLIENT_CONF DEFAULT access_types_mapping "nfs:ip,cifs:user"
 
+# Dummy driver is capable of running share migration tests
+iniset $MANILACLIENT_CONF DEFAULT run_migration_tests "True"
+
 # Create share network and use it for functional tests if required
 USE_SHARE_NETWORK=$(trueorfalse True USE_SHARE_NETWORK)
 if [[ ${USE_SHARE_NETWORK} = True ]]; then

manilaclient/api_versions.py

@@ -27,7 +27,7 @@ from manilaclient import utils
 
 LOG = logging.getLogger(__name__)
 
-MAX_VERSION = '2.28'
+MAX_VERSION = '2.29'
 MIN_VERSION = '2.0'
 DEPRECATED_VERSION = '1.0'
 _VERSIONED_METHOD_MAP = {}

manilaclient/common/constants.py

@@ -73,6 +73,12 @@ CG_SNAPSHOT_MEMBERS_SORT_KEY_VALUES = (
     'cgsnapshot_id',
 )
 
+TASK_STATE_MIGRATION_SUCCESS = 'migration_success'
+TASK_STATE_MIGRATION_ERROR = 'migration_error'
+TASK_STATE_MIGRATION_CANCELLED = 'migration_cancelled'
+TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE = 'migration_driver_phase1_done'
+TASK_STATE_DATA_COPYING_COMPLETED = 'data_copying_completed'
+
 EXPERIMENTAL_HTTP_HEADER = 'X-OpenStack-Manila-API-Experimental'
 V1_SERVICE_TYPE = 'share'
 V2_SERVICE_TYPE = 'sharev2'

manilaclient/config.py

@@ -154,6 +154,12 @@ share_opts = [
                      "or not. Disable this feature if used driver doesn't "
                      "support it or when autodeletion of share servers "
                      "is enabled."),
+    cfg.BoolOpt("run_migration_tests",
+                default=False,
+                help="Defines whether to run share migration tests or not. "
+                     "Disable this feature if there is no more than one "
+                     "storage pool being tested or if used driver does not "
+                     "support it."),
 ]
 
 # 2. Generate config

manilaclient/tests/functional/client.py

@@ -23,6 +23,7 @@ from tempest.lib.cli import output_parser
 from tempest.lib.common.utils import data_utils
 from tempest.lib import exceptions as tempest_lib_exc
 
+from manilaclient.common import constants
 from manilaclient import config
 from manilaclient.tests.functional import exceptions
 from manilaclient.tests.functional import utils
@@ -713,6 +714,36 @@ class ManilaCLIClient(base.CLIClient):
                         "build_timeout": self.build_timeout})
                 raise tempest_lib_exc.TimeoutException(message)
 
+    def wait_for_migration_task_state(self, share_id, dest_host,
+                                      task_state_to_wait, microversion=None):
+        """Waits for a share to migrate to a certain host."""
+        statuses = ((task_state_to_wait,)
+                    if not isinstance(task_state_to_wait, (tuple, list, set))
+                    else task_state_to_wait)
+        share = self.get_share(share_id, microversion=microversion)
+        start = int(time.time())
+        while share['task_state'] not in statuses:
+            time.sleep(self.build_interval)
+            share = self.get_share(share_id, microversion=microversion)
+            if share['task_state'] in statuses:
+                break
+            elif share['task_state'] == constants.TASK_STATE_MIGRATION_ERROR:
+                raise exceptions.ShareMigrationException(
+                    share_id=share['id'], src=share['host'], dest=dest_host)
+            elif int(time.time()) - start >= self.build_timeout:
+                message = ('Share %(share_id)s failed to reach a status in'
+                           '%(status)s when migrating from host %(src)s to '
+                           'host %(dest)s within the required time '
+                           '%(timeout)s.' % {
+                               'src': share['host'],
+                               'dest': dest_host,
+                               'share_id': share['id'],
+                               'timeout': self.build_timeout,
+                               'status': six.text_type(statuses),
+                           })
+                raise tempest_lib_exc.TimeoutException(message)
+        return share
+
     @not_found_wrapper
     def _set_share_metadata(self, share, data, update_all=False,
                             microversion=None):
@@ -981,6 +1012,47 @@ class ManilaCLIClient(base.CLIClient):
             'share': share_id,
         }, microversion=version)
 
+    def migration_start(self, share_id, dest_host, writable, nondisruptive,
+                        preserve_metadata, preserve_snapshots,
+                        force_host_assisted_migration, new_share_network=None,
+                        new_share_type=None):
+        cmd = ('migration-start %(share)s %(host)s '
+               '--writable %(writable)s --nondisruptive %(nondisruptive)s '
+               '--preserve-metadata %(preserve_metadata)s '
+               '--preserve-snapshots %(preserve_snapshots)s') % {
+            'share': share_id,
+            'host': dest_host,
+            'writable': writable,
+            'nondisruptive': nondisruptive,
+            'preserve_metadata': preserve_metadata,
+            'preserve_snapshots': preserve_snapshots,
+        }
+        if force_host_assisted_migration:
+            cmd += (' --force-host-assisted-migration %s'
+                    % force_host_assisted_migration)
+        if new_share_network:
+            cmd += ' --new-share-network %s' % new_share_network
+        if new_share_type:
+            cmd += ' --new-share-type %s' % new_share_type
+        return self.manila(cmd)
+
+    def migration_complete(self, share_id):
+        return self.manila('migration-complete %s' % share_id)
+
+    def migration_cancel(self, share_id):
+        return self.manila('migration-cancel %s' % share_id)
+
+    def migration_get_progress(self, share_id):
+        result = self.manila('migration-get-progress %s' % share_id)
+        return output_parser.details(result)
+
+    def pool_list(self, detail=False):
+        cmd = 'pool-list'
+        if detail:
+            cmd += ' --detail'
+        response = self.manila(cmd)
+        return output_parser.listing(response)
+
     def create_security_service(self, type='ldap', name=None, description=None,
                                 dns_ip=None, server=None, domain=None,
                                 user=None, password=None, microversion=None):

manilaclient/tests/functional/exceptions.py

@@ -54,3 +54,8 @@ class AccessRuleCreateErrorException(exceptions.TempestException):
 
 class AccessRuleDeleteErrorException(exceptions.TempestException):
     message = "Access rule %(access)s failed to delete and is in ERROR state."
+
+
+class ShareMigrationException(exceptions.TempestException):
+    message = ("Share %(share_id)s failed to migrate from "
+               "host %(src)s to host %(dest)s.")

manilaclient/tests/functional/test_shares.py

@@ -14,9 +14,11 @@
 #    under the License.
 
 import ddt
+import testtools
 
 from tempest.lib.common.utils import data_utils
 
+from manilaclient.common import constants
 from manilaclient import config
 from manilaclient.tests.functional import base
 from manilaclient.tests.functional import utils
@@ -99,20 +101,118 @@ class SharesTestMigration(base.BaseTestCase):
     def setUpClass(cls):
         super(SharesTestMigration, cls).setUpClass()
 
-        cls.share = cls.create_share(
+        cls.old_type = cls.create_share_type(
-            share_protocol='nfs',
+            data_utils.rand_name('test_share_type'),
-            size=1,
+            driver_handles_share_servers=True)
-            name=data_utils.rand_name('autotest_share_name'),
+        cls.new_type = cls.create_share_type(
-            client=cls.get_user_client(),
+            data_utils.rand_name('test_share_type'),
-            cleanup_in_class=True)
+            driver_handles_share_servers=True)
+        cls.error_type = cls.create_share_type(
+            data_utils.rand_name('test_share_type'),
+            driver_handles_share_servers=True,
+            extra_specs={'cause_error': 'no_valid_host'})
+
+        cls.old_share_net = cls.get_user_client().get_share_network(
+            cls.get_user_client().share_network)
+        cls.new_share_net = cls.create_share_network(
+            neutron_net_id=cls.old_share_net['neutron_net_id'],
+            neutron_subnet_id=cls.old_share_net['neutron_subnet_id'])
 
     @utils.skip_if_microversion_not_supported('2.22')
     @ddt.data('migration_error', 'migration_success', 'None')
     def test_reset_task_state(self, state):
-        self.admin_client.reset_task_state(self.share['id'], state)
+
-        share = self.admin_client.get_share(self.share['id'])
+        share = self.create_share(
+            share_protocol='nfs',
+            size=1,
+            name=data_utils.rand_name('autotest_share_name'),
+            client=self.get_user_client(),
+            share_type=self.old_type['ID'],
+            share_network=self.old_share_net['id'],
+            wait_for_creation=True)
+        share = self.user_client.get_share(share['id'])
+
+        self.admin_client.reset_task_state(share['id'], state)
+        share = self.user_client.get_share(share['id'])
         self.assertEqual(state, share['task_state'])
 
+    @utils.skip_if_microversion_not_supported('2.29')
+    @testtools.skipUnless(
+        CONF.run_migration_tests, 'Share migration tests are disabled.')
+    @ddt.data('cancel', 'success', 'error')
+    def test_full_migration(self, test_type):
+        # We are testing with DHSS=True only because it allows us to specify
+        # new_share_network.
+
+        share = self.create_share(
+            share_protocol='nfs',
+            size=1,
+            name=data_utils.rand_name('autotest_share_name'),
+            client=self.get_user_client(),
+            share_type=self.old_type['ID'],
+            share_network=self.old_share_net['id'],
+            wait_for_creation=True)
+        share = self.user_client.get_share(share['id'])
+
+        pools = self.admin_client.pool_list(detail=True)
+
+        dest_pool = utils.choose_matching_backend(
+            share, pools, self.new_type)
+
+        self.assertIsNotNone(dest_pool)
+
+        source_pool = share['host']
+
+        new_type = self.new_type
+        if test_type == 'error':
+            statuses = constants.TASK_STATE_MIGRATION_ERROR
+            new_type = self.error_type
+        else:
+            statuses = (constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE,
+                        constants.TASK_STATE_DATA_COPYING_COMPLETED)
+
+        self.admin_client.migration_start(
+            share['id'], dest_pool, writable=True, nondisruptive=False,
+            preserve_metadata=True, preserve_snapshots=True,
+            force_host_assisted_migration=False,
+            new_share_network=self.new_share_net['id'],
+            new_share_type=new_type['ID'])
+
+        share = self.admin_client.wait_for_migration_task_state(
+            share['id'], dest_pool, statuses)
+
+        progress = self.admin_client.migration_get_progress(share['id'])
+        self.assertEqual('100', progress['total_progress'])
+
+        self.assertEqual(source_pool, share['host'])
+        self.assertEqual(self.old_type['ID'], share['share_type'])
+        self.assertEqual(self.old_share_net['id'], share['share_network_id'])
+
+        if test_type == 'error':
+            self.assertEqual(statuses, progress['task_state'])
+        else:
+            if test_type == 'success':
+                self.admin_client.migration_complete(share['id'])
+                statuses = constants.TASK_STATE_MIGRATION_SUCCESS
+            elif test_type == 'cancel':
+                self.admin_client.migration_cancel(share['id'])
+                statuses = constants.TASK_STATE_MIGRATION_CANCELLED
+
+            share = self.admin_client.wait_for_migration_task_state(
+                share['id'], dest_pool, statuses)
+            progress = self.admin_client.migration_get_progress(share['id'])
+            self.assertEqual(statuses, progress['task_state'])
+            if test_type == 'success':
+                self.assertEqual(dest_pool, share['host'])
+                self.assertEqual(new_type['ID'], share['share_type'])
+                self.assertEqual(self.new_share_net['id'],
+                                 share['share_network_id'])
+            else:
+                self.assertEqual(source_pool, share['host'])
+                self.assertEqual(self.old_type['ID'], share['share_type'])
+                self.assertEqual(self.old_share_net['id'],
+                                 share['share_network_id'])
+
 
 class NFSSharesReadWriteTest(SharesReadWriteBase):
     protocol = 'nfs'

manilaclient/tests/functional/utils.py

@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import ast
 import six
 from tempest.lib.cli import output_parser
 import testtools
@@ -117,3 +118,23 @@ def skip_if_microversion_not_supported(microversion):
                   "allowed to be used by configuration." % microversion)
         return testtools.skip(reason)
     return lambda f: f
+
+
+def choose_matching_backend(share, pools, share_type):
+    extra_specs = {}
+
+    # convert extra-specs in provided type to dict format
+    pair = [x.strip() for x in share_type['required_extra_specs'].split(':')]
+    if len(pair) == 2:
+        value = (True if six.text_type(pair[1]).lower() == 'true'
+                 else False if six.text_type(pair[1]).lower() == 'false'
+                 else pair[1])
+        extra_specs[pair[0]] = value
+
+    selected_pool = next(
+        (x for x in pools if (x['Name'] != share['host'] and all(
+            y in ast.literal_eval(x['Capabilities']).items() for y in
+            extra_specs.items()))),
+        None)
+
+    return selected_pool['Name']

manilaclient/tests/unit/v2/test_shares.py

@@ -554,20 +554,24 @@ class SharesTest(utils.TestCase):
     def test_migration_start(self):
         share = "fake_share"
         host = "fake_host"
-        version = api_versions.APIVersion('2.22')
+        version = api_versions.APIVersion('2.29')
         manager = shares.ShareManager(
             api=fakes.FakeClient(api_version=version))
 
         with mock.patch.object(manager, "_action",
                                mock.Mock(return_value="fake")):
-            result = manager.migration_start(share, host, True)
+            result = manager.migration_start(
+                share, host, force_host_assisted_migration=True,
+                preserve_metadata=True, writable=True, nondisruptive=True,
+                preserve_snapshots=True)
             manager._action.assert_called_once_with(
                 'migration_start', share, {
                     "host": host,
                     "force_host_assisted_migration": True,
                     "preserve_metadata": True,
                     "writable": True,
-                    "nondisruptive": False,
+                    "nondisruptive": True,
+                    "preserve_snapshots": True,
                     "new_share_network_id": None,
                     "new_share_type_id": None,
                 })

manilaclient/tests/unit/v2/test_shell.py

@@ -2076,10 +2076,10 @@ class ShellTest(test_utils.TestCase):
                            body=expected)
 
     def test_migration_start(self):
-        command = ("migration-start --preserve-metadata False --writable False"
+        command = ("migration-start --force-host-assisted-migration True "
-                   " --force-host-assisted-migration True "
+                   "--new-share-network 1111 --new-share-type 1 1234 "
-                   "--non-disruptive True --new-share-network 1111 "
+                   "host@backend#pool --writable False --nondisruptive True "
-                   "--new-share-type 1 1234 host@backend#pool")
+                   "--preserve-metadata False --preserve-snapshots True")
         self.run_command(command)
         expected = {'migration_start': {
             'host': 'host@backend#pool',
@@ -2087,6 +2087,7 @@ class ShellTest(test_utils.TestCase):
             'preserve-metadata': 'False',
             'writable': 'False',
             'nondisruptive': 'True',
+            'preserve_snapshots': 'True',
             'new_share_network_id': '1111',
             'new_share_type_id': '1'
         }}

manilaclient/v2/shares.py

@@ -44,14 +44,14 @@ class Share(common_base.Resource):
         self.manager.unmanage(self, **kwargs)
 
     def migration_start(self, host, force_host_assisted_migration,
-                        preserve_metadata=True, writable=True,
+                        preserve_metadata, writable, nondisruptive,
-                        nondisruptive=False, new_share_network_id=None,
+                        preserve_snapshots, new_share_network_id=None,
                         new_share_type_id=None):
         """Migrate the share to a new host."""
         self.manager.migration_start(self, host, force_host_assisted_migration,
                                      preserve_metadata, writable,
-                                     nondisruptive, new_share_network_id,
+                                     nondisruptive, preserve_snapshots,
-                                     new_share_type_id)
+                                     new_share_network_id, new_share_type_id)
 
     def migration_complete(self):
         """Complete migration of a share."""
@@ -154,17 +154,18 @@ class ShareManager(base.ManagerWithFind):
         }
         return self._create('/shares', {'share': body}, 'share')
 
-    @api_versions.wraps("2.22")
+    @api_versions.wraps("2.29")
     @api_versions.experimental_api
     def migration_start(self, share, host, force_host_assisted_migration,
-                        preserve_metadata=True, writable=True,
+                        preserve_metadata, writable, nondisruptive,
-                        nondisruptive=False, new_share_network_id=None,
+                        preserve_snapshots, new_share_network_id=None,
                         new_share_type_id=None):
         return self._action(
             "migration_start", share, {
                 "host": host,
                 "force_host_assisted_migration": force_host_assisted_migration,
                 "preserve_metadata": preserve_metadata,
+                "preserve_snapshots": preserve_snapshots,
                 "writable": writable,
                 "nondisruptive": nondisruptive,
                 "new_share_network_id": new_share_network_id,

manilaclient/v2/shell.py

@@ -652,7 +652,7 @@ def do_create(cs, args):
     _print_share(cs, share)
 
 
-@api_versions.wraps("2.22")
+@api_versions.wraps("2.29")
 @cliutils.arg(
     'share',
     metavar='<share>',
@@ -660,56 +660,61 @@ def do_create(cs, args):
 @cliutils.arg(
     'host',
     metavar='<host@backend#pool>',
-    help="Destination host, backend and pool in format 'host@backend#pool'.")
+    help="Destination host where share will be migrated to. Use the "
+         "format 'host@backend#pool'.")
 @cliutils.arg(
     '--force_host_assisted_migration',
     '--force-host-assisted-migration',
     metavar='<True|False>',
     choices=['True', 'False'],
-    required=False,
     action='single_alias',
-    help='Enables or disables generic host-based force-migration, which '
+    required=False,
-         'bypasses driver optimizations. Default=False. '
+    default=False,
-         'Renamed from "force_host_copy" in version 2.22.',
+    help="Enforces the use of the host-assisted migration approach, "
-    default=False)
+         "which bypasses driver optimizations. Default=False.")
 @cliutils.arg(
     '--preserve-metadata',
     '--preserve_metadata',
     action='single_alias',
     metavar='<True|False>',
     choices=['True', 'False'],
-    required=False,
+    required=True,
-    help='Chooses whether migration should be forced to preserve all file '
+    help="Enforces migration to preserve all file metadata when moving its "
-         'metadata when moving its contents. Default=True. '
+         "contents. If set to True, host-assisted migration will not be "
-         'Introduced in version 2.22.',
+         "attempted.")
-    default=True)
+@cliutils.arg(
+    '--preserve-snapshots',
+    '--preserve_snapshots',
+    action='single_alias',
+    metavar='<True|False>',
+    choices=['True', 'False'],
+    required=True,
+    help="Enforces migration of the share snapshots to the destination. If "
+         "set to True, host-assisted migration will not be attempted.")
 @cliutils.arg(
     '--writable',
     metavar='<True|False>',
     choices=['True', 'False'],
-    required=False,
+    required=True,
-    help='Chooses whether migration should be forced to remain writable '
+    help="Enforces migration to keep the share writable while contents are "
-         'while contents are being moved. Default=True. '
+         "being moved. If set to True, host-assisted migration will not be "
-         'Introduced in version 2.22.',
+         "attempted.")
-    default=True)
 @cliutils.arg(
-    '--non-disruptive',
+    '--nondisruptive',
-    '--non_disruptive',
-    action='single_alias',
     metavar='<True|False>',
     choices=['True', 'False'],
-    required=False,
+    required=True,
-    help='Chooses whether migration should only be performed if it is not '
+    help="Enforces migration to be nondisruptive. If set to True, "
-         'disruptive. Default=False. Introduced in version 2.22.',
+         "host-assisted migration will not be attempted.")
-    default=False)
 @cliutils.arg(
     '--new_share_network',
     '--new-share-network',
     metavar='<new_share_network>',
     action='single_alias',
     required=False,
-    help='Specifies a new share network if desired to change. Default=None. '
+    help='Specify the new share network for the share. Do not specify this '
-         'Introduced in version 2.22.',
+         'parameter if the migrating share has to be retained within its '
+         'current share network.',
     default=None)
 @cliutils.arg(
     '--new_share_type',
@@ -717,8 +722,9 @@ def do_create(cs, args):
     metavar='<new_share_type>',
     required=False,
     action='single_alias',
-    help='Specifies a new share type if desired to change. Default=None. '
+    help='Specify the new share type for the share. Do not specify this '
-         'Introduced in version 2.22.',
+         'parameter if the migrating share has to be retained with its '
+         'current share type.',
     default=None)
 def do_migration_start(cs, args):
     """Migrates share to a new host (Admin only, Experimental)."""
@@ -733,8 +739,8 @@ def do_migration_start(cs, args):
         new_share_type_id = share_type.id if share_type else None
     share.migration_start(args.host, args.force_host_assisted_migration,
                           args.preserve_metadata, args.writable,
-                          args.non_disruptive, new_share_net_id,
+                          args.nondisruptive, args.preserve_snapshots,
-                          new_share_type_id)
+                          new_share_net_id, new_share_type_id)
 
 
 @cliutils.arg(

releasenotes/notes/bp-ocata-migration-improvements-f63c5d233856fbee.yaml

@@ -0,0 +1,11 @@
+---
+features:
+  - Added 'preserve-snapshots' to migration-start command.
+upgrades:
+  - Share migration driver-assisted parameters are now mandatory.
+deprecations:
+  - Support for the experimental share migration APIs has been
+    dropped for API microversions prior to 2.29.
+fixes:
+  - Updated descriptions of migration-start parameters.
+