openstack
/
cinderlib
Code Issues Proposed changes

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
This commit is contained in:
Gorka Eguileor 2019-03-12 14:36:50 +01:00
parent f9dab3d6fb
commit 4219b90330
6 changed files with 156 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
cinderlib/cinderlib.py
View File

@ -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):

56
cinderlib/objects.py
View File

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

10
cinderlib/tests/unit/objects/test_snapshot.py
View File

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

76
cinderlib/tests/unit/objects/test_volume.py
View File

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

3
cinderlib/tests/unit/test_cinderlib.py
View File

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

31
cinderlib/utils.py Normal file
View File

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