From ec0cda28cd3399a312cc28a58b13a1ff3891fad1 Mon Sep 17 00:00:00 2001
From: Rodrigo Barbieri <rodrigo.barbieri@fit-tecnologia.org.br>
Date: Mon, 28 Nov 2016 15:15:42 -0200
Subject: [PATCH] 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 | 32 +++++++++-
manila/share/manager.py | 20 +++---
manila/share/migration.py | 40 +++---------
manila/tests/share/test_access.py | 51 +++++++++++++++
manila/tests/share/test_manager.py | 31 ++++++----
manila/tests/share/test_migration.py | 62 ++-----------------
...ration-rw-access-fix-7da3365c7b5b90a1.yaml | 5 ++
7 files changed, 132 insertions(+), 109 deletions(-)
create mode 100644 releasenotes/notes/bug-1626523-migration-rw-access-fix-7da3365c7b5b90a1.yaml
diff --git a/manila/share/access.py b/manila/share/access.py
index 6cbbb1b523..ec49f05a4b 100644
--- a/manila/share/access.py
+++ b/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
diff --git a/manila/share/manager.py b/manila/share/manager.py
index 92afc87e2c..d9e4e4bdb2 100644
--- a/manila/share/manager.py
+++ b/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,
- readonly_support, self.driver)
+ self.db.share_instance_update_access_status(
+ 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(
- 'migration_readonly_rules_support')
+ self.db.share_instance_update_access_status(
+ context, src_share_instance['id'],
+ constants.STATUS_OUT_OF_SYNC)
- helper.change_to_read_only(src_share_instance, share_server,
- readonly_support, self.driver)
+ self.access_helper.update_access_rules(
+ context, src_share_instance['id'], share_server=share_server)
try:
dest_share_instance = helper.create_instance_and_wait(
diff --git a/manila/share/migration.py b/manila/share/migration.py
index e45cc65190..68ebddf930 100644
--- a/manila/share/migration.py
+++ b/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(
diff --git a/manila/tests/share/test_access.py b/manila/tests/share/test_access.py
index 54c56d8df7..b3c0f84ae2 100644
--- a/manila/tests/share/test_access.py
+++ b/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)
diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py
index ab917bb901..39fe887cc1 100644
--- a/manila/tests/share/test_manager.py
+++ b/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,
- 'change_to_read_only')
+ self.mock_object(self.share_manager.db,
+ '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'])
-
- migration_api.ShareMigrationHelper.change_to_read_only.\
- assert_called_once_with(instance, server, True,
- self.share_manager.driver)
+ (self.share_manager.db.share_instance_update_access_status.
+ assert_called_once_with(self.context, instance['id'],
+ constants.STATUS_OUT_OF_SYNC))
+ (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)
diff --git a/manila/tests/share/test_migration.py b/manila/tests/share/test_migration.py
index 0429f09db4..5ef08493da 100644
--- a/manila/tests/share/test_migration.py
+++ b/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)
diff --git a/releasenotes/notes/bug-1626523-migration-rw-access-fix-7da3365c7b5b90a1.yaml b/releasenotes/notes/bug-1626523-migration-rw-access-fix-7da3365c7b5b90a1.yaml
new file mode 100644
index 0000000000..a76cd2729a
--- /dev/null
+++ b/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.
+