openstack/manila
Code Issues Proposed changes

HNAS: Fix concurrency creating/deleting snapshots

Browse Source 
When attempting to create or delete empty folders in HNAS,
respective to snapshot creation and deletion, there can
be context changes due to concurrent requests received by
HNAS.

The result of the failed command is an error that the
operation cannot be completed in the current filesystem if
the context change switched to another EVS. In case the
context switch changed only the filesystem within the same
EVS, the operation may be run in the wrong filesystem,
although, those operations are harmless.

Closes-bug: #1662615
Change-Id: I3439f8ab2dd967a438eac575172e4b96e897b46c
This commit is contained in:
Rodrigo Barbieri 2017-02-08 14:30:21 -02:00
parent 65754aa153
commit 4daea8d7cc
6 changed files with 198 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
manila/exception.py
View File

@ -816,6 +816,14 @@ class HNASSSCIsBusy(ManilaException):
message = _("HNAS SSC is busy and cannot execute the command: %(msg)s")
class HNASSSCContextChange(ManilaException):
message = _("HNAS SSC Context has been changed unexpectedly: %(msg)s")
class HNASDirectoryNotEmpty(ManilaException):
message = _("HNAS Directory is not empty: %(msg)s")
class HNASItemNotFoundException(StorageResourceNotFound):
message = _("HNAS Item Not Found Exception: %(msg)s")

4
manila/share/drivers/hitachi/hnas/driver.py
View File

@ -1138,7 +1138,7 @@ class HitachiHNASDriver(driver.ShareDriver):
snapshot['share']['share_proto'])
self._check_fs_mounted()
self.hnas.check_snapshot(snapshot['provider_location'])
self.hnas.check_directory(snapshot['provider_location'])
export_list = None
if snapshot['share'].get('mount_snapshot_support'):
@ -1260,7 +1260,7 @@ class HitachiHNASDriver(driver.ShareDriver):
'share_id': snapshot['share_id']}
raise exception.ManageInvalidShareSnapshot(reason=msg)
if not self.hnas.check_snapshot(snapshot['provider_location']):
if not self.hnas.check_directory(snapshot['provider_location']):
msg = _("Snapshot %(snap_id)s does not exist in "
"HNAS.") % {'snap_id': hnas_snapshot_id}
raise exception.ManageInvalidShareSnapshot(reason=msg)

54
manila/share/drivers/hitachi/hnas/ssh.py
View File

