From 7979bef0151f71c066939f32467e6bf31496a35e Mon Sep 17 00:00:00 2001 From: Kiran Pawar Date: Thu, 13 Aug 2026 11:08:40 +0000 Subject: [PATCH 1/2] Force delete share type extra specs The force delete in upstream is done using share type extra spec while sapcc is using config option. Remove CONF.force_delete_time and move the logic to share type extra specs. Change-Id: I32aa98a75663f61bde9cb5b3385cc35cde59100b --- manila/common/constants.py | 2 + .../netapp/dataontap/cluster_mode/lib_base.py | 12 +++--- manila/share/manager.py | 37 +++++++------------ .../dataontap/cluster_mode/test_lib_base.py | 14 ++++++- 4 files changed, 35 insertions(+), 30 deletions(-) diff --git a/manila/common/constants.py b/manila/common/constants.py index b96deee79c..f119917fd8 100644 --- a/manila/common/constants.py +++ b/manila/common/constants.py @@ -307,6 +307,8 @@ SHARE_LOCKED_BY_ACCESS_LOCK_REASON = 'Locked by access lock: %(lock_id)s' +ONE_WEEK_IN_SECONDS = (7 * 24 * 60 * 60) + class ExtraSpecs(object): 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 85c84369ba..28466dbb23 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -2351,11 +2351,13 @@ def _delete_share(self, share, vserver, vserver_client, # Share doesn't need to exist to be assigned to a fpolicy scope self._delete_fpolicy_for_share(share, vserver, vserver_client) - duration_sec = share.get('duration_seconds', 24 * 60 * 60) # 24 Hrs - force_delete_sec = CONF.force_delete_time * 60 * 60 - force_delete = False - if force_delete_sec > 0 and duration_sec < force_delete_sec: - force_delete = True + duration_sec = share.get( + 'duration_seconds', constants.ONE_WEEK_IN_SECONDS) + extra_specs = share_types.get_extra_specs_from_share(share) + force_delete_minutes = int( + extra_specs.get('netapp:force_delete_time_minutes', 0)) + force_delete = (force_delete_minutes > 0 + and duration_sec < force_delete_minutes * 60) if self._share_exists(share_name, vserver_client): clone_status = vserver_client.volume_clone_split_status(share_name) if clone_status == 'ongoing': diff --git a/manila/share/manager.py b/manila/share/manager.py index 39552068a0..c41a8b551d 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -85,17 +85,6 @@ default=False, help='Whether share servers will ' 'be deleted on deletion of the last share.'), - cfg.IntOpt('force_delete_time', - default=2, - min=0, - max=12, - help='Maximum time diff in hours to decide whether share will ' - 'be deleted or force deleted. If time difference between ' - 'share deletion time and creation time is less than ' - 'force_delete_time, share will be force deleted. In case ' - 'force delete fails, volume will be deleted in regular ' - 'way and it will stay in recovery queue for some time. ' - 'Value of 0 indicates disable force-delete feature.'), cfg.BoolOpt('unmanage_remove_access_rules', default=False, help='If set to True, then manila will deny access and remove ' @@ -4083,6 +4072,14 @@ def _get_share_details_from_instance(self, context, share_instance_id): share_server.get('backend_details') return (share, share_instance, share_server) + def _get_duration_seconds_for_instances(self, share_instance): + scheduled_at = share_instance.get('scheduled_at') + terminated_at = share_instance.get('terminated_at') + if scheduled_at and terminated_at: + duration = terminated_at - scheduled_at + return duration.total_seconds() + return constants.ONE_WEEK_IN_SECONDS + @run_concurrently @add_hooks @utils.require_driver_initialized @@ -4166,12 +4163,9 @@ def delete_share_instance(self, context, share_instance_id, force=False, return try: - scheduled_at = share_instance.get('scheduled_at') - terminated_at = share_instance.get('terminated_at') - if scheduled_at and terminated_at: - duration = terminated_at - scheduled_at - share_instance.update( - {'duration_seconds': duration.total_seconds()}) + duration_seconds = self._get_duration_seconds_for_instances( + share_instance) + share_instance.update({'duration_seconds': duration_seconds}) self.driver.delete_share(context, share_instance, share_server=share_server) except exception.ShareResourceNotFound: @@ -4304,12 +4298,9 @@ def do_deferred_share_deletion(self, ctxt): continue try: - scheduled_at = share_instance.get('scheduled_at') - terminated_at = share_instance.get('terminated_at') - if scheduled_at and terminated_at: - duration = terminated_at - scheduled_at - share_instance.update( - {'duration_seconds': duration.total_seconds()}) + duration_seconds = self._get_duration_seconds_for_instances( + share_instance) + share_instance.update({'duration_seconds': duration_seconds}) self.driver.delete_share(ctxt, share_instance, share_server=share_server) except exception.ShareResourceNotFound: 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 8fd5109353..2a3d0be172 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 @@ -1214,6 +1214,8 @@ def test__update_create_from_snapshot_status_driver_error(self): mock_pvt_storage_get = self.mock_object( self.library.private_storage, 'get', mock.Mock(return_value=json.dumps(copy_fake_src_share))) + self.mock_object(share_types, 'get_extra_specs_from_share', + mock.Mock(return_value={})) mock__create_continue = self.mock_object( self.library, '_create_from_snapshot_continue', mock.Mock(side_effect=exception.NetAppException)) @@ -2534,8 +2536,12 @@ def test_delete_share(self, force): vserver_client = mock.Mock() fake_share = copy.deepcopy(fake.SHARE) if force: - # assuming CONF.force_delete_time > 0 (which it is by default) - fake_share['duration_seconds'] = 0 + extra_specs = {'netapp:force_delete_time_minutes': '10'} + fake_share.update({'duration_seconds': 59}) + else: + extra_specs = {} + self.mock_object(share_types, 'get_extra_specs_from_share', + mock.Mock(return_value=extra_specs)) self.mock_object(self.library, '_get_vserver', mock.Mock(return_value=(fake.VSERVER1, @@ -2570,6 +2576,8 @@ def test_delete_share(self, force): def test__delete_share_no_remove_qos_and_export(self): vserver_client = mock.Mock() + self.mock_object(share_types, 'get_extra_specs_from_share', + mock.Mock(return_value={})) mock_share_exists = self.mock_object(self.library, '_share_exists', mock.Mock(return_value=True)) @@ -2632,6 +2640,8 @@ def test_delete_share_no_share_server(self, get_vserver_exception): def test_delete_share_not_found(self): vserver_client = mock.Mock() + self.mock_object(share_types, 'get_extra_specs_from_share', + mock.Mock(return_value={})) self.mock_object(self.library, '_get_vserver', mock.Mock(return_value=(fake.VSERVER1, From 310801d1fa3bcf458e9a3a9cf006f8be37dbb4ea Mon Sep 17 00:00:00 2001 From: Medha Choudhary Date: Fri, 14 Aug 2026 06:22:13 +0000 Subject: [PATCH 2/2] NetApp: fix InvalidShareType in create_share_from_snapshot The force-delete extra-spec feature added an unconditional share_types.get_extra_specs_from_share(share) call in _delete_share(), which needs share['share_type_id']. The synthetic share dict used to track the temporary source clone during cross-AZ/cross-backend create-from-snapshot never carried a share_type_id, causing: manila.exception.InvalidShareType: Invalid share type: id cannot be None. Fix: * Populate 'share_type_id' in the synthetic share dict. * Skip the extra specs lookup in _delete_share() when share_type_id is missing, instead of raising. Closes-Bug: #2163563 Signed-off-by: Medha Choudhary Assisted-By: GitHub Copilot Co-authored-by: Copilot Change-Id: Id7f303d3b07c4efb5d345d115069c3f393d33695 (cherry picked from commit 3be932ef584b8c351dc0471611e3970edda9a312) --- .../drivers/netapp/dataontap/cluster_mode/lib_base.py | 11 +++++++---- .../netapp/dataontap/cluster_mode/test_lib_base.py | 3 ++- ...nvalidsharetype-force-delete-bfe7751e644cf5c6.yaml | 6 ++++++ 3 files changed, 15 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/netapp-fix-invalidsharetype-force-delete-bfe7751e644cf5c6.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 28466dbb23..c55df7dc6f 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -965,7 +965,8 @@ def create_share_from_snapshot(self, context, share, snapshot, src_share_instance = { 'id': share['id'], 'host': parent_share.get('host'), - 'share_server': parent_share_server or None + 'share_server': parent_share_server or None, + 'share_type_id': share.get('share_type_id'), } # NOTE(dviroel): Data Motion functions access share's 'share_server' # attribute to get vserser information. @@ -2351,11 +2352,13 @@ def _delete_share(self, share, vserver, vserver_client, # Share doesn't need to exist to be assigned to a fpolicy scope self._delete_fpolicy_for_share(share, vserver, vserver_client) - duration_sec = share.get( - 'duration_seconds', constants.ONE_WEEK_IN_SECONDS) - extra_specs = share_types.get_extra_specs_from_share(share) + extra_specs = {} + if share.get('share_type_id'): + extra_specs = share_types.get_extra_specs_from_share(share) force_delete_minutes = int( extra_specs.get('netapp:force_delete_time_minutes', 0)) + duration_sec = share.get( + 'duration_seconds', constants.ONE_WEEK_IN_SECONDS) force_delete = (force_delete_minutes > 0 and duration_sec < force_delete_minutes * 60) if self._share_exists(share_name, vserver_client): 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 2a3d0be172..cfd25e897b 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 @@ -1001,7 +1001,8 @@ def to_dict(self): self.temp_src_share = { 'id': self.fake_share['id'], 'host': self.parent_share['host'], - 'share_server': self.parent_share_server or None + 'share_server': self.parent_share_server or None, + 'share_type_id': self.fake_share.get('share_type_id'), } @ddt.data({'dest_cluster': fake.CLUSTER_NAME, 'is_flexgroup': False, diff --git a/releasenotes/notes/netapp-fix-invalidsharetype-force-delete-bfe7751e644cf5c6.yaml b/releasenotes/notes/netapp-fix-invalidsharetype-force-delete-bfe7751e644cf5c6.yaml new file mode 100644 index 0000000000..9254cc5360 --- /dev/null +++ b/releasenotes/notes/netapp-fix-invalidsharetype-force-delete-bfe7751e644cf5c6.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + NetApp driver: fixed the share type extra specs lookup in the share + delete workflow (added for ``netapp:force_delete_time_minutes`` + support), which could raise an ``InvalidShareType`` exception.