From ad62e9dde3fe1ad555f412e8f01680ab5bd7fd52 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 4 Jan 2019 14:23:08 +0800 Subject: [PATCH] QNAP: Fix inconsistent cases while create/manage from snapshot There are two situation may cause the size of share/snapshot managed by manila is inconsistent with the NAS backend. One is to create a share from snapshot. While the other one is to manage an existing snapshot. Change-Id: Iaef8d8cb4be0d8872a2796c0fc69279c14f15a80 Closes-Bug: #1810476 --- manila/share/drivers/qnap/api.py | 3 +- manila/share/drivers/qnap/qnap.py | 27 ++--- manila/tests/share/drivers/qnap/fakes.py | 107 +----------------- manila/tests/share/drivers/qnap/test_api.py | 10 +- manila/tests/share/drivers/qnap/test_qnap.py | 67 +---------- ...napshot-inconsistant-bd628c6e14eeab14.yaml | 5 + 6 files changed, 28 insertions(+), 191 deletions(-) create mode 100644 releasenotes/notes/qnap-fix-share-and-snapshot-inconsistant-bd628c6e14eeab14.yaml diff --git a/manila/share/drivers/qnap/api.py b/manila/share/drivers/qnap/api.py index ef87cb6407..b4e630d0ec 100644 --- a/manila/share/drivers/qnap/api.py +++ b/manila/share/drivers/qnap/api.py @@ -467,13 +467,14 @@ class QnapAPIExecutor(object): raise exception.ShareBackendException(msg=msg) @_connection_checker - def clone_snapshot(self, snapshot_id, new_sharename): + def clone_snapshot(self, snapshot_id, new_sharename, clone_size): """Execute CGI to clone snapshot as share.""" params = { 'func': 'clone_qsnapshot', 'by_vol': '1', 'snapshotID': snapshot_id, 'new_name': new_sharename, + 'clone_size': '{}g'.format(clone_size), 'sid': self.sid, } sanitized_params = self._sanitize_params(params) diff --git a/manila/share/drivers/qnap/qnap.py b/manila/share/drivers/qnap/qnap.py index 3d56c31406..17017ea4ab 100644 --- a/manila/share/drivers/qnap/qnap.py +++ b/manila/share/drivers/qnap/qnap.py @@ -77,6 +77,8 @@ class QnapShareDriver(driver.ShareDriver): 1.0.7 - Add support for QES fw on TDS series NAS model. 1.0.8 - Fix bug, driver should not manage snapshot which does not exist in NAS. + Fix bug, driver should create share from snapshot with + specified size. """ DRIVER_VERSION = '1.0.8' @@ -540,7 +542,8 @@ class QnapShareDriver(driver.ShareDriver): msg = _("Failed to create an unused share name.") raise exception.ShareBackendException(msg=msg) - self.api_executor.clone_snapshot(snapshot_id, create_share_name) + self.api_executor.clone_snapshot(snapshot_id, + create_share_name, share['size']) create_volID = "" created_share = self.api_executor.get_share_info( @@ -553,10 +556,6 @@ class QnapShareDriver(driver.ShareDriver): msg = _("Failed to clone a snapshot in time.") raise exception.ShareBackendException(msg=msg) - snap_share = self.share_api.get( - context, snapshot['share_instance']['share_id']) - LOG.debug('snap_share[size]: %s', snap_share['size']) - thin_provision = self.private_storage.get( snapshot['share_instance_id'], 'thin_provision') compression = self.private_storage.get( @@ -574,19 +573,6 @@ class QnapShareDriver(driver.ShareDriver): 'deduplication': deduplication, 'ssd_cache': ssd_cache}) - if (share['size'] > snap_share['size']): - share_dict = { - 'sharename': create_share_name, - 'old_sharename': create_share_name, - 'new_size': share['size'], - 'thin_provision': thin_provision == 'True', - 'compression': compression == 'True', - 'deduplication': deduplication == 'True', - 'ssd_cache': ssd_cache == 'True', - 'share_proto': share['share_proto'] - } - self.api_executor.edit_share(share_dict) - # Use private_storage to record volume ID and Name created in the NAS. _metadata = { 'volID': create_volID, @@ -930,6 +916,11 @@ class QnapShareDriver(driver.ShareDriver): 'snapshot_id': snapshot_id, } self.private_storage.update(snapshot['id'], _metadata) + parent_size = check_snapshot.find('parent_size') + snap_size_gb = None + if parent_size is not None: + snap_size_gb = math.ceil(float(parent_size.text) / units.Gi) + return {'size': snap_size_gb} def unmanage_snapshot(self, snapshot): """Remove the specified snapshot from Manila management.""" diff --git a/manila/tests/share/drivers/qnap/fakes.py b/manila/tests/share/drivers/qnap/fakes.py index 3522c3430a..b93dcc81e8 100644 --- a/manila/tests/share/drivers/qnap/fakes.py +++ b/manila/tests/share/drivers/qnap/fakes.py @@ -173,62 +173,6 @@ FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TES_ES_2_2_0 = """ """ -FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_TS_4_0_0 = """ - - - - - - - - - """ - -FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_TS_4_3_0 = """ - - - - - - - - - """ - -FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_ES_1_1_1 = """ - - - - - - - - - """ - -FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_ES_2_1_0 = """ - - - - - - - - - """ - -FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_ES_2_2_0 = """ - - - - - - - - - """ - - FAKE_RES_DETAIL_DATA_GETBASIC_INFO_ERROR = """ @@ -276,6 +220,7 @@ FAKE_RES_DETAIL_DATA_SNAPSHOT = """ + 10 @@ -681,56 +626,6 @@ class FakeGetBasicInfoResponseTesEs_2_2_0(object): return FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TES_ES_2_2_0 -class FakeGetBasicInfoResponseTdsTs_4_0_0(object): - """Fake GetBasicInfoTS response from TS nas.""" - - status = 'fackStatus' - - def read(self): - """Mock response.read.""" - return FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_TS_4_0_0 - - -class FakeGetBasicInfoResponseTdsTs_4_3_0(object): - """Fake GetBasicInfoTS response from TS nas.""" - - status = 'fackStatus' - - def read(self): - """Mock response.read.""" - return FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_TS_4_3_0 - - -class FakeGetBasicInfoResponseTdsEs_1_1_1(object): - """Fake GetBasicInfoTS response from ES nas.""" - - status = 'fackStatus' - - def read(self): - """Mock response.read.""" - return FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_ES_1_1_1 - - -class FakeGetBasicInfoResponseTdsEs_2_1_0(object): - """Fake GetBasicInfoTS response from ES nas.""" - - status = 'fackStatus' - - def read(self): - """Mock response.read.""" - return FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_ES_2_1_0 - - -class FakeGetBasicInfoResponseTdsEs_2_2_0(object): - """Fake GetBasicInfoTS response from ES nas.""" - - status = 'fackStatus' - - def read(self): - """Mock response.read.""" - return FAKE_RES_DETAIL_DATA_GETBASIC_INFO_TDS_ES_2_2_0 - - class FakeGetBasicInfoResponseError(object): """Fake GetBasicInfoTS response from TS nas.""" diff --git a/manila/tests/share/drivers/qnap/test_api.py b/manila/tests/share/drivers/qnap/test_api.py index 389c3073e9..3eea241ae8 100644 --- a/manila/tests/share/drivers/qnap/test_api.py +++ b/manila/tests/share/drivers/qnap/test_api.py @@ -424,13 +424,15 @@ class QnapAPITestCase(QnapShareDriverBaseTestCase): 'qnapadmin', 'Storage Pool 1') self.driver.api_executor.clone_snapshot( 'fakeSnapshotId', - 'fakeNewShareName') + 'fakeNewShareName', + 'fakeCloneSize') fake_params = { 'func': 'clone_qsnapshot', 'by_vol': '1', 'snapshotID': 'fakeSnapshotId', 'new_name': 'fakeNewShareName', + 'clone_size': '{}g'.format('fakeCloneSize'), 'sid': 'fakeSid', } sanitized_params = self._sanitize_params(fake_params) @@ -804,12 +806,14 @@ class QnapAPITestCase(QnapShareDriverBaseTestCase): fakes.FakeGetBasicInfoResponseEs_1_1_3()], ['self.driver.api_executor.clone_snapshot', {'snapshot_id': 'fakeSnapshotId', - 'new_sharename': 'fakeNewShareName'}, + 'new_sharename': 'fakeNewShareName', + 'clone_size': 'fakeCloneSize'}, fakes.FakeResultNegativeResponse(), fakes.FakeGetBasicInfoResponseEs_1_1_3()], ['self.driver.api_executor.clone_snapshot', {'snapshot_id': 'fakeSnapshotId', - 'new_sharename': 'fakeNewShareName'}, + 'new_sharename': 'fakeNewShareName', + 'clone_size': 'fakeCloneSize'}, fakes.FakeAuthPassFailResponse(), fakes.FakeGetBasicInfoResponseEs_1_1_3()], ['self.driver.api_executor.edit_share', diff --git a/manila/tests/share/drivers/qnap/test_qnap.py b/manila/tests/share/drivers/qnap/test_qnap.py index 6c397deeeb..d059e36a49 100644 --- a/manila/tests/share/drivers/qnap/test_qnap.py +++ b/manila/tests/share/drivers/qnap/test_qnap.py @@ -159,12 +159,6 @@ class QnapShareDriverLoginTestCase(QnapShareDriverBaseTestCase): }, { 'fake_basic_info': fakes.FakeGetBasicInfoResponseTesEs_2_1_0(), 'expect_result': api.QnapAPIExecutor - }, { - 'fake_basic_info': fakes.FakeGetBasicInfoResponseTdsTs_4_3_0(), - 'expect_result': api.QnapAPIExecutorTS - }, { - 'fake_basic_info': fakes.FakeGetBasicInfoResponseTdsEs_2_1_0(), - 'expect_result': api.QnapAPIExecutor }, { 'fake_basic_info': fakes.FakeGetBasicInfoResponseEs_1_1_3(), 'expect_result': api.QnapAPIExecutor @@ -200,15 +194,6 @@ class QnapShareDriverLoginTestCase(QnapShareDriverBaseTestCase): }, { 'fake_basic_info': fakes.FakeGetBasicInfoResponseTesEs_2_2_0(), 'expect_result': exception.ShareBackendException - }, { - 'fake_basic_info': fakes.FakeGetBasicInfoResponseTdsTs_4_0_0(), - 'expect_result': exception.ShareBackendException - }, { - 'fake_basic_info': fakes.FakeGetBasicInfoResponseTdsEs_1_1_1(), - 'expect_result': exception.ShareBackendException - }, { - 'fake_basic_info': fakes.FakeGetBasicInfoResponseTdsEs_2_2_0(), - 'expect_result': exception.ShareBackendException }, { 'fake_basic_info': fakes.FakeGetBasicInfoResponseEs_1_1_1(), 'expect_result': exception.ShareBackendException @@ -730,15 +715,13 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): expected_call_list, mock_api_return.get_share_info.call_args_list) mock_api_return.clone_snapshot.assert_called_once_with( - 'fakeShareName@fakeSnapshotName', 'fakeShareName') + 'fakeShareName@fakeSnapshotName', 'fakeShareName', 10) @mock.patch.object(qnap.QnapShareDriver, '_get_location_path') - @mock.patch('manila.share.API') @mock.patch.object(qnap.QnapShareDriver, '_gen_random_name') def test_create_share_from_snapshot_diff_size( self, mock_gen_random_name, - mock_share_api, mock_get_location_path): """Test create share from snapshot.""" fake_snapshot = fakes.SnapshotClass( @@ -755,9 +738,6 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): 'False', 'False', 'fakeVolName'] - mock_share_api.return_value.get.return_value = {'size': 5} - mock_api_executor.return_value.edit_share.return_value = ( - None) self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', 'qnapadmin', 'Storage Pool 1', @@ -775,19 +755,7 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): expected_call_list, mock_api_return.get_share_info.call_args_list) mock_api_return.clone_snapshot.assert_called_once_with( - 'fakeShareName@fakeSnapshotName', 'fakeShareName') - expect_share_dict = { - 'sharename': 'fakeShareName', - 'old_sharename': 'fakeShareName', - 'new_size': 10, - 'thin_provision': True, - 'compression': True, - 'deduplication': False, - 'ssd_cache': False, - 'share_proto': 'NFS' - } - mock_api_return.edit_share.assert_called_once_with( - expect_share_dict) + 'fakeShareName@fakeSnapshotName', 'fakeShareName', 10) def test_create_share_from_snapshot_without_snapshot_id(self): """Test create share from snapshot.""" @@ -1129,6 +1097,8 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): mock_api_executor = qnap.QnapShareDriver._create_api_executor mock_api_executor.return_value.get_share_info.return_value = ( self.get_share_info_return_value()) + mock_api_executor.return_value.get_snapshot_info.return_value = ( + self.get_snapshot_info_return_value()) mock_private_storage = mock.Mock() mock_private_storage.update.return_value = None mock_private_storage.get.side_effect = [ @@ -1147,35 +1117,6 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): mock_private_storage.update.assert_called_once_with( 'fakeSnapshotId', fake_metadata) - @mock.patch.object(qnap.QnapShareDriver, '_get_location_path') - def test_manage_existing_snapshot_not_exist( - self, - mock_get_location_path): - """Test manage existing snapshot with snapshot which does not exist.""" - fake_snapshot = fakes.SnapshotClass( - 10, 'fakeShareName@fakeSnapshotName') - - mock_api_executor = qnap.QnapShareDriver._create_api_executor - mock_api_executor.return_value.get_share_info.return_value = ( - self.get_share_info_return_value()) - mock_api_executor.return_value.get_snapshot_info.return_value = None - mock_private_storage = mock.Mock() - mock_private_storage.get.side_effect = [ - 'fakeVolId', 'fakeVolName'] - - self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', - 'qnapadmin', 'Storage Pool 1', - private_storage=mock_private_storage) - - mock_api_return = mock_api_executor.return_value - self.assertRaises( - exception.InvalidParameterValue, - self.driver.manage_existing_snapshot, - snapshot=fake_snapshot, - driver_options='driver_options') - mock_api_return.get_share_info.assert_called_once_with( - 'Storage Pool 1', vol_no='fakeVolId') - def test_unmanage_snapshot(self): """Test unmanage snapshot.""" fake_snapshot = fakes.SnapshotClass( diff --git a/releasenotes/notes/qnap-fix-share-and-snapshot-inconsistant-bd628c6e14eeab14.yaml b/releasenotes/notes/qnap-fix-share-and-snapshot-inconsistant-bd628c6e14eeab14.yaml new file mode 100644 index 0000000000..5b1a0af492 --- /dev/null +++ b/releasenotes/notes/qnap-fix-share-and-snapshot-inconsistant-bd628c6e14eeab14.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixed the QNAP driver so that the managed snapshot and the share which + created from snapshot will not be inconsistent in some cases.