openstack/manila
Code Issues Proposed changes

[NetApp] Fix non-disruptive migration cifs shares

Browse Source 
Migrate non-disruptive cifs share from different pools change the
export location. When the non-disruptive migration complete
process is started a new share and export location is created. As
result, Manila finds a conflict between the old export location and
the new one.

This patch add a condition to skip export location creation when
a CIFS migration is in progress, also change the way that the export
location is created. Instead of create the export path with share
name, the new one is taken from the backend. The fix is only for
ZAPI API calls.

Change-Id: I1bb888a0b644f0b071816d275d464c4dd27125a7
Co-authored-by: Lucas Oliveira <lucasmoliveira059@gmail.com>
Closes-bug: #1920937
This commit is contained in:
CaiqueMello 2022-09-16 14:01:17 +00:00 committed by MelloCaique
parent f03e84ae62
commit 9ddddeeea3
8 changed files with 74 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
manila/db/sqlalchemy/api.py
View File

@ -1698,7 +1698,7 @@ def share_instance_get(context, share_instance_id, session=None,
).filter_by(
id=share_instance_id,
).options(
joinedload('export_locations'),
joinedload('export_locations').joinedload('_el_metadata_bare'),
joinedload('share_type'),
).first()
if result is None:

5
manila/share/drivers/netapp/dataontap/client/client_cmode.py
View File

@ -3434,9 +3434,8 @@ class NetAppCmodeClient(client_base.NetAppBaseClient):
self.send_request('cg-commit', api_args)
@na_utils.trace
def create_cifs_share(self, share_name):
share_path = '/%s' % share_name
api_args = {'path': share_path, 'share-name': share_name}
def create_cifs_share(self, share_name, path):
api_args = {'path': path, 'share-name': share_name}
self.send_request('cifs-share-create', api_args)
@na_utils.trace

27
manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py
View File

@ -3571,12 +3571,31 @@ class NetAppCmodeFileStorageLibrary(object):
}
LOG.info(msg, msg_args)
# NOTE(gouthamr): For nondisruptive migration, current export
# NOTE(gouthamr): For NFS nondisruptive migration, current export
# policy will not be cleared, the export policy will be renamed to
# match the name of the share.
export_locations = self._create_export(
destination_share, share_server, vserver, vserver_client,
clear_current_export_policy=False)
# NOTE (caiquemello): For CIFS nondisruptive migration, current CIFS
# share cannot be renamed, so keep the previous CIFS share.
share_protocol = source_share['share_proto'].lower()
if share_protocol != 'cifs':
export_locations = self._create_export(
destination_share, share_server, vserver, vserver_client,
clear_current_export_policy=False)
else:
export_locations = []
for item in source_share['export_locations']:
export_locations.append(
{
'path': item['path'],
'is_admin_only': item['is_admin_only'],
'metadata': item['el_metadata']
}
)
# Sort the export locations to report preferred paths first
export_locations = self._sort_export_locations_by_preferred_paths(
export_locations)
src_snaps_dict = {s['id']: s for s in source_snapshots}
snapshot_updates = {}

24
manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py
View File