@ -353,15 +353,39 @@ class HNASSSHBackend(object):
LOG.exception(msg)
raise exception.HNASBackendException(msg=msg)
@mutils.retry(exception=exception.HNASSSCContextChange, wait_random=True,
retries=5)
def create_directory(self, dest_path):
self._locked_selectfs('create', dest_path)
if not self.check_directory(dest_path):
msg = _("Command to create directory %(path)s was run in another "
"filesystem instead of %(fs)s.") % {
'path': dest_path,
'fs': self.fs_name,
}
LOG.warning(msg)
raise exception.HNASSSCContextChange(msg=msg)
@mutils.retry(exception=exception.HNASSSCContextChange, wait_random=True,
retries=5)
def delete_directory(self, path):
self._locked_selectfs('delete', path)
try:
self._locked_selectfs('delete', path)
except exception.HNASDirectoryNotEmpty:
pass
else:
if self.check_directory(path):
msg = _("Command to delete empty directory %(path)s was run in"
" another filesystem instead of %(fs)s.") % {
'path': path,
'fs': self.fs_name,
}
LOG.debug(msg)
raise exception.HNASSSCContextChange(msg=msg)
@mutils.retry(exception=exception.HNASSSCIsBusy, wait_random=True,
retries=5)
def check_snapshot(self, path):
def check_directory(self, path):
command = ['path-to-object-number', '-f', self.fs_name, path]
try:
@ -652,10 +676,16 @@ class HNASSSHBackend(object):
self.evs_id, 'mkdir', '-p', path]
try:
self._execute(command)
except processutils.ProcessExecutionError:
msg = _("Failed to create directory %s.") % path
LOG.exception(msg)
raise exception.HNASBackendException(msg=msg)
except processutils.ProcessExecutionError as e:
if "Current file system invalid: VolumeNotFound" in e.stderr:
msg = _("Command to create directory %s failed due to "
"context change.") % path
LOG.debug(msg)
raise exception.HNASSSCContextChange(msg=msg)
else:
msg = _("Failed to create directory %s.") % path
LOG.exception(msg)
raise exception.HNASBackendException(msg=msg)
if op == 'delete':
command = ['selectfs', self.fs_name, '\n',
@ -665,11 +695,17 @@ class HNASSSHBackend(object):
self._execute(command)
except processutils.ProcessExecutionError as e:
if 'DirectoryNotEmpty' in e.stderr:
LOG.debug("Share %(path)s has more snapshots.",
{'path': path})
elif 'NotFound' in e.stderr:
msg = _("Share %s has more snapshots.") % path
LOG.debug(msg)
raise exception.HNASDirectoryNotEmpty(msg=msg)
elif 'cannot remove' in e.stderr and 'NotFound' in e.stderr:
LOG.warning(_LW("Attempted to delete path %s but it does "
"not exist."), path)
elif 'Current file system invalid: VolumeNotFound' in e.stderr:
msg = _("Command to delete empty directory %s failed due "
"to context change.") % path
LOG.debug(msg)
raise exception.HNASSSCContextChange(msg=msg)
else:
msg = _("Failed to delete directory %s.") % path
LOG.exception(msg)

28
manila/tests/share/drivers/hitachi/hnas/test_driver.py
View File

@ -235,7 +235,7 @@ class HitachiHNASTestCase(test.TestCase):
self.mock_object(ssh.HNASSSHBackend, "check_quota", mock.Mock())
self.mock_object(ssh.HNASSSHBackend, "check_cifs", mock.Mock())
self.mock_object(ssh.HNASSSHBackend, "check_export", mock.Mock())
self.mock_object(ssh.HNASSSHBackend, 'check_snapshot')
self.mock_object(ssh.HNASSSHBackend, 'check_directory')
@ddt.data('hitachi_hnas_driver_helper', 'hitachi_hnas_evs_id',
'hitachi_hnas_evs_ip', 'hitachi_hnas_ip', 'hitachi_hnas_user')
@ -990,12 +990,12 @@ class HitachiHNASTestCase(test.TestCase):
else:
expected = None
ssh.HNASSSHBackend.check_snapshot.assert_called_once_with(
ssh.HNASSSHBackend.check_directory.assert_called_once_with(
snapshot['provider_location'])
self.assertEqual(expected, result)
def test_manage_existing_snapshot(self):
self.mock_object(ssh.HNASSSHBackend, 'check_snapshot',
self.mock_object(ssh.HNASSSHBackend, 'check_directory',
mock.Mock(return_value=True))
self.mock_object(self._driver, '_ensure_snapshot',
mock.Mock(return_value=[]))
@ -1006,7 +1006,7 @@ class HitachiHNASTestCase(test.TestCase):
out = self._driver.manage_existing_snapshot(manage_snapshot,
{'size': 20})
ssh.HNASSSHBackend.check_snapshot.assert_called_with(
ssh.HNASSSHBackend.check_directory.assert_called_with(
'/snapshots/aa4a7710-f326-41fb-ad18-b4ad587fc87a'
'/snapshot18-05-2106')
self._driver._ensure_snapshot.assert_called_with(
@ -1022,7 +1022,7 @@ class HitachiHNASTestCase(test.TestCase):
'path': '172.24.44.10:/snapshots/'
'3377b015-a695-4a5a-8aa5-9b931b023380'}]
self.mock_object(ssh.HNASSSHBackend, 'check_snapshot',
self.mock_object(ssh.HNASSSHBackend, 'check_directory',
mock.Mock(return_value=True))
self.mock_object(self._driver, '_ensure_snapshot',
mock.Mock(return_value=[], side_effect=exc))
@ -1038,7 +1038,7 @@ class HitachiHNASTestCase(test.TestCase):
snapshot_mount_support_nfs,
{'size': 20, 'export_locations': export_locations})
ssh.HNASSSHBackend.check_snapshot.assert_called_with(
ssh.HNASSSHBackend.check_directory.assert_called_with(
'/snapshots/62125744-fcdd-4f55-a8c1-d1498102f634'
'/3377b015-a695-4a5a-8aa5-9b931b023380')
self._driver._ensure_snapshot.assert_called_with(
@ -1081,7 +1081,7 @@ class HitachiHNASTestCase(test.TestCase):
self.assertTrue(self.mock_log.debug.called)
def test_manage_inexistent_snapshot_exception(self):
self.mock_object(ssh.HNASSSHBackend, 'check_snapshot',
self.mock_object(ssh.HNASSSHBackend, 'check_directory',
mock.Mock(return_value=False))
self.assertRaises(exception.ManageInvalidShareSnapshot,
@ -1120,7 +1120,7 @@ class HitachiHNASTestCase(test.TestCase):
ssh.HNASSSHBackend.tree_clone.assert_called_once_with(
'/'.join(('/snapshots', snap['share_id'], snap['id'])),
'/'.join(('/shares', snap['share_id'])))
ssh.HNASSSHBackend.check_snapshot.assert_called_once_with(
ssh.HNASSSHBackend.check_directory.assert_called_once_with(
snap['provider_location'])
if exc:
@ -1146,7 +1146,7 @@ class HitachiHNASTestCase(test.TestCase):
ssh.HNASSSHBackend.update_nfs_access_rule.assert_called_once_with(
[access1['access_to'] + '(ro)', access2['access_to'] + '(ro)'],
snapshot_id=snapshot_nfs['id'])
ssh.HNASSSHBackend.check_snapshot.assert_called_once_with(
ssh.HNASSSHBackend.check_directory.assert_called_once_with(
snapshot_nfs['provider_location'])
self.assertTrue(self.mock_log.debug.called)
@ -1163,7 +1163,7 @@ class HitachiHNASTestCase(test.TestCase):
ssh.HNASSSHBackend.update_nfs_access_rule.assert_called_once_with(
[], snapshot_id=snapshot_nfs['id'])
ssh.HNASSSHBackend.check_snapshot.assert_called_once_with(
ssh.HNASSSHBackend.check_directory.assert_called_once_with(
snapshot_nfs['provider_location'])
self.assertTrue(self.mock_log.debug.called)
@ -1176,7 +1176,7 @@ class HitachiHNASTestCase(test.TestCase):
self.assertRaises(exception.InvalidSnapshotAccess,
self._driver.snapshot_update_access, 'ctxt',
snapshot_nfs, [access1], [], [])
ssh.HNASSSHBackend.check_snapshot.assert_called_once_with(
ssh.HNASSSHBackend.check_directory.assert_called_once_with(
snapshot_nfs['provider_location'])
def test_cifs_snapshot_update_access_allow(self):
@ -1192,7 +1192,7 @@ class HitachiHNASTestCase(test.TestCase):
ssh.HNASSSHBackend.cifs_allow_access.assert_called_with(
snapshot_cifs['id'], access1['access_to'], 'ar', is_snapshot=True)
ssh.HNASSSHBackend.check_snapshot.assert_called_once_with(
ssh.HNASSSHBackend.check_directory.assert_called_once_with(
snapshot_cifs['provider_location'])
self.assertTrue(self.mock_log.debug.called)
@ -1209,7 +1209,7 @@ class HitachiHNASTestCase(test.TestCase):
ssh.HNASSSHBackend.cifs_deny_access.assert_called_with(
snapshot_cifs['id'], access1['access_to'], is_snapshot=True)
ssh.HNASSSHBackend.check_snapshot.assert_called_once_with(
ssh.HNASSSHBackend.check_directory.assert_called_once_with(
snapshot_cifs['provider_location'])
self.assertTrue(self.mock_log.debug.called)
@ -1241,6 +1241,6 @@ class HitachiHNASTestCase(test.TestCase):
ssh.HNASSSHBackend.cifs_allow_access.assert_called_with(
snapshot_cifs['id'], access2['access_to'].replace('\\', '\\\\'),
'ar', is_snapshot=True)
ssh.HNASSSHBackend.check_snapshot.assert_called_once_with(
ssh.HNASSSHBackend.check_directory.assert_called_once_with(
snapshot_cifs['provider_location'])
self.assertTrue(self.mock_log.debug.called)

139
manila/tests/share/drivers/hitachi/hnas/test_ssh.py
View File

@ -511,6 +511,7 @@ class HNASSSHTestCase(test.TestCase):
'share_id': 'vvol_test',
'host': 'ubuntu@hitachi2#HITACHI2',
}
self.mock_log.debug.reset_mock()
def test_get_stats(self):
fake_list_command = ['df', '-a', '-f', self.fs_name]
@ -811,7 +812,7 @@ class HNASSSHTestCase(test.TestCase):
self._driver_ssh.cifs_deny_access('vvol_test', 'fake_user')
self._driver_ssh._execute.assert_called_with(fake_cifs_deny_command)
self.assertTrue(self.mock_log.debug.called)
self.assertTrue(self.mock_log.warning.called)
def test_cifs_deny_access_backend_exception(self):
fake_cifs_deny_command = ['cifs-saa', 'delete', '--target-label',
@ -963,35 +964,114 @@ class HNASSSHTestCase(test.TestCase):
def test_create_directory(self):
locked_selectfs_args = ['create', '/path']
self.mock_object(ssh.HNASSSHBackend, "_locked_selectfs", mock.Mock())
self.mock_object(ssh.HNASSSHBackend, "_locked_selectfs")
self.mock_object(ssh.HNASSSHBackend, "check_directory",
mock.Mock(return_value=True))
self._driver_ssh.create_directory("/path")
self._driver_ssh._locked_selectfs.assert_called_with(
*locked_selectfs_args)
ssh.HNASSSHBackend.check_directory.assert_called_once_with('/path')
self.assertFalse(self.mock_log.warning.called)
def test_create_directory_context_change_fail(self):
locked_selectfs_args = ['create', '/path']
self.mock_object(time, 'sleep')
self.mock_object(ssh.HNASSSHBackend, "_locked_selectfs")
self.mock_object(ssh.HNASSSHBackend, "check_directory",
mock.Mock(return_value=False))
self.assertRaises(exception.HNASSSCContextChange,
self._driver_ssh.create_directory, "/path")
self._driver_ssh._locked_selectfs.assert_called_with(
*locked_selectfs_args)
ssh.HNASSSHBackend.check_directory.assert_called_with('/path')
self.assertTrue(self.mock_log.warning.called)
def test_create_directory_context_change_success(self):
locked_selectfs_args = ['create', '/path']
self.mock_object(time, 'sleep')
self.mock_object(ssh.HNASSSHBackend, "_locked_selectfs")
self.mock_object(ssh.HNASSSHBackend, "check_directory",
mock.Mock(side_effect=[False, False, True]))
self._driver_ssh.create_directory("/path")
self._driver_ssh._locked_selectfs.assert_called_with(
*locked_selectfs_args)
ssh.HNASSSHBackend.check_directory.assert_called_with('/path')
self.assertTrue(self.mock_log.warning.called)
def test_delete_directory(self):
locked_selectfs_args = ['delete', '/path']
self.mock_object(ssh.HNASSSHBackend, "_locked_selectfs", mock.Mock())
self.mock_object(ssh.HNASSSHBackend, "_locked_selectfs")
self.mock_object(ssh.HNASSSHBackend, "check_directory",
mock.Mock(return_value=False))
self._driver_ssh.delete_directory("/path")
self._driver_ssh._locked_selectfs.assert_called_with(
*locked_selectfs_args)
ssh.HNASSSHBackend.check_directory.assert_called_once_with('/path')
self.assertFalse(self.mock_log.debug.called)
def test_check_snapshot(self):
def test_delete_directory_directory_not_empty(self):
locked_selectfs_args = ['delete', '/path']
self.mock_object(ssh.HNASSSHBackend, "_locked_selectfs", mock.Mock(
side_effect=exception.HNASDirectoryNotEmpty(msg='fake')))
self.mock_object(ssh.HNASSSHBackend, "check_directory")
self._driver_ssh.delete_directory("/path")
self._driver_ssh._locked_selectfs.assert_called_with(
*locked_selectfs_args)
ssh.HNASSSHBackend.check_directory.assert_not_called()
self.assertFalse(self.mock_log.debug.called)
def test_delete_directory_context_change_fail(self):
locked_selectfs_args = ['delete', '/path']
self.mock_object(time, 'sleep')
self.mock_object(ssh.HNASSSHBackend, "_locked_selectfs")
self.mock_object(ssh.HNASSSHBackend, "check_directory",
mock.Mock(return_value=True))
self.assertRaises(exception.HNASSSCContextChange,
self._driver_ssh.delete_directory, "/path")
self._driver_ssh._locked_selectfs.assert_called_with(
*locked_selectfs_args)
ssh.HNASSSHBackend.check_directory.assert_called_with('/path')
self.assertTrue(self.mock_log.debug.called)
def test_delete_directory_context_change_success(self):
locked_selectfs_args = ['delete', '/path']
self.mock_object(time, 'sleep')
self.mock_object(ssh.HNASSSHBackend, "_locked_selectfs")
self.mock_object(ssh.HNASSSHBackend, "check_directory",
mock.Mock(side_effect=[True, True, False]))
self._driver_ssh.delete_directory("/path")
self._driver_ssh._locked_selectfs.assert_called_with(
*locked_selectfs_args)
ssh.HNASSSHBackend.check_directory.assert_called_with('/path')
self.assertTrue(self.mock_log.debug.called)
def test_check_directory(self):
path = ("/snapshots/" + self.snapshot['share_id'] + "/" +
self.snapshot['id'])
check_snap_args = ['path-to-object-number', '-f', self.fs_name, path]
self.mock_object(ssh.HNASSSHBackend, '_execute')
out = self._driver_ssh.check_snapshot(path)
out = self._driver_ssh.check_directory(path)
self.assertTrue(out)
self._driver_ssh._execute.assert_called_with(check_snap_args)
def test_check_snapshot_retry(self):
def test_check_directory_retry(self):
error_msg = ("Unable to run path-to-object-number as "
"path-to-object-number is currently running on volume "
"39.")
@ -1006,7 +1086,7 @@ class HNASSSHTestCase(test.TestCase):
stdout=error_msg), putils.ProcessExecutionError(
stdout=error_msg), 'Object number: 0x45a4']))
out = self._driver_ssh.check_snapshot(path)
out = self._driver_ssh.check_directory(path)
self.assertIs(True, out)
self._driver_ssh._execute.assert_called_with(check_snap_args)
@ -1020,12 +1100,12 @@ class HNASSSHTestCase(test.TestCase):
mock.Mock(side_effect=putils.ProcessExecutionError(
stdout=HNAS_RESULT_check_snap_error)))
out = self._driver_ssh.check_snapshot(path)
out = self._driver_ssh.check_directory(path)
self.assertFalse(out)
self._driver_ssh._execute.assert_called_with(check_snap_args)
def test_check_snapshot_error(self):
def test_check_directory_error(self):
path = "/path/snap1/snapshot07-08-2016"
check_snap_args = ['path-to-object-number', '-f', self.fs_name, path]
@ -1035,7 +1115,7 @@ class HNASSSHTestCase(test.TestCase):
stdout="Internal Server Error.")))
self.assertRaises(exception.HNASBackendException,
self._driver_ssh.check_snapshot, path)
self._driver_ssh.check_directory, path)
self._driver_ssh._execute.assert_called_with(check_snap_args)
@ -1100,7 +1180,7 @@ class HNASSSHTestCase(test.TestCase):
self._driver_ssh.vvol_delete("vvol")
self.assertTrue(self.mock_log.debug.called)
self.assertTrue(self.mock_log.warning.called)
self._driver_ssh._execute.assert_called_with(fake_vvol_delete_command)
def test_vvol_delete_error(self):
@ -1422,14 +1502,31 @@ class HNASSSHTestCase(test.TestCase):
exec_command = ['selectfs', self.fs_name, '\n', 'ssc', '127.0.0.1',
'console-context', '--evs', six.text_type(self.evs_id),
'mkdir', '-p', '/path']
self.mock_object(ssh.HNASSSHBackend, '_execute',
mock.Mock(side_effect=putils.ProcessExecutionError))
self.mock_object(
ssh.HNASSSHBackend, '_execute',
mock.Mock(side_effect=putils.ProcessExecutionError(
stderr="some error")))
self.assertRaises(exception.HNASBackendException,
self._driver_ssh._locked_selectfs, 'create', '/path')
self._driver_ssh._execute.assert_called_with(exec_command)
def test__locked_selectfs_create_operation_context_change(self):
exec_command = ['selectfs', self.fs_name, '\n', 'ssc', '127.0.0.1',
'console-context', '--evs', six.text_type(self.evs_id),
'mkdir', '-p', '/path']
self.mock_object(
ssh.HNASSSHBackend, '_execute',
mock.Mock(side_effect=putils.ProcessExecutionError(
stderr="Current file system invalid: VolumeNotFound")))
self.assertRaises(exception.HNASSSCContextChange,
self._driver_ssh._locked_selectfs, 'create', '/path')
self._driver_ssh._execute.assert_called_with(exec_command)
self.assertTrue(self.mock_log.debug.called)
def test__locked_selectfs_delete_operation_successful(self):
exec_command = ['selectfs', self.fs_name, '\n', 'ssc', '127.0.0.1',
'console-context', '--evs', six.text_type(self.evs_id),
@ -1446,7 +1543,8 @@ class HNASSSHTestCase(test.TestCase):
self.mock_object(ssh.HNASSSHBackend, '_execute', mock.Mock(
side_effect=[putils.ProcessExecutionError(stderr=msg)]))
self._driver_ssh._locked_selectfs('delete', '/path')
self.assertRaises(exception.HNASDirectoryNotEmpty,
self._driver_ssh._locked_selectfs, 'delete', '/path')
self.assertTrue(self.mock_log.debug.called)
@ -1461,7 +1559,7 @@ class HNASSSHTestCase(test.TestCase):
self.assertTrue(self.mock_log.exception.called)
def test__locked_selectfs_delete_not_found(self):
msg = "rmdir: NotFound '/path'"
msg = "rmdir: cannot remove '/path': NotFound"
self.mock_object(ssh.HNASSSHBackend, '_execute', mock.Mock(
side_effect=[putils.ProcessExecutionError(stderr=msg)]))
@ -1469,3 +1567,14 @@ class HNASSSHTestCase(test.TestCase):
self._driver_ssh._locked_selectfs('delete', 'path')
self.assertTrue(self.mock_log.warning.called)
def test__locked_selectfs_delete_context_change(self):
msg = "Current file system invalid: VolumeNotFound"
self.mock_object(ssh.HNASSSHBackend, '_execute', mock.Mock(
side_effect=[putils.ProcessExecutionError(stderr=msg)]))
self.assertRaises(exception.HNASSSCContextChange,
self._driver_ssh._locked_selectfs, 'delete', 'path')
self.assertTrue(self.mock_log.debug.called)

5
releasenotes/notes/bug-1662615-hnas-snapshot-concurrency-2147159ea6b086c5.yaml Normal file
View File

@ -0,0 +1,5 @@
---
fixes:
- Fixed HNAS driver error when creating or deleting
snapshots caused by concurrency in backend.