From 4219b903303968a0899587863b46726d5f8427a1 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 12 Mar 2019 14:36:50 +0100 Subject: [PATCH] Fix Volume snapshots and connections on deletion In some cases deleted snapshots and connections are not removed form the cached lists, so we end up with a non existent element in the "snapshots" or "connections" list that should not be there. The reason why this happens is because we are looking for specific instances instead of searching by the ID of the resource, and we could have different instances in the list than the one we are looking for. This patch ensure we are always looking for the elements to remove using the ID. Closes-Bug: #1819709 Change-Id: Ieb14e63c3de2f74f5d239a5174aeda9ee5c8fca1 --- cinderlib/cinderlib.py | 9 +-- cinderlib/objects.py | 56 +++++++++----- cinderlib/tests/unit/objects/test_snapshot.py | 10 ++- cinderlib/tests/unit/objects/test_volume.py | 76 ++++++++++++++++++- cinderlib/tests/unit/test_cinderlib.py | 3 +- cinderlib/utils.py | 31 ++++++++ 6 files changed, 156 insertions(+), 29 deletions(-) create mode 100644 cinderlib/utils.py diff --git a/cinderlib/cinderlib.py b/cinderlib/cinderlib.py index 3e71429..9a4766e 100644 --- a/cinderlib/cinderlib.py +++ b/cinderlib/cinderlib.py @@ -42,6 +42,7 @@ from cinderlib import nos_brick from cinderlib import objects from cinderlib import persistence from cinderlib import serialization +from cinderlib import utils as cinderlib_utils __all__ = ['setup', 'Backend'] @@ -157,11 +158,9 @@ class Backend(object): return vol def _volume_removed(self, volume): - if self._volumes: - for i, vol in enumerate(self._volumes): - if vol.id == volume.id: - del self._volumes[i] - break + i, vol = cinderlib_utils.find_by_id(volume.id, self._volumes) + if vol: + del self._volumes[i] @classmethod def _start_creating_volume(cls, volume): diff --git a/cinderlib/objects.py b/cinderlib/objects.py index 19effd8..c22be18 100644 --- a/cinderlib/objects.py +++ b/cinderlib/objects.py @@ -31,6 +31,7 @@ from oslo_utils import timeutils import six from cinderlib import exception +from cinderlib import utils LOG = logging.getLogger(__name__) @@ -447,6 +448,29 @@ class Volume(NamedObject): finally: self.save() + def _snapshot_removed(self, snapshot): + # The snapshot instance in memory could be out of sync and not be + # identical, so check by ID. + i, snap = utils.find_by_id(snapshot.id, self._snapshots) + if snap: + del self._snapshots[i] + + i, ovo = utils.find_by_id(snapshot.id, self._ovo.snapshots.objects) + if ovo: + del self._ovo.snapshots.objects[i] + + def _connection_removed(self, connection): + # The connection instance in memory could be out of sync and not be + # identical, so check by ID. + i, conn = utils.find_by_id(connection.id, self._connections) + if conn: + del self._connections[i] + + ovo_conns = getattr(self._ovo, CONNECTIONS_OVO_FIELD).objects + i, ovo_conn = utils.find_by_id(connection.id, ovo_conns) + if ovo_conn: + del ovo_conns[i] + def delete(self): if self.snapshots: msg = 'Cannot delete volume %s with snapshots' % self.id @@ -559,10 +583,7 @@ class Volume(NamedObject): def _disconnect(self, connection): self._remove_export() - if self._connections: - self._connections.remove(connection) - ovo_conns = getattr(self._ovo, CONNECTIONS_OVO_FIELD).objects - ovo_conns.remove(connection._ovo) + self._connection_removed(connection) if not self.connections: self._ovo.status = 'available' @@ -747,12 +768,12 @@ class Connection(Object, LazyVolumeAttr): if save: conn.save() # Restore circular reference only if we have all the elements - if conn._volume and conn._volume._connections is not None: - conn._volume._connections.append(conn) - ovo_conns = getattr(conn._volume._ovo, - CONNECTIONS_OVO_FIELD).objects - if ovo not in ovo_conns: - ovo_conns.append(ovo) + if conn._volume: + utils.add_by_id(conn, conn._volume._connections) + + connections = getattr(conn._volume._ovo, + CONNECTIONS_OVO_FIELD).objects + utils.add_by_id(conn._ovo, connections) return conn def _disconnect(self, force=False): @@ -876,10 +897,9 @@ class Snapshot(NamedObject, LazyVolumeAttr): if save: snap.save() # Restore circular reference only if we have all the elements - if snap._volume and snap._volume._snapshots is not None: - snap._volume._snapshots.append(snap) - if ovo not in snap._volume._ovo.snapshots.objects: - snap._volume._ovo.snapshots.objects.append(ovo) + if snap._volume: + utils.add_by_id(snap, snap._volume._snapshots) + utils.add_by_id(snap._ovo, snap._volume._ovo.snapshots.objects) return snap def create(self): @@ -903,12 +923,8 @@ class Snapshot(NamedObject, LazyVolumeAttr): self.status = 'error_deleting' self.save() self._raise_with_resource() - if self._volume is not None and self._volume._snapshots is not None: - try: - self._volume._snapshots.remove(self) - self._volume._ovo.snapshots.objects.remove(self._ovo) - except ValueError: - pass + if self._volume is not None: + self._volume._snapshot_removed(self) def create_volume(self, **new_vol_params): new_vol_params.setdefault('size', self.volume_size) diff --git a/cinderlib/tests/unit/objects/test_snapshot.py b/cinderlib/tests/unit/objects/test_snapshot.py index d92bf2f..bbde8f6 100644 --- a/cinderlib/tests/unit/objects/test_snapshot.py +++ b/cinderlib/tests/unit/objects/test_snapshot.py @@ -73,7 +73,11 @@ class TestSnapshot(base.BaseTest): self.persistence.set_snapshot.assert_called_once_with(self.snap) def test_delete(self): - self.snap.delete() + with mock.patch.object( + self.vol, '_snapshot_removed', + wraps=self.vol._snapshot_removed) as snap_removed_mock: + self.snap.delete() + snap_removed_mock.assert_called_once_with(self.snap) self.backend.driver.delete_snapshot.assert_called_once_with( self.snap._ovo) self.persistence.delete_snapshot.assert_called_once_with(self.snap) @@ -81,13 +85,15 @@ class TestSnapshot(base.BaseTest): self.assertEqual([], self.vol._ovo.snapshots.objects) self.assertEqual('deleted', self.snap.status) - def test_delete_error(self): + @mock.patch('cinderlib.objects.Volume._snapshot_removed') + def test_delete_error(self, snap_removed_mock): self.backend.driver.delete_snapshot.side_effect = exception.NotFound with self.assertRaises(exception.NotFound) as assert_context: self.snap.delete() self.assertEqual(self.snap, assert_context.exception.resource) self.backend.driver.delete_snapshot.assert_called_once_with( self.snap._ovo) + snap_removed_mock.assert_not_called() self.persistence.delete_snapshot.assert_not_called() self.assertEqual([self.snap], self.vol.snapshots) self.assertEqual([self.snap._ovo], self.vol._ovo.snapshots.objects) diff --git a/cinderlib/tests/unit/objects/test_volume.py b/cinderlib/tests/unit/objects/test_volume.py index 4fe3f53..416da8d 100644 --- a/cinderlib/tests/unit/objects/test_volume.py +++ b/cinderlib/tests/unit/objects/test_volume.py @@ -397,13 +397,15 @@ class TestVolume(base.BaseTest): mock_conn._disconnect.assert_called_once_with(mock.sentinel.force) mock_disconnect.assert_called_once_with(mock_conn) + @mock.patch('cinderlib.objects.Volume._connection_removed') @mock.patch('cinderlib.objects.Volume._remove_export') - def test__disconnect(self, mock_remove_export): + def test__disconnect(self, mock_remove_export, mock_conn_removed): vol = objects.Volume(self.backend_name, status='in-use', size=10) vol._disconnect(mock.sentinel.connection) mock_remove_export.assert_called_once_with() + mock_conn_removed.assert_called_once_with(mock.sentinel.connection) self.assertEqual('available', vol.status) self.persistence.set_volume.assert_called_once_with(vol) @@ -426,3 +428,75 @@ class TestVolume(base.BaseTest): mock_remove_export.assert_called_once_with() for c in connections: c.detach.asssert_called_once_with() + + def test__snapshot_removed_not_loaded(self): + vol = objects.Volume(self.backend, + name='vol_name', description='vol_desc', size=10) + vol._snapshots = None + snap = objects.Snapshot(vol) + # Just check it doesn't break + vol._snapshot_removed(snap) + + def test__snapshot_removed_not_present(self): + vol = objects.Volume(self.backend, + name='vol_name', description='vol_desc', size=10) + snap = objects.Snapshot(vol) + snap2 = objects.Snapshot(vol) + vol._snapshots = [snap2] + vol._ovo.snapshots.objects = [snap2._ovo] + # Just check it doesn't break or remove any other snaps + vol._snapshot_removed(snap) + self.assertEqual([snap2], vol._snapshots) + self.assertEqual([snap2._ovo], vol._ovo.snapshots.objects) + + def test__snapshot_removed(self): + vol = objects.Volume(self.backend, + name='vol_name', description='vol_desc', size=10) + snap = objects.Snapshot(vol) + snap2 = objects.Snapshot(vol) + snap_other_instance = objects.Snapshot(vol, id=snap.id, + description='d') + snap_other_instance2 = objects.Snapshot(vol, id=snap.id, + description='e') + vol._snapshots = [snap2, snap_other_instance] + vol._ovo.snapshots.objects = [snap2._ovo, snap_other_instance2._ovo] + # Just check it doesn't break or remove any other snaps + vol._snapshot_removed(snap) + self.assertEqual([snap2], vol._snapshots) + self.assertEqual([snap2._ovo], vol._ovo.snapshots.objects) + + def test__connection_removed_not_loaded(self): + vol = objects.Volume(self.backend, + name='vol_name', description='vol_desc', size=10) + vol._connections = None + conn = objects.Connection(self.backend, connection_info={'conn': {}}) + # Just check it doesn't break + vol._connection_removed(conn) + + def test__connection_removed_not_present(self): + vol = objects.Volume(self.backend, + name='vol_name', description='vol_desc', size=10) + conn = objects.Connection(self.backend, connection_info={'conn': {}}) + conn2 = objects.Connection(self.backend, connection_info={'conn': {}}) + vol._connections = [conn2] + vol._ovo.volume_attachment.objects = [conn2._ovo] + # Just check it doesn't break or remove any other snaps + vol._connection_removed(conn) + self.assertEqual([conn2], vol._connections) + self.assertEqual([conn2._ovo], vol._ovo.volume_attachment.objects) + + def test__connection_removed(self): + vol = objects.Volume(self.backend, size=10) + conn = objects.Connection(self.backend, connection_info={'conn': {}}) + conn2 = objects.Connection(self.backend, connection_info={'conn': {}}) + conn_other_instance = objects.Connection(self.backend, id=conn.id, + connection_info={'conn': {}}) + conn_other_instance2 = objects.Connection(self.backend, id=conn.id, + connection_info={'conn': {}}) + vol._connections = [conn2, conn_other_instance] + vol._ovo.volume_attachment.objects = [conn2._ovo, + conn_other_instance2._ovo] + # Just check it doesn't break or remove any other snaps + vol._connection_removed(conn) + self.assertEqual([conn2], vol._connections) + self.assertEqual([conn2._ovo], vol._ovo.volume_attachment.objects) diff --git a/cinderlib/tests/unit/test_cinderlib.py b/cinderlib/tests/unit/test_cinderlib.py index 7211b2a..afa07d1 100644 --- a/cinderlib/tests/unit/test_cinderlib.py +++ b/cinderlib/tests/unit/test_cinderlib.py @@ -176,7 +176,8 @@ class TestCinderlib(base.BaseTest): mock_vol.return_value.create.assert_called_once_with() def test__volume_removed_no_list(self): - self.backend._volume_removed(mock.sentinel.volume) + vol = cinderlib.objects.Volume(self.backend, size=10) + self.backend._volume_removed(vol) def test__volume_removed(self): vol = cinderlib.objects.Volume(self.backend, size=10) diff --git a/cinderlib/utils.py b/cinderlib/utils.py new file mode 100644 index 0000000..55f37c7 --- /dev/null +++ b/cinderlib/utils.py @@ -0,0 +1,31 @@ +# Copyright (c) 2019, Red Hat, Inc. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +def find_by_id(resource_id, elements): + if elements: + for i, element in enumerate(elements): + if resource_id == element.id: + return i, element + return None, None + + +def add_by_id(resource, elements): + if elements is not None: + i, element = find_by_id(resource.id, elements) + if element: + elements[i] = resource + else: + elements.append(resource)