From 5eca81eef555cd9d4771872f2a1f79dede446c08 Mon Sep 17 00:00:00 2001 From: Kiran Pawar Date: Tue, 5 May 2026 12:59:55 +0000 Subject: [PATCH] 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.