From 5eca81eef555cd9d4771872f2a1f79dede446c08 Mon Sep 17 00:00:00 2001 From: Kiran Pawar Date: Tue, 5 May 2026 12:59:55 +0000 Subject: [PATCH 1/2] netapp: skip pcuser config for DP read-only volumes When all_squash=true metadata is set on an NFS share and access rules are updated on a readable DP (data protection) replica, the driver was calling set_pcuser_for_volume on the SnapMirror destination volume. ONTAP rejects this with error code 160 because DP volumes are read-only and their user attribute cannot be modified. Fix by propagating the replica flag (True for non-active readable DP replicas, False otherwise) from lib_base into the NFS protocol helper update_access() method. When replica=True the pcuser configuration step is skipped with an informational log message. The replica flag is set correctly in all callers: - update_access(): True for non-active readable replicas (DP volumes) - create_replica(): True (new replica is a DP volume) - promote_replica(): False (promoted replica becomes read-write) - _safe_change_replica_source(): True (old source becomes new DP target) Change-Id: I48fd7e4aced6c14fcd6dae053833ff786ec507df Signed-off-by: Kiran Pawar (cherry picked from commit 74a8848477a740e01debadf6901929dda5f0ca46) --- .../netapp/dataontap/cluster_mode/lib_base.py | 15 ++++-- .../netapp/dataontap/protocols/base.py | 2 +- .../netapp/dataontap/protocols/cifs_cmode.py | 2 +- .../netapp/dataontap/protocols/nfs_cmode.py | 10 ++-- .../dataontap/cluster_mode/test_lib_base.py | 48 +++++++++++++++++-- .../dataontap/protocols/test_nfs_cmode.py | 24 ++++++++++ ...dp-flexgroup-replica-a3f8b2c1d4e56789.yaml | 5 ++ 7 files changed, 92 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/netapp-skip-pcuser-dp-flexgroup-replica-a3f8b2c1d4e56789.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 3fc4dea4a7..7f50a4dbda 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -3089,9 +3089,14 @@ def update_access(self, context, share, access_rules, add_rules, share_name = self._get_backend_share_name(share['id']) if self._share_exists(share_name, vserver_client): + is_dp_replica = ( + replica_state is not None and + replica_state != constants.REPLICA_STATE_ACTIVE and + self._is_readable_replica(share)) helper = self._get_helper(share) helper.set_client(vserver_client) - helper.update_access(share, share_name, access_rules) + helper.update_access(share, share_name, access_rules, + replica=is_dp_replica) else: raise exception.ShareResourceNotFound(share_id=share['id']) @@ -3386,7 +3391,8 @@ def create_replica(self, context, replica_list, new_replica, helper.set_client(vserver_client) share_name = self._get_backend_share_name(new_replica_id) try: - helper.update_access(new_replica, share_name, access_rules) + helper.update_access(new_replica, share_name, access_rules, + replica=True) except Exception: model_update['access_rules_status'] = ( constants.SHARE_INSTANCE_RULES_ERROR) @@ -3922,7 +3928,8 @@ def _convert_destination_replica_to_independent( helper = self._get_helper(replica) helper.set_client(vserver_client) try: - helper.update_access(replica, share_name, access_rules) + helper.update_access(replica, share_name, access_rules, + replica=False) except Exception: new_active_replica['access_rules_status'] = ( constants.SHARE_INSTANCE_RULES_SYNCING) @@ -4032,7 +4039,7 @@ def _safe_change_replica_source(self, dm_session, replica, helper.set_client(replica_client) try: helper.update_access( - replica, replica_volume_name, access_rules) + replica, replica_volume_name, access_rules, replica=True) except Exception: replica['access_rules_status'] = ( constants.SHARE_INSTANCE_RULES_ERROR) diff --git a/manila/share/drivers/netapp/dataontap/protocols/base.py b/manila/share/drivers/netapp/dataontap/protocols/base.py index dd1ebc437e..3d3c647436 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/base.py +++ b/manila/share/drivers/netapp/dataontap/protocols/base.py @@ -77,7 +77,7 @@ def delete_share(self, share, share_name): """Deletes NAS share.""" @abc.abstractmethod - def update_access(self, share, share_name, rules): + def update_access(self, share, share_name, rules, replica=False): """Replaces the list of access rules known to the backend storage.""" @abc.abstractmethod diff --git a/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py b/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py index 0fb3677bd1..644547a1cc 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py +++ b/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py @@ -73,7 +73,7 @@ def delete_share(self, share, share_name): @na_utils.trace @base.access_rules_synchronized - def update_access(self, share, share_name, rules): + def update_access(self, share, share_name, rules, replica=False): """Replaces the list of access rules known to the backend storage.""" _, cifs_share_name = self._get_export_location(share) diff --git a/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py b/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py index ef3f229b6e..7758953c99 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py +++ b/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py @@ -85,7 +85,7 @@ def delete_share(self, share, share_name): @na_utils.trace @base.access_rules_synchronized - def update_access(self, share, share_name, rules): + def update_access(self, share, share_name, rules, replica=False): """Replaces the list of access rules known to the backend storage.""" # Ensure rules are valid @@ -155,8 +155,12 @@ def update_access(self, share, share_name, rules): export_policy_name) if set_volume_user_to_pcuser: - LOG.info('Setting user to pcuser for share %s.', share_name) - self._client.set_pcuser_for_volume(share_name) + if replica: + LOG.info('Skipping pcuser configuration for DP read-only ' + 'volume %s.', share_name) + else: + LOG.info('Setting user to pcuser for share %s.', share_name) + self._client.set_pcuser_for_volume(share_name) @na_utils.trace def _validate_access_rule(self, rule): 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 1cb6250e1e..cddf72bffc 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 @@ -4004,7 +4004,8 @@ def test_update_access(self): mock_share_exists.assert_called_once_with(share_name, vserver_client) protocol_helper.set_client.assert_called_once_with(vserver_client) protocol_helper.update_access.assert_called_once_with( - fake.SHARE, fake.SHARE_NAME, [fake.SHARE_ACCESS]) + fake.SHARE, fake.SHARE_NAME, [fake.SHARE_ACCESS], + replica=False) @ddt.data(exception.InvalidInput(reason='fake_reason'), exception.VserverNotSpecified(), @@ -4106,7 +4107,8 @@ def test_update_access_to_active_replica(self): mock_share_exists.assert_called_once_with(share_name, vserver_client) protocol_helper.set_client.assert_called_once_with(vserver_client) protocol_helper.update_access.assert_called_once_with( - fake.SHARE, fake.SHARE_NAME, [fake.SHARE_ACCESS]) + fake_share_copy, fake.SHARE_NAME, [fake.SHARE_ACCESS], + replica=False) @ddt.data(True, False) def test_update_access_to_in_sync_replica(self, is_readable): @@ -4139,9 +4141,38 @@ def test_update_access_to_in_sync_replica(self, is_readable): if is_readable: mock_get_vserver.assert_called_once_with( share_server=fake.SHARE_SERVER) + protocol_helper.update_access.assert_called_once_with( + fake_share_copy, fake.SHARE_NAME, [fake.SHARE_ACCESS], + replica=True) else: mock_get_vserver.assert_not_called() + def test_update_access_to_readable_dp_replica_skips_pcuser(self): + """Verify replica=True is passed for non-active readable replicas.""" + fake_share_copy = copy.deepcopy(fake.SHARE) + fake_share_copy['replica_state'] = constants.REPLICA_STATE_OUT_OF_SYNC + vserver_client = mock.Mock() + self.mock_object( + self.library, '_get_vserver', + mock.Mock(return_value=(fake.VSERVER1, vserver_client))) + self.mock_object(self.library, '_is_readable_replica', + mock.Mock(return_value=True)) + protocol_helper = mock.Mock() + self.mock_object(self.library, '_get_helper', + mock.Mock(return_value=protocol_helper)) + self.mock_object(self.library, '_share_exists', + mock.Mock(return_value=True)) + + self.library.update_access(self.context, + fake_share_copy, + [fake.SHARE_ACCESS], + [], [], [], + share_server=fake.SHARE_SERVER) + + protocol_helper.update_access.assert_called_once_with( + fake_share_copy, fake.SHARE_NAME, [fake.SHARE_ACCESS], + replica=True) + def test_setup_server(self): self.assertRaises(NotImplementedError, self.library.setup_server, @@ -4403,7 +4434,8 @@ def test_create_replica(self, is_readable, rules_status): fake.SHARE, None, fake.VSERVER1, vserver_client, replica=True) mock_get_helper.assert_called_once_with(fake.SHARE) protocol_helper.update_access.assert_called_once_with( - fake.SHARE, fake.SHARE_NAME, [fake.SHARE_ACCESS]) + fake.SHARE, fake.SHARE_NAME, [fake.SHARE_ACCESS], + replica=True) else: mock_create_export.assert_not_called() mock_get_helper.assert_not_called() @@ -5439,7 +5471,8 @@ def test_promote_replica_with_access_rules(self): self.fake_replica_2['id']) mock_helper.update_access.assert_called_once_with(self.fake_replica_2, share_name, - [fake.SHARE_ACCESS]) + [fake.SHARE_ACCESS], + replica=False) self.library._unmount_orig_active_replica.assert_called_once_with( self.fake_replica, fake.VSERVER1) self.library._handle_qos_on_replication_change.assert_called_once() @@ -5757,7 +5790,8 @@ def test_convert_destination_replica_to_independent_failed_access_rules( fake_helper.assert_has_calls([ mock.call.set_client(mock.ANY), - mock.call.update_access(mock.ANY, mock.ANY, fake_access_rules), + mock.call.update_access(mock.ANY, mock.ANY, fake_access_rules, + replica=False), ]) self.assertEqual('fake_export_location', @@ -5809,11 +5843,15 @@ def test_safe_change_replica_source(self, is_dr): if is_dr: self.assertEqual([], replica['export_locations']) mock_dm_session.wait_for_mount_replica.assert_not_called() + protocol_helper.update_access.assert_not_called() else: self.assertEqual('fake_export_location', replica['export_locations']) mock_dm_session.wait_for_mount_replica.assert_called_once_with( mock_client, fake.SHARE_NAME, timeout=30) + protocol_helper.update_access.assert_called_once_with( + self.fake_replica, fake.SHARE_NAME, [fake.SHARE_ACCESS], + replica=True) @ddt.data(True, False) def test_safe_change_replica_source_sync_policy(self, is_dr): diff --git a/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py b/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py index ae406993a6..db7490bf8a 100644 --- a/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py +++ b/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py @@ -131,6 +131,30 @@ def test_update_access(self, all_squash_metadata): if all_squash_metadata: self.mock_client.set_pcuser_for_volume.assert_called_once_with( fake.SHARE_NAME) + else: + self.mock_client.set_pcuser_for_volume.assert_not_called() + + def test_update_access_all_squash_replica_skips_pcuser(self): + + self.mock_object(self.helper, '_ensure_export_policy') + self.mock_object(self.helper, + '_get_export_policy_name', + mock.Mock(return_value='fake_export_policy')) + self.mock_object(self.helper, + '_get_temp_export_policy_name', + mock.Mock(side_effect=['fake_new_export_policy', + 'fake_old_export_policy'])) + self.mock_object(self.helper, + '_get_auth_methods', + mock.Mock(return_value='fake_auth_method')) + + share = fake.NFS_SHARE.copy() + share.update({'metadata': {'all_squash': 'true'}}) + + self.helper.update_access(share, fake.SHARE_NAME, [fake.IP_ACCESS], + replica=True) + + self.mock_client.set_pcuser_for_volume.assert_not_called() def test_validate_access_rule(self): diff --git a/releasenotes/notes/netapp-skip-pcuser-dp-flexgroup-replica-a3f8b2c1d4e56789.yaml b/releasenotes/notes/netapp-skip-pcuser-dp-flexgroup-replica-a3f8b2c1d4e56789.yaml new file mode 100644 index 0000000000..a8b28dec86 --- /dev/null +++ b/releasenotes/notes/netapp-skip-pcuser-dp-flexgroup-replica-a3f8b2c1d4e56789.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixed an issue on the NetApp ONTAP driver during updating access rules for + shares having replicas in combination with ``all_squash=true`` metadata. From b31f5315db8b4902566f1658f155e2942c5a0a45 Mon Sep 17 00:00:00 2001 From: Kiran Pawar Date: Tue, 24 Mar 2026 12:46:30 +0000 Subject: [PATCH 2/2] Fix TypeError in capacity weigher when capacity is None When free_capacity_gb, total_capacity_gb, or provisioned_capacity_gb is None, the weigher previously raised a TypeError by attempting arithmetic with None. Following the existing 'unknown' treatment, None values are now handled as worst-case (sorts to bottom), which is safer than assuming zero provisioned capacity. Closes-bug: #2145805 Change-Id: I6e7e9dce772beebadd6199039aed5906e6ba24e6 Signed-off-by: Kiran Pawar (cherry picked from commit 3c04a171dee22bf41cac8c8df2c7f04910c88d36) --- manila/scheduler/weighers/capacity.py | 15 ++++++++---- .../tests/scheduler/weighers/test_capacity.py | 24 +++++++++++++++++++ ...-in-capacity-weigher-1facc9636e4abdde.yaml | 6 +++++ 3 files changed, 41 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/bug-2145805-Fix-TypeError-in-capacity-weigher-1facc9636e4abdde.yaml diff --git a/manila/scheduler/weighers/capacity.py b/manila/scheduler/weighers/capacity.py index 222853b639..4182f19768 100644 --- a/manila/scheduler/weighers/capacity.py +++ b/manila/scheduler/weighers/capacity.py @@ -78,10 +78,17 @@ def _weigh_object(self, host_state, weight_properties): if use_thin_logic and thin_provisioning: # NOTE(xyang): Calculate virtual free capacity for thin # provisioning. - free = math.floor( - total * host_state.max_over_subscription_ratio - - host_state.provisioned_capacity_gb - - total * reserved) + if host_state.provisioned_capacity_gb is not None: + free = math.floor( + total * host_state.max_over_subscription_ratio - + host_state.provisioned_capacity_gb - + total * reserved) + else: + # None is treated same as 'unknown' + if CONF.capacity_weight_multiplier > 0: + free = float('-inf') + else: + free = float('inf') else: # NOTE(xyang): Calculate how much free space is left after # taking into account the reserved space. diff --git a/manila/tests/scheduler/weighers/test_capacity.py b/manila/tests/scheduler/weighers/test_capacity.py index cedd644d7b..a817280698 100644 --- a/manila/tests/scheduler/weighers/test_capacity.py +++ b/manila/tests/scheduler/weighers/test_capacity.py @@ -146,6 +146,30 @@ def test_unknown_is_last(self): 'host6', utils.extract_host(last_host.obj.host)) self.assertEqual(0.0, last_host.weight) + def test_none_provisioned_capacity_thin_is_treated_as_unknown(self): + """None provisioned_capacity_gb in thin path must sort to worst.""" + host_state = mock.Mock() + host_state.free_capacity_gb = 500 + host_state.total_capacity_gb = 1024 + host_state.provisioned_capacity_gb = None + host_state.max_over_subscription_ratio = 2.0 + host_state.thin_provisioning = True + host_state.reserved_percentage = 0 + host_state.reserved_snapshot_percentage = 0 + host_state.reserved_share_extend_percentage = 0 + + weigher = capacity.CapacityWeigher() + weight_properties = { + 'size': 1, + 'share_type': { + 'extra_specs': { + 'capabilities:thin_provisioning': ' True', + } + } + } + result = weigher._weigh_object(host_state, weight_properties) + self.assertEqual(float('-inf'), result) + @ddt.data( {'cap_thin': ' True', 'cap_thin_key': 'capabilities:thin_provisioning', diff --git a/releasenotes/notes/bug-2145805-Fix-TypeError-in-capacity-weigher-1facc9636e4abdde.yaml b/releasenotes/notes/bug-2145805-Fix-TypeError-in-capacity-weigher-1facc9636e4abdde.yaml new file mode 100644 index 0000000000..f274fd0675 --- /dev/null +++ b/releasenotes/notes/bug-2145805-Fix-TypeError-in-capacity-weigher-1facc9636e4abdde.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Manila scheduler capacity weigher is fixed in case provisioned_capacity_gb + is None. For more details, please refer to + `launchpad bug #2145805 `_.