@ -46,12 +46,13 @@ class NetAppCmodeCIFSHelper(base.NetAppBaseHelper):
"""
cifs_exist = self._client.cifs_share_exists(share_name)
export_path = self._client.get_volume_junction_path(share_name)
if ensure_share_already_exists and not cifs_exist:
msg = _("The expected CIFS share %(share_name)s was not found.")
msg_args = {'share_name': share_name}
raise exception.NetAppException(msg % msg_args)
elif not cifs_exist:
self._client.create_cifs_share(share_name)
self._client.create_cifs_share(share_name, export_path)
self._client.remove_cifs_share_access(share_name, 'Everyone')
# Ensure 'ntfs' security style for RW volume. DP volumes cannot set it.
@ -61,8 +62,8 @@ class NetAppCmodeCIFSHelper(base.NetAppBaseHelper):
# Return a callback that may be used for generating export paths
# for this share.
return (lambda export_address, share_name=share_name:
r'\\%s\%s' % (export_address, share_name))
return (lambda export_address, export_path=export_path:
r'\\%s%s' % (export_address, export_path.replace('/', '\\')))
@na_utils.trace
def delete_share(self, share, share_name):
@ -75,6 +76,7 @@ class NetAppCmodeCIFSHelper(base.NetAppBaseHelper):
def update_access(self, share, share_name, rules):
"""Replaces the list of access rules known to the backend storage."""
_, cifs_share_name = self._get_export_location(share)
# Ensure rules are valid
for rule in rules:
self._validate_access_rule(rule)
@ -82,13 +84,13 @@ class NetAppCmodeCIFSHelper(base.NetAppBaseHelper):
new_rules = {rule['access_to']: rule['access_level'] for rule in rules}
# Get rules from share
existing_rules = self._get_access_rules(share, share_name)
existing_rules = self._get_access_rules(share, cifs_share_name)
# Update rules in an order that will prevent transient disruptions
self._handle_added_rules(share_name, existing_rules, new_rules)
self._handle_ro_to_rw_rules(share_name, existing_rules, new_rules)
self._handle_rw_to_ro_rules(share_name, existing_rules, new_rules)
self._handle_deleted_rules(share_name, existing_rules, new_rules)
self._handle_added_rules(cifs_share_name, existing_rules, new_rules)
self._handle_ro_to_rw_rules(cifs_share_name, existing_rules, new_rules)
self._handle_rw_to_ro_rules(cifs_share_name, existing_rules, new_rules)
self._handle_deleted_rules(cifs_share_name, existing_rules, new_rules)
@na_utils.trace
def _validate_access_rule(self, rule):
@ -170,8 +172,10 @@ class NetAppCmodeCIFSHelper(base.NetAppBaseHelper):
@na_utils.trace
def get_share_name_for_share(self, share):
"""Returns the flexvol name that hosts a share."""
_, share_name = self._get_export_location(share)
return share_name
_, volume_junction_path = self._get_export_location(share)
volume = self._client.get_volume_at_junction_path(
f"/{volume_junction_path}")
return volume.get('name') if volume else None
@na_utils.trace
def _get_export_location(self, share):

5
manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py
View File

@ -5139,10 +5139,11 @@ class NetAppClientCmodeTestCase(test.TestCase):
self.mock_object(self.client, 'send_request')
self.client.create_cifs_share(fake.SHARE_NAME)
self.client.create_cifs_share(
fake.SHARE_NAME, fake.VOLUME_JUNCTION_PATH)
cifs_share_create_args = {
'path': '/%s' % fake.SHARE_NAME,
'path': fake.VOLUME_JUNCTION_PATH,
'share-name': fake.SHARE_NAME
}

4
manila/tests/share/drivers/netapp/dataontap/protocols/fakes.py
View File

@ -23,8 +23,10 @@ SHARE_ADDRESS_2 = '10.10.10.20'
CLIENT_ADDRESS_1 = '20.20.20.10'
CLIENT_ADDRESS_2 = '20.20.20.20'
CIFS_SHARE_PATH = '/%s' % SHARE_NAME
CIFS_SHARE_PATH_PARSED = '\\%s' % SHARE_NAME
CIFS_SHARE = {
'export_location': r'\\%s\%s' % (SHARE_ADDRESS_1, SHARE_NAME),
'export_location': r'\\%s%s' % (SHARE_ADDRESS_1, CIFS_SHARE_PATH_PARSED),
'id': SHARE_ID
}

23
manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py
View File

@ -46,6 +46,8 @@ class NetAppClusteredCIFSHelperTestCase(test.TestCase):
def test_create_share(self, replica, cifs_exist):
self.mock_client.cifs_share_exists.return_value = cifs_exist
self.mock_client.get_volume_junction_path.return_value = (
fake.CIFS_SHARE_PATH)
result = self.helper.create_share(
fake.CIFS_SHARE, fake.SHARE_NAME,
@ -54,8 +56,8 @@ class NetAppClusteredCIFSHelperTestCase(test.TestCase):
export_addresses = [fake.SHARE_ADDRESS_1, fake.SHARE_ADDRESS_2]
export_paths = [result(address) for address in export_addresses]
expected_paths = [
r'\\%s\%s' % (fake.SHARE_ADDRESS_1, fake.SHARE_NAME),
r'\\%s\%s' % (fake.SHARE_ADDRESS_2, fake.SHARE_NAME),
r'\\%s%s' % (fake.SHARE_ADDRESS_1, fake.CIFS_SHARE_PATH_PARSED),
r'\\%s%s' % (fake.SHARE_ADDRESS_2, fake.CIFS_SHARE_PATH_PARSED),
]
self.assertEqual(expected_paths, export_paths)
@ -66,7 +68,7 @@ class NetAppClusteredCIFSHelperTestCase(test.TestCase):
self.mock_client.remove_cifs_share.assert_not_called()
else:
self.mock_client.create_cifs_share.assert_called_once_with(
fake.SHARE_NAME)
fake.SHARE_NAME, fake.CIFS_SHARE_PATH)
self.mock_client.remove_cifs_share_access.assert_called_once_with(
fake.SHARE_NAME, 'Everyone')
@ -214,9 +216,24 @@ class NetAppClusteredCIFSHelperTestCase(test.TestCase):
def test_get_share_name_for_share(self):
self.mock_client.get_volume_at_junction_path.return_value = (
fake.VOLUME)
share_name = self.helper.get_share_name_for_share(fake.CIFS_SHARE)
self.assertEqual(fake.SHARE_NAME, share_name)
self.mock_client.get_volume_at_junction_path.assert_called_once_with(
fake.CIFS_SHARE_PATH)
def test_get_share_name_for_share_not_found(self):
self.mock_client.get_volume_at_junction_path.return_value = None
share_name = self.helper.get_share_name_for_share(fake.CIFS_SHARE)
self.assertIsNone(share_name)
self.mock_client.get_volume_at_junction_path.assert_called_once_with(
fake.CIFS_SHARE_PATH)
@ddt.data(
{

8
releasenotes/notes/bug-1920937-fixed-cifs-share-migration-752fde9631fb077a.yaml Normal file
View File

@ -0,0 +1,8 @@
---
fixes:
- |
Fixed non-disruptive share migration of CIFS shares in the NetApp ONTAP
driver using ZAPI API. During the CIFS share migration the creation of a
new export path is skipped and the actual export path is taken from the
backend. For more details, please refer to
`launchpad bug #1920937 <https://bugs.launchpad.net/manila/+bug/1920937>`_.