From fb48a1fe64dfc6f241eaff968b4ba0ed84a39adc Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Fri, 15 Dec 2017 14:27:51 +0200 Subject: [PATCH] SMBFS: fix detecting if a volume is in-use The SMBFS driver along with the remotefs module rely on the volume status field when checking if it's attached. While in most cases this is fine, it will cause issues when backing up volumes. In this situation, the volume status will be 'backing-up', in which case the driver will wrongfully consider that the volume is detached (by not having the 'in-use' state). In particular, this affects the volume snapshot workflow. This change fixes this check, adding a helper method which detects the volume attach status by looking at the volume 'attach_status' field. Change-Id: I154d85cde693c6e77e22a677cae7150be48fce07 Partial-Implements: windows-smb-backup --- .../tests/unit/volume/drivers/test_remotefs.py | 6 ++++-- cinder/tests/unit/windows/test_smbfs.py | 18 +++++++++--------- cinder/volume/drivers/remotefs.py | 14 +++++++++----- cinder/volume/drivers/windows/smbfs.py | 5 ++--- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/test_remotefs.py b/cinder/tests/unit/volume/drivers/test_remotefs.py index 79a1d2e8d71..7daeeaf6c6a 100644 --- a/cinder/tests/unit/volume/drivers/test_remotefs.py +++ b/cinder/tests/unit/volume/drivers/test_remotefs.py @@ -113,7 +113,8 @@ class RemoteFsSnapDriverTestCase(test.TestCase): 'deleting', 'downloading'] if volume_in_use: - self._fake_snapshot.volume.status = 'in-use' + self._fake_snapshot.volume.status = 'backing-up' + self._fake_snapshot.volume.attach_status = 'attached' self._driver._read_info_file.return_value = fake_info @@ -279,7 +280,8 @@ class RemoteFsSnapDriverTestCase(test.TestCase): self._fake_snapshot.id = 'tmp-snap-%s' % self._fake_snapshot.id if volume_in_use: - self._fake_snapshot.volume.status = 'in-use' + self._fake_snapshot.volume.status = 'backing-up' + self._fake_snapshot.volume.attach_status = 'attached' expected_method_called = '_create_snapshot_online' else: self._fake_snapshot.volume.status = 'available' diff --git a/cinder/tests/unit/windows/test_smbfs.py b/cinder/tests/unit/windows/test_smbfs.py index e0dfcdb0648..58c6b709426 100644 --- a/cinder/tests/unit/windows/test_smbfs.py +++ b/cinder/tests/unit/windows/test_smbfs.py @@ -493,10 +493,10 @@ class WindowsSmbFsTestCase(test.TestCase): def test_get_snapshot_info(self): self._test_get_img_info(self._FAKE_VOLUME_PATH) - @ddt.data('in-use', 'available') - def test_create_snapshot(self, volume_status): + @ddt.data('attached', 'detached') + def test_create_snapshot(self, attach_status): snapshot = self._simple_snapshot() - snapshot.volume.status = volume_status + snapshot.volume.attach_status = attach_status self._smbfs_driver._vhdutils.create_differencing_vhd = ( mock.Mock()) @@ -511,7 +511,7 @@ class WindowsSmbFsTestCase(test.TestCase): os.path.basename(self._FAKE_VOLUME_PATH), self._FAKE_SNAPSHOT_PATH) - if volume_status != 'in-use': + if attach_status != 'attached': fake_create_diff.assert_called_once_with(self._FAKE_SNAPSHOT_PATH, self._FAKE_VOLUME_PATH) else: @@ -570,7 +570,7 @@ class WindowsSmbFsTestCase(test.TestCase): @ddt.data({}, {'delete_latest': True}, - {'volume_status': 'available'}, + {'attach_status': 'detached'}, {'snap_info_contains_snap_id': False}) @ddt.unpack @mock.patch.object(remotefs.RemoteFSSnapDriverDistributed, @@ -586,11 +586,11 @@ class WindowsSmbFsTestCase(test.TestCase): mock_local_path_volume_info, mock_get_local_dir, mock_remotefs_snap_delete, - volume_status='in-use', + attach_status='attached', snap_info_contains_snap_id=True, delete_latest=False): snapshot = self._simple_snapshot() - snapshot.volume.status = volume_status + snapshot.volume.attach_status = attach_status fake_snap_file = 'snap_file' fake_snap_parent_path = os.path.join(self._FAKE_MNT_POINT, @@ -612,7 +612,7 @@ class WindowsSmbFsTestCase(test.TestCase): self._smbfs_driver._delete_snapshot(snapshot) - if volume_status != 'in-use': + if attach_status != 'attached': mock_remotefs_snap_delete.assert_called_once_with(snapshot) elif snap_info_contains_snap_id: mock_local_path_volume_info.assert_called_once_with( @@ -639,7 +639,7 @@ class WindowsSmbFsTestCase(test.TestCase): mock_write_info_file.assert_called_once_with(mock_info_path, snap_info) - if volume_status != 'in-use' or not snap_info_contains_snap_id: + if attach_status != 'attached' or not snap_info_contains_snap_id: mock_nova_assisted_snap_del.assert_not_called() mock_write_info_file.assert_not_called() diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index c49434db617..97372876a5c 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -986,6 +986,9 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): return not utils.paths_normcase_equal(active_fpath, base_vol_path) + def _is_volume_attached(self, volume): + return volume.attach_status == fields.VolumeAttachStatus.ATTACHED + def _create_cloned_volume(self, volume, src_vref): LOG.info('Cloning volume %(src)s to volume %(dst)s', {'src': src_vref.id, @@ -1073,10 +1076,10 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): :returns: None """ - LOG.debug('Deleting %(type)s snapshot %(snap)s of volume %(vol)s', {'snap': snapshot.id, 'vol': snapshot.volume.id, - 'type': ('online' if snapshot.volume.status == 'in-use' + 'type': ('online' + if self._is_volume_attached(snapshot.volume) else 'offline')}) volume_status = snapshot.volume.status @@ -1129,7 +1132,7 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): # Find what file has this as its backing file active_file = self.get_active_image_from_info(snapshot.volume) - if volume_status == 'in-use': + if self._is_volume_attached(snapshot.volume): # Online delete context = snapshot._context @@ -1385,7 +1388,8 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): LOG.debug('Creating %(type)s snapshot %(snap)s of volume %(vol)s', {'snap': snapshot.id, 'vol': snapshot.volume.id, - 'type': ('online' if snapshot.volume.status == 'in-use' + 'type': ('online' + if self._is_volume_attached(snapshot.volume) else 'offline')}) status = snapshot.volume.status @@ -1405,7 +1409,7 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): snapshot.volume) new_snap_path = self._get_new_snap_path(snapshot) - if status == 'in-use': + if self._is_volume_attached(snapshot.volume): self._create_snapshot_online(snapshot, backing_filename, new_snap_path) diff --git a/cinder/volume/drivers/windows/smbfs.py b/cinder/volume/drivers/windows/smbfs.py index cd8273d3d73..e873b13f190 100644 --- a/cinder/volume/drivers/windows/smbfs.py +++ b/cinder/volume/drivers/windows/smbfs.py @@ -425,7 +425,7 @@ class WindowsSmbfsDriver(remotefs_drv.RevertToSnapshotMixin, backing_file_name) def _do_create_snapshot(self, snapshot, backing_file, new_snap_path): - if snapshot.volume.status == 'in-use': + if self._is_volume_attached(snapshot.volume): LOG.debug("Snapshot is in-use. Performing Nova " "assisted creation.") return @@ -451,8 +451,7 @@ class WindowsSmbfsDriver(remotefs_drv.RevertToSnapshotMixin, # NOTE(lpetrut): We're slightly diverging from the super class # workflow. The reason is that we cannot query in-use vhd/x images, # nor can we add or remove images from a vhd/x chain in this case. - volume_status = snapshot.volume.status - if volume_status != 'in-use': + if not self._is_volume_attached(snapshot.volume): return super(WindowsSmbfsDriver, self)._delete_snapshot(snapshot) info_path = self._local_path_volume_info(snapshot.volume)