Fix share writable in host-assisted migration

For drivers that implement update_access always through recovery
mode, access rules previously set to read-only were being reset
to read-write when the Data Service was adding/removing its access
rule.

Fixed it by integrating the logic that casts DB rules to read-only
into access helper class.

Change-Id: Ife35dcdb99dffa2f266b5c2f6a68fe163da7edd3
Closes-bug: #1626523

manila/share/access.py

@@ -100,13 +100,43 @@ class ShareInstanceAccess(object):
                         if rule["id"] in delete_ids]
                     delete_rules = []
 
+        # NOTE(ganso): up to here we are certain of the rules that we are
+        # supposed to pass to drivers. 'rules' variable is used for validating
+        # the refresh mechanism later, according to the 'supposed' rules.
+        driver_rules = rules
+
+        if share_instance['status'] == constants.STATUS_MIGRATING:
+            # NOTE(ganso): If the share instance is the source in a migration,
+            # it should have all its rules cast to read-only.
+
+            readonly_support = self.driver.configuration.safe_get(
+                'migration_readonly_rules_support')
+
+            # NOTE(ganso): If the backend supports read-only rules, then all
+            # rules are cast to read-only here and passed to drivers.
+            if readonly_support:
+                for rule in driver_rules + add_rules:
+                    rule['access_level'] = constants.ACCESS_LEVEL_RO
+                LOG.debug("All access rules of share instance %s are being "
+                          "cast to read-only for migration.",
+                          share_instance['id'])
+            # NOTE(ganso): If the backend does not support read-only rules, we
+            # will remove all access to the share and have only the access
+            # requested by the Data Service for migration as RW.
+            else:
+                LOG.debug("All previously existing access rules of share "
+                          "instance %s are being removed for migration, as "
+                          "driver does not support read-only mode rules.",
+                          share_instance['id'])
+                driver_rules = add_rules
+
         try:
             access_keys = None
             try:
                 access_keys = self.driver.update_access(
                     context,
                     share_instance,
-                    rules,
+                    driver_rules,
                     add_rules=add_rules,
                     delete_rules=delete_rules,
                     share_server=share_server

manila/share/manager.py

@@ -741,11 +741,14 @@ class ShareManager(manager.SchedulerDependentManager):
                 raise exception.ShareMigrationFailed(reason=msg)
 
             if not compatibility.get('writable'):
-                readonly_support = self.driver.configuration.safe_get(
-                    'migration_readonly_rules_support')
 
-                helper.change_to_read_only(src_share_instance, share_server,
+                self.db.share_instance_update_access_status(
-                                           readonly_support, self.driver)
+                    context, src_share_instance['id'],
+                    constants.STATUS_OUT_OF_SYNC)
+
+                self.access_helper.update_access_rules(
+                    context, src_share_instance['id'],
+                    share_server=share_server)
 
             LOG.debug("Initiating driver migration for share %s.",
                       share_ref['id'])
@@ -936,11 +939,12 @@ class ShareManager(manager.SchedulerDependentManager):
         share_server = self._get_share_server(context.elevated(),
                                               src_share_instance)
 
-        readonly_support = self.driver.configuration.safe_get(
+        self.db.share_instance_update_access_status(
-            'migration_readonly_rules_support')
+            context, src_share_instance['id'],
+            constants.STATUS_OUT_OF_SYNC)
 
-        helper.change_to_read_only(src_share_instance, share_server,
+        self.access_helper.update_access_rules(
-                                   readonly_support, self.driver)
+            context, src_share_instance['id'], share_server=share_server)
 
         try:
             dest_share_instance = helper.create_instance_and_wait(

manila/share/migration.py

@@ -132,43 +132,21 @@ class ShareMigrationHelper(object):
                         " migration for share %s."), self.share['id'])
 
     def cleanup_access_rules(self, share_instance, share_server, driver):
+
+        # NOTE(ganso): For the purpose of restoring the access rules, the share
+        # instance status must not be "MIGRATING", else they would be cast to
+        # read-only. We briefly change them to "INACTIVE" so they are restored
+        # and after cleanup finishes, the invoking method will set the status
+        # back to "AVAILABLE".
+        self.db.share_instance_update(self.context, share_instance['id'],
+                                      {'status': constants.STATUS_INACTIVE})
+
         try:
             self.revert_access_rules(share_instance, share_server, driver)
         except Exception:
             LOG.warning(_LW("Failed to cleanup access rules during generic"
                         " migration for share %s."), self.share['id'])
 
