From 28908d69dad4c39962b9ed15bb0eb6729db0996c Mon Sep 17 00:00:00 2001 From: Kiran Pawar Date: Fri, 24 May 2024 10:09:00 +0000 Subject: [PATCH] Allow drivers to set share size after revert to snapshot For NetApp ONTAP driver, share revert to snapshot considers the largest size between original share size and snapshot size after revert is done. This is different from the users' and Manila's expectation where share size is updated to snapshot size. Closes-bug: #2064502 Change-Id: I6a1b87eabeb7bef80725a537acb6771c35b76afc --- .../netapp/dataontap/cluster_mode/lib_base.py | 4 ++ manila/share/manager.py | 43 ++++++++++++++----- .../dataontap/cluster_mode/test_lib_base.py | 3 +- manila/tests/share/test_manager.py | 38 ++++++++++------ ...x-revert-to-snapshot-f23ab4dc325b2c42.yaml | 9 ++++ 5 files changed, 74 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/bug-2064502-netapp-fix-revert-to-snapshot-f23ab4dc325b2c42.yaml diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py index 01b18eb286..db883940bc 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -1831,6 +1831,10 @@ class NetAppCmodeFileStorageLibrary(object): self._get_backend_snapshot_name(snapshot['id'])) LOG.debug('Restoring snapshot %s', snapshot_name) vserver_client.restore_snapshot(share_name, snapshot_name) + volume = vserver_client.get_volume(share_name) + + # When calculating the size, round up to the next GB. + return int(math.ceil(float(volume['size']) / units.Gi)) @na_utils.trace def delete_snapshot(self, context, snapshot, share_server=None, diff --git a/manila/share/manager.py b/manila/share/manager.py index 7fabfd694f..2916c09ddb 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -3463,11 +3463,12 @@ class ShareManager(manager.SchedulerDependentManager): context, snapshot_instance, snapshot=snapshot) try: - self.driver.revert_to_snapshot(context, - snapshot_instance_dict, - share_access_rules, - snapshot_access_rules, - share_server=share_server) + updated_share_size = self.driver.revert_to_snapshot( + context, + snapshot_instance_dict, + share_access_rules, + snapshot_access_rules, + share_server=share_server) except Exception as excep: with excutils.save_and_reraise_exception(): @@ -3496,15 +3497,37 @@ class ShareManager(manager.SchedulerDependentManager): resource_id=share_id, exception=excep) + # fail-safe in case driver returned size is None or invalid + if not updated_share_size: + updated_share_size = snapshot['size'] + else: + try: + int(updated_share_size) + except ValueError: + updated_share_size = snapshot['size'] + if reservations: - QUOTAS.commit( - context, reservations, project_id=project_id, user_id=user_id, - share_type_id=share_type_id, - ) + if updated_share_size == snapshot['size']: + QUOTAS.commit( + context, reservations, project_id=project_id, + user_id=user_id, share_type_id=share_type_id, + ) + else: + # NOTE(kpdev): The driver tells us that the share size wasn't + # modified to the snapshot's size; so no need to commit quota + # changes + QUOTAS.rollback( + context, reservations, project_id=project_id, + user_id=user_id, share_type_id=share_type_id, + ) + if updated_share_size != share['size']: + LOG.error("Driver returned an unexpected size %d on " + "revert to snapshot operation. You need to " + "adjust the quota", updated_share_size) self.db.share_update( context, share_id, - {'status': constants.STATUS_AVAILABLE, 'size': snapshot['size']}) + {'status': constants.STATUS_AVAILABLE, 'size': updated_share_size}) self.db.share_snapshot_update( context, snapshot_id, {'status': constants.STATUS_AVAILABLE}) diff --git a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py index 67d11cbce1..ca660e6ea8 100644 --- a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py +++ b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py @@ -2449,6 +2449,7 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): '_get_vserver', mock.Mock(return_value=(fake.VSERVER1, vserver_client))) + vserver_client.get_volume.return_value = fake.FLEXVOL_TO_MANAGE fake_snapshot = copy.deepcopy(fake.SNAPSHOT) if use_snap_provider_location: fake_snapshot['provider_location'] = 'fake-provider-location' @@ -2458,7 +2459,7 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): result = self.library.revert_to_snapshot( self.context, fake_snapshot, share_server=fake.SHARE_SERVER) - self.assertIsNone(result) + self.assertIsNotNone(result) share_name = self.library._get_backend_share_name( fake_snapshot['share_id']) snapshot_name = (self.library._get_backend_snapshot_name( diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index cb0a033a45..229c95f99b 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -7930,9 +7930,12 @@ class ShareManagerTestCase(test.TestCase): mock.ANY, share, snapshot, reservations, share_access_rules, snapshot_access_rules, share_id=share_id) - @ddt.data(None, 'fake_reservations') - def test__revert_to_snapshot(self, reservations): - + @ddt.data((None, False), + (None, True), + ('fake_reservations', False), + ('fake_reservations', True)) + @ddt.unpack + def test__revert_to_snapshot(self, reservations, revert_return_share_size): mock_quotas_rollback = self.mock_object(quota.QUOTAS, 'rollback') mock_quotas_commit = self.mock_object(quota.QUOTAS, 'commit') self.mock_object( @@ -7944,14 +7947,15 @@ class ShareManagerTestCase(test.TestCase): share = fakes.fake_share( id=share_id, instance={'id': 'fake_instance_id', 'share_type_id': 'fake_share_type_id'}, - project_id='fake_project', user_id='fake_user', size=2) + project_id='fake_project', user_id='fake_user', + size=5 if revert_return_share_size else 3) snapshot_instance = fakes.fake_snapshot_instance( share_id=share_id, share=share, name='fake_snapshot', share_instance=share['instance']) snapshot = fakes.fake_snapshot( id='fake_snapshot_id', share_id=share_id, share=share, instance=snapshot_instance, project_id='fake_project', - user_id='fake_user', size=1) + user_id='fake_user', size=4) share_access_rules = [] snapshot_access_rules = [] @@ -7965,6 +7969,8 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.db, 'share_update') mock_share_snapshot_update = self.mock_object( self.share_manager.db, 'share_snapshot_update') + mock_driver.revert_to_snapshot.return_value = ( + 5 if revert_return_share_size else None) self.share_manager._revert_to_snapshot(self.context, share, snapshot, reservations, @@ -7978,19 +7984,27 @@ class ShareManagerTestCase(test.TestCase): share_access_rules, snapshot_access_rules, share_server=None) - self.assertFalse(mock_quotas_rollback.called) if reservations: - mock_quotas_commit.assert_called_once_with( - mock.ANY, reservations, project_id='fake_project', - user_id='fake_user', - share_type_id=( - snapshot_instance['share_instance']['share_type_id'])) + if revert_return_share_size: + mock_quotas_rollback.assert_called_once_with( + mock.ANY, reservations, project_id='fake_project', + user_id='fake_user', + share_type_id=( + snapshot_instance['share_instance']['share_type_id'])) + else: + self.assertFalse(mock_quotas_rollback.called) + mock_quotas_commit.assert_called_once_with( + mock.ANY, reservations, project_id='fake_project', + user_id='fake_user', + share_type_id=( + snapshot_instance['share_instance']['share_type_id'])) else: self.assertFalse(mock_quotas_commit.called) mock_share_update.assert_called_once_with( mock.ANY, share_id, - {'status': constants.STATUS_AVAILABLE, 'size': snapshot['size']}) + {'status': constants.STATUS_AVAILABLE, + 'size': 5 if revert_return_share_size else 4}) mock_share_snapshot_update.assert_called_once_with( mock.ANY, 'fake_snapshot_id', {'status': constants.STATUS_AVAILABLE}) diff --git a/releasenotes/notes/bug-2064502-netapp-fix-revert-to-snapshot-f23ab4dc325b2c42.yaml b/releasenotes/notes/bug-2064502-netapp-fix-revert-to-snapshot-f23ab4dc325b2c42.yaml new file mode 100644 index 0000000000..ce5fa25e59 --- /dev/null +++ b/releasenotes/notes/bug-2064502-netapp-fix-revert-to-snapshot-f23ab4dc325b2c42.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + When reverting shares to snapshots that are larger or smaller than + the share, some storage systems such as NetApp ONTAP always defer + to the larger of the two sizes. Manila's share manager interface now + accounts for this behavior, and adjusts project quotas appropriately. + For more details, please check + Launchpad `bug 2064502 `_