diff --git a/manila/exception.py b/manila/exception.py index b0a537dc64..82bdf6cac6 100644 --- a/manila/exception.py +++ b/manila/exception.py @@ -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") diff --git a/manila/share/drivers/hitachi/hnas/driver.py b/manila/share/drivers/hitachi/hnas/driver.py index 49e01622b7..02e9f76291 100644 --- a/manila/share/drivers/hitachi/hnas/driver.py +++ b/manila/share/drivers/hitachi/hnas/driver.py @@ -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) diff --git a/manila/share/drivers/hitachi/hnas/ssh.py b/manila/share/drivers/hitachi/hnas/ssh.py index b6d931880a..082272ff01 100644 --- a/manila/share/drivers/hitachi/hnas/ssh.py +++ b/manila/share/drivers/hitachi/hnas/ssh.py @@ -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) diff --git a/manila/tests/share/drivers/hitachi/hnas/test_driver.py b/manila/tests/share/drivers/hitachi/hnas/test_driver.py index 19dcc3e3ad..2a38fcecb3 100644 --- a/manila/tests/share/drivers/hitachi/hnas/test_driver.py +++ b/manila/tests/share/drivers/hitachi/hnas/test_driver.py @@ -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) diff --git a/manila/tests/share/drivers/hitachi/hnas/test_ssh.py b/manila/tests/share/drivers/hitachi/hnas/test_ssh.py index 4526267d29..8312a82452 100644 --- a/manila/tests/share/drivers/hitachi/hnas/test_ssh.py +++ b/manila/tests/share/drivers/hitachi/hnas/test_ssh.py @@ -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) diff --git a/releasenotes/notes/bug-1662615-hnas-snapshot-concurrency-2147159ea6b086c5.yaml b/releasenotes/notes/bug-1662615-hnas-snapshot-concurrency-2147159ea6b086c5.yaml new file mode 100644 index 0000000000..a6e0f315f9 --- /dev/null +++ b/releasenotes/notes/bug-1662615-hnas-snapshot-concurrency-2147159ea6b086c5.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - Fixed HNAS driver error when creating or deleting + snapshots caused by concurrency in backend. +