-    def change_to_read_only(self, share_instance, share_server,
-                            readonly_support, driver):
-
-        # NOTE(ganso): If the share does not allow readonly mode we
-        # should remove all access rules and prevent any access
-
-        rules = self.db.share_access_get_all_for_instance(
-            self.context, share_instance['id'])
-
-        if len(rules) > 0:
-
-            if readonly_support:
-
-                LOG.debug("Changing all of share %s access rules "
-                          "to read-only.", self.share['id'])
-
-                for rule in rules:
-                    rule['access_level'] = constants.ACCESS_LEVEL_RO
-
-                driver.update_access(self.context, share_instance, rules,
-                                     add_rules=[], delete_rules=[],
-                                     share_server=share_server)
-            else:
-
-                LOG.debug("Removing all access rules for migration of "
-                          "share %s." % self.share['id'])
-
-                driver.update_access(self.context, share_instance, [],
-                                     add_rules=[], delete_rules=rules,
-                                     share_server=share_server)
-
     def revert_access_rules(self, share_instance, share_server, driver):
 
         rules = self.db.share_access_get_all_for_instance(

manila/tests/share/test_access.py

@@ -259,3 +259,54 @@ class ShareInstanceAccessTestCase(test.TestCase):
             mock.call(self.context, share_instance, original_rules,
                       add_rules=[], delete_rules=[], share_server=None)
         ])
+
+    @ddt.data(True, False)
+    def test_update_access_rules_migrating(self, read_only_support):
+
+        def override_conf(conf_name):
+            if conf_name == 'migration_readonly_rules_support':
+                return read_only_support
+
+        rules = []
+        for i in range(2):
+            rules.append(
+                db_utils.create_access(
+                    id='fakeid%s' % i,
+                    share_id=self.share['id'],
+                    access_to='fakeip%s' % i,
+                ))
+        driver_rules = [] if not read_only_support else rules
+        access_rules_status = constants.STATUS_OUT_OF_SYNC
+        share_instance = db_utils.create_share_instance(
+            id='fakeid',
+            status=constants.STATUS_MIGRATING,
+            share_id=self.share['id'],
+            access_rules_status=access_rules_status)
+
+        self.mock_object(db, "share_instance_get", mock.Mock(
+            return_value=share_instance))
+        self.mock_object(db, "share_access_get_all_for_instance",
+                         mock.Mock(return_value=rules))
+        self.mock_object(db, "share_instance_update_access_status",
+                         mock.Mock())
+        self.mock_object(self.driver, "update_access",
+                         mock.Mock(return_value=None))
+        self.mock_object(self.share_access_helper,
+                         "_remove_access_rules", mock.Mock())
+        self.mock_object(self.share_access_helper, "_check_needs_refresh",
+                         mock.Mock(return_value=False))
+        self.mock_object(self.driver.configuration, 'safe_get',
+                         mock.Mock(side_effect=override_conf))
+
+        self.share_access_helper.update_access_rules(
+            self.context, share_instance['id'])
+
+        self.driver.update_access.assert_called_once_with(
+            self.context, share_instance, driver_rules, add_rules=[],
+            delete_rules=[], share_server=None)
+        self.share_access_helper._remove_access_rules.assert_called_once_with(
+            self.context, [], share_instance['id'])
+        self.share_access_helper._check_needs_refresh.assert_called_once_with(
+            self.context, rules, share_instance)
+        db.share_instance_update_access_status.assert_called_with(
+            self.context, share_instance['id'], constants.STATUS_ACTIVE)

manila/tests/share/test_manager.py

@@ -3792,8 +3792,10 @@ class ShareManagerTestCase(test.TestCase):
                          mock.Mock(return_value=server))
         self.mock_object(self.share_manager.db, 'share_instance_update',
                          mock.Mock(return_value=server))
-        self.mock_object(migration_api.ShareMigrationHelper,
+        self.mock_object(self.share_manager.db,
-                         'change_to_read_only')
+                         'share_instance_update_access_status')
+        self.mock_object(self.share_manager.access_helper,
+                         'update_access_rules')
         if exc is None:
             self.mock_object(migration_api.ShareMigrationHelper,
                              'create_instance_and_wait',
@@ -3824,10 +3826,12 @@ class ShareManagerTestCase(test.TestCase):
         self.share_manager.db.share_server_get.assert_called_once_with(
             utils.IsAMatcher(context.RequestContext),
             instance['share_server_id'])
-
+        (self.share_manager.db.share_instance_update_access_status.
-        migration_api.ShareMigrationHelper.change_to_read_only.\
+         assert_called_once_with(self.context, instance['id'],
-            assert_called_once_with(instance, server, True,
+                                 constants.STATUS_OUT_OF_SYNC))
-                                    self.share_manager.driver)
+        (self.share_manager.access_helper.update_access_rules.
+         assert_called_once_with(
+             self.context, instance['id'], share_server=server))
         migration_api.ShareMigrationHelper.create_instance_and_wait.\
             assert_called_once_with(share, 'fake_host', 'fake_net_id',
                                     'fake_az_id', 'fake_type_id')
@@ -3896,10 +3900,12 @@ class ShareManagerTestCase(test.TestCase):
         self.mock_object(
             migration_api.ShareMigrationHelper, 'wait_for_share_server',
             mock.Mock(return_value=dest_server))
-        self.mock_object(
-            migration_api.ShareMigrationHelper, 'change_to_read_only')
         self.mock_object(self.share_manager.driver, 'migration_start')
         self.mock_object(self.share_manager, '_migration_delete_instance')
+        self.mock_object(self.share_manager.db,
+                         'share_instance_update_access_status')
+        self.mock_object(self.share_manager.access_helper,
+                         'update_access_rules')
 
         # run
         if exc:
@@ -3926,12 +3932,15 @@ class ShareManagerTestCase(test.TestCase):
                     {'task_state':
                      constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS})
             ])
