From 839ba2301854fa2cf5357d38e039e4d6f28eecbe Mon Sep 17 00:00:00 2001 From: Valeriy Ponomaryov Date: Mon, 1 Aug 2016 17:05:01 +0300 Subject: [PATCH] [ZFSonLinux] Fix replicated snapshot deletion error Commit [1] implemented bug when deletion of replicated snapshots may fail if active replica deleted not as last among all snapshot replicas. It was caused by deletion of private data of snapshot, only by active replica. So, make that data be deleted only in the end of driver method that deletes replicated snapshots. [1] If240ecde5676c334d39faaccd5703e93544aaa06 Change-Id: Ic2b7b7daf0ba5db8eb7166969413b590b27effb5 Closes-Bug: #1607765 --- manila/share/drivers/zfsonlinux/driver.py | 5 ++- .../share/drivers/zfsonlinux/test_driver.py | 45 ++++++++++++------- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/manila/share/drivers/zfsonlinux/driver.py b/manila/share/drivers/zfsonlinux/driver.py index 9ff8547218..e77a4e9e4c 100644 --- a/manila/share/drivers/zfsonlinux/driver.py +++ b/manila/share/drivers/zfsonlinux/driver.py @@ -498,7 +498,8 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver): @ensure_share_server_not_provided def delete_snapshot(self, context, snapshot, share_server=None): """Is called to remove a snapshot.""" - return self._delete_snapshot(context, snapshot) + self._delete_snapshot(context, snapshot) + self.private_storage.delete(snapshot['snapshot_id']) def _get_saved_snapshot_name(self, snapshot_instance): snapshot_tag = self.private_storage.get( @@ -521,7 +522,6 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver): _LW("Snapshot with '%(id)s' ID and '%(name)s' NAME is " "absent on backend. Nothing has been deleted."), {'id': snapshot['id'], 'name': snapshot_name}) - self.private_storage.delete(snapshot['snapshot_id']) @ensure_share_server_not_provided def create_share_from_snapshot(self, context, share, snapshot, @@ -1252,6 +1252,7 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver): replica_snapshots_dict[replica_snapshot['id']]['status'] = ( constants.STATUS_DELETED) + self.private_storage.delete(replica_snapshot['snapshot_id']) return list(replica_snapshots_dict.values()) @ensure_share_server_not_provided diff --git a/manila/tests/share/drivers/zfsonlinux/test_driver.py b/manila/tests/share/drivers/zfsonlinux/test_driver.py index 310bdbe14f..445ae03164 100644 --- a/manila/tests/share/drivers/zfsonlinux/test_driver.py +++ b/manila/tests/share/drivers/zfsonlinux/test_driver.py @@ -673,8 +673,17 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): self.driver.private_storage.update( snapshot['share_instance_id'], {'dataset_name': dataset_name}) + self.assertEqual( + snap_tag, + self.driver.private_storage.get( + snapshot['snapshot_id'], 'snapshot_tag')) + self.driver.delete_snapshot(context, snapshot, share_server=None) + self.assertIsNone( + self.driver.private_storage.get( + snapshot['snapshot_id'], 'snapshot_tag')) + self.assertEqual(0, zfs_driver.LOG.warning.call_count) self.driver.zfs.assert_called_once_with( 'list', '-r', '-t', 'snapshot', snap_name) @@ -1924,6 +1933,8 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): old_repl_snapshot_tag = ( self.driver._get_replication_snapshot_prefix( active_replica) + 'foo') + new_repl_snapshot_tag = 'foo_snapshot_tag' + dataset_name = 'some_dataset_name' self.driver.private_storage.update( active_replica['id'], {'dataset_name': src_dataset_name, @@ -1933,20 +1944,17 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): for replica in (replica, second_replica): self.driver.private_storage.update( replica['id'], - {'dataset_name': 'some_dataset_name', + {'dataset_name': dataset_name, 'ssh_cmd': 'fake_ssh_cmd'} ) self.driver.private_storage.update( snapshot_instances[0]['snapshot_id'], - {'snapshot_tag': 'foo_snapshot_tag'} + {'snapshot_tag': new_repl_snapshot_tag} ) snap_name = 'fake_snap_name' - self.mock_object(self.driver, '_delete_snapshot') self.mock_object( - self.driver, '_get_saved_snapshot_name', - mock.Mock(return_value=snap_name)) - self.mock_object(self.driver, 'execute_with_retry') + self.driver, 'zfs', mock.Mock(return_value=['out', 'err'])) self.mock_object( self.driver, 'execute', mock.Mock(side_effect=[ ('a', 'b'), @@ -1957,31 +1965,36 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): self.driver, 'parse_zfs_answer', mock.Mock(side_effect=[ ({'NAME': 'foo'}, {'NAME': snap_name}), ({'NAME': 'bar'}, {'NAME': snap_name}), + [], ])) expected = sorted([ {'id': si['id'], 'status': 'deleted'} for si in snapshot_instances ], key=lambda item: item['id']) + self.assertEqual( + new_repl_snapshot_tag, + self.driver.private_storage.get( + snapshot_instances[0]['snapshot_id'], 'snapshot_tag')) + result = self.driver.delete_replicated_snapshot( 'fake_context', replica_list, snapshot_instances) - self.driver._get_saved_snapshot_name.assert_has_calls([ - mock.call(si) for si in snapshot_instances - ]) - self.driver._delete_snapshot.assert_called_once_with( - 'fake_context', active_snapshot_instance) + self.assertIsNone( + self.driver.private_storage.get( + snapshot_instances[0]['snapshot_id'], 'snapshot_tag')) + self.driver.execute.assert_has_calls([ mock.call('ssh', 'fake_ssh_cmd', 'sudo', 'zfs', 'list', '-r', '-t', - 'snapshot', snap_name) for i in (0, 1) - ]) - self.driver.execute_with_retry.assert_has_calls([ - mock.call('ssh', 'fake_ssh_cmd', 'sudo', 'zfs', 'destroy', - '-f', snap_name) for i in (0, 1) + 'snapshot', dataset_name + '@' + new_repl_snapshot_tag) + for i in (0, 1) ]) self.assertIsInstance(result, list) self.assertEqual(3, len(result)) self.assertEqual(expected, sorted(result, key=lambda item: item['id'])) + self.driver.parse_zfs_answer.assert_has_calls([ + mock.call('out'), + ]) @ddt.data( ({'NAME': 'fake'}, zfs_driver.constants.STATUS_ERROR),