Merge "[ZFSonLinux] Fix replicated snapshot deletion error"

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

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),