+            (self.share_manager.db.share_instance_update_access_status.
+             assert_called_once_with(self.context, src_instance['id'],
+                                     constants.STATUS_OUT_OF_SYNC))
+            (self.share_manager.access_helper.update_access_rules.
+             assert_called_once_with(
+                 self.context, src_instance['id'], share_server=src_server))
             self.share_manager.driver.migration_start.assert_called_once_with(
                 self.context, src_instance, migrating_instance,
                 src_server, dest_server)
-            (migration_api.ShareMigrationHelper.change_to_read_only.
-             assert_called_once_with(src_instance, src_server, True,
-                                     self.share_manager.driver))
 
         self.share_manager.db.share_instance_get.assert_called_once_with(
             self.context, migrating_instance['id'], with_share_data=True)

manila/tests/share/test_migration.py

@@ -248,64 +248,6 @@ class ShareMigrationHelperTestCase(test.TestCase):
         # asserts
         db.share_server_get.assert_called_with(self.context, 'fake_server_id')
 
-    def test_change_to_read_only_with_ro_support(self):
-
-        share_instance = db_utils.create_share_instance(
-            share_id=self.share['id'], status=constants.STATUS_AVAILABLE)
-
-        access = db_utils.create_access(share_id=self.share['id'],
-                                        access_to='fake_ip',
-                                        access_level='rw')
-
-        server = db_utils.create_share_server(share_id=self.share['id'])
-
-        # mocks
-        share_driver = mock.Mock()
-        self.mock_object(share_driver, 'update_access')
-
-        self.mock_object(db, 'share_access_get_all_for_instance',
-                         mock.Mock(return_value=[access]))
-
-        # run
-        self.helper.change_to_read_only(share_instance, server, True,
-                                        share_driver)
-
-        # asserts
-        db.share_access_get_all_for_instance.assert_called_once_with(
-            self.context, share_instance['id'])
-        share_driver.update_access.assert_called_once_with(
-            self.context, share_instance, [access], add_rules=[],
-            delete_rules=[], share_server=server)
-
-    def test_change_to_read_only_without_ro_support(self):
-
-        share_instance = db_utils.create_share_instance(
-            share_id=self.share['id'], status=constants.STATUS_AVAILABLE)
-
-        access = db_utils.create_access(share_id=self.share['id'],
-                                        access_to='fake_ip',
-                                        access_level='rw')
-
-        server = db_utils.create_share_server(share_id=self.share['id'])
-
-        # mocks
-        share_driver = mock.Mock()
-        self.mock_object(share_driver, 'update_access')
-
-        self.mock_object(db, 'share_access_get_all_for_instance',
-                         mock.Mock(return_value=[access]))
-
-        # run
-        self.helper.change_to_read_only(share_instance, server, False,
-                                        share_driver)
-
-        # asserts
-        db.share_access_get_all_for_instance.assert_called_once_with(
-            self.context, share_instance['id'])
-        share_driver.update_access.assert_called_once_with(
-            self.context, share_instance, [], add_rules=[],
-            delete_rules=[access], share_server=server)
-
     def test_revert_access_rules(self):
 
         share_instance = db_utils.create_share_instance(
@@ -396,6 +338,7 @@ class ShareMigrationHelperTestCase(test.TestCase):
         share_driver = mock.Mock()
         self.mock_object(self.helper, 'revert_access_rules',
                          mock.Mock(side_effect=exc))
+        self.mock_object(self.helper.db, 'share_instance_update')
 
         self.mock_object(migration.LOG, 'warning')
 
@@ -406,6 +349,9 @@ class ShareMigrationHelperTestCase(test.TestCase):
         # asserts
         self.helper.revert_access_rules.assert_called_once_with(
             self.share_instance, server, share_driver)
+        self.helper.db.share_instance_update.assert_called_once_with(
+            self.context, self.share_instance['id'],
+            {'status': constants.STATUS_INACTIVE})
 
         if exc:
             self.assertEqual(1, migration.LOG.warning.call_count)

releasenotes/notes/bug-1626523-migration-rw-access-fix-7da3365c7b5b90a1.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - Fixed share remaining with read/write access rules during a
+    host-assisted share migration.
+