Merge "Fix "preferred" export locations in the CephFS driver"

manila/share/drivers/cephfs/driver.py

@@ -1256,8 +1256,9 @@ class NFSProtocolHelperMixin():
             export_location = {
                 'path': export_path,
                 'is_admin_only': False,
-                'metadata': {},
+                'metadata': {
-                'preferred': export_ip['preferred']
+                    'preferred': export_ip['preferred'],
+                },
             }
             export_locations.append(export_location)
         return export_locations
@@ -1407,7 +1408,7 @@ class NFSProtocolHelper(NFSProtocolHelperMixin, ganesha.GaneshaNASHelper2):
             ganesha_export_ips = [self.ganesha_host]
 
         export_ips = []
-        for ip in ganesha_export_ips:
+        for ip in set(ganesha_export_ips):
             export_ips.append({'ip': ip, 'preferred': False})
 
         return export_ips
@@ -1466,7 +1467,7 @@ class NFSClusterProtocolHelper(NFSProtocolHelperMixin, ganesha.NASHelperBase):
             ganesha_server_ips = (
                 [ganesha_server_ips] if ganesha_server_ips else [])
 
-        return ganesha_server_ips
+        return set(ganesha_server_ips)
 
     def _get_export_ips(self):
         """Get NFS cluster export ips."""
@@ -1501,13 +1502,15 @@ class NFSClusterProtocolHelper(NFSProtocolHelperMixin, ganesha.NASHelperBase):
             raise exception.ShareBackendException(msg=msg)
 
         export_ips = []
-        for ip in ceph_nfs_export_ips:
+        for ip in set(ceph_nfs_export_ips):
             export_ips.append({'ip': ip, 'preferred': True})
 
         # It's possible for deployers to state additional
         # NFS interfaces directly via manila.conf. If they do,
         # these are represented as non-preferred export paths.
         # This is mostly to allow NFS-Ganesha server migrations.
+        ganesha_export_ips = (eip for eip in ganesha_export_ips
+                              if eip not in ceph_nfs_export_ips)
         for ip in ganesha_export_ips:
             export_ips.append({'ip': ip, 'preferred': False})
 

manila/tests/share/drivers/cephfs/test_driver.py

@@ -1467,8 +1467,9 @@ class NFSProtocolHelperTestCase(test.TestCase):
             [{
                 'path': '1.2.3.4:/foo/bar',
                 'is_admin_only': False,
-                'metadata': {},
+                'metadata': {
-                'preferred': False
+                    'preferred': False,
+                },
             }], ret)
 
     def test_get_export_locations_with_export_ips_configured(self):
@@ -1491,25 +1492,28 @@ class NFSProtocolHelperTestCase(test.TestCase):
 
         ret = helper.get_export_locations(self._share, cephfs_subvolume_path)
 
-        self.assertEqual(
+        self._assertEqualListsOfObjects(
             [
                 {
                     'path': '127.0.0.1:/foo/bar',
                     'is_admin_only': False,
-                    'metadata': {},
+                    'metadata': {
-                    'preferred': False
+                        'preferred': False,
+                    },
                 },
                 {
                     'path': '[fd3f:c057:1192:1::1]:/foo/bar',
                     'is_admin_only': False,
-                    'metadata': {},
+                    'metadata': {
-                    'preferred': False
+                        'preferred': False,
+                    },
                 },
                 {
                     'path': '[::1]:/foo/bar',
                     'is_admin_only': False,
-                    'metadata': {},
+                    'metadata': {
-                    'preferred': False
+                        'preferred': False,
+                    },
                 },
             ], ret)
 
@@ -1802,13 +1806,15 @@ class NFSClusterProtocolHelperTestCase(test.TestCase):
         expected_export_locations = [{
             'path': '10.0.0.10:/foo/bar',
             'is_admin_only': False,
-            'metadata': {},
+            'metadata': {
-            'preferred': True
+                'preferred': True,
+            },
         }, {
             'path': '10.0.0.11:/foo/bar',
             'is_admin_only': False,
-            'metadata': {},
+            'metadata': {
-            'preferred': True
+                'preferred': True,
+            },
         }]
 
         export_locations = (
@@ -1819,17 +1825,19 @@ class NFSClusterProtocolHelperTestCase(test.TestCase):
             self._rados_client,
             cluster_info_prefix, cluster_info_dict)
 
-        self.assertEqual(expected_export_locations, export_locations)
+        self._assertEqualListsOfObjects(expected_export_locations,
+                                        export_locations)
+
+    @ddt.data('cephfs_ganesha_server_ip', 'cephfs_ganesha_export_ips')
+    def test_get_export_locations_ganesha_still_configured(self, confopt):
+        if confopt == 'cephfs_ganesha_server_ip':
+            val = '10.0.0.1'
+        else:
+            val = ['10.0.0.2', '10.0.0.3']
 
-    @ddt.data(
-        ('cephfs_ganesha_server_ip', '10.0.0.1'),
-        ('cephfs_ganesha_export_ips', ['10.0.0.2, 10.0.0.3'])
-    )
-    @ddt.unpack
-    def test_get_export_locations_ganesha_still_configured(self, opt, val):
         cluster_info_prefix = "nfs cluster info"
         nfs_clusterid = self._nfscluster_protocol_helper.nfs_clusterid
-        self.fake_conf.set_default(opt, val)
+        self.fake_conf.set_default(confopt, val)
 
         cluster_info_dict = {
             "cluster_id": nfs_clusterid,
@@ -1850,46 +1858,65 @@ class NFSClusterProtocolHelperTestCase(test.TestCase):
         driver.rados_command.return_value = json.dumps(cluster_info)
 
         fake_cephfs_subvolume_path = "/foo/bar"
-        expected_export_locations = [{
+        expected_export_locations = [
-            'path': '10.0.0.10:/foo/bar',
+            {
-            'is_admin_only': False,
+                'path': '10.0.0.10:/foo/bar',
-            'metadata': {},
+                'is_admin_only': False,
-            'preferred': True
+                'metadata': {
-        }, {
+                    'preferred': True,
-            'path': '10.0.0.11:/foo/bar',
+                },
-            'is_admin_only': False,
+            },
-            'metadata': {},
+            {
-            'preferred': True
+                'path': '10.0.0.11:/foo/bar',
-        }]
+                'is_admin_only': False,
+                'metadata': {
+                    'preferred': True,
+                },
+            },
+        ]
+
         if isinstance(val, list):
             for ip in val:
                 expected_export_locations.append(
                     {
                         'path': f'{ip}:/foo/bar',
                         'is_admin_only': False,
-                        'metadata': {},
+                        'metadata': {
-                        'preferred': False
+                            'preferred': False,
-                    }
+                        },
+                    },
                 )
         else:
             expected_export_locations.append(
                 {
                     'path': f'{val}:/foo/bar',
                     'is_admin_only': False,
-                    'metadata': {},
+                    'metadata': {
-                    'preferred': False
+                        'preferred': False,
+                    },
                 }
             )
 
+        expected_export_locations = sorted(
+            expected_export_locations,
+            key=lambda d: d['path']
+        )
         export_locations = (
             self._nfscluster_protocol_helper.get_export_locations(
-                self._share, fake_cephfs_subvolume_path))
+                self._share, fake_cephfs_subvolume_path)
+        )
+
+        actual_export_locations = sorted(
+            export_locations,
+            key=lambda d: d['path']
+        )
 
         driver.rados_command.assert_called_once_with(
             self._rados_client,
             cluster_info_prefix, cluster_info_dict)
 
-        self.assertEqual(expected_export_locations, export_locations)
+        self.assertEqual(expected_export_locations,
+                         actual_export_locations)
 
 
 @ddt.ddt

releasenotes/notes/bug-2053100-fix-ceph-driver-preferred-path-update-70147668e0f19c4d.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    The CephFS driver used to set the "preferred" export path incorrectly.
+    It is now fixed to set it as part of export location metadata.
+    See `Launchpad bug 2053100 <https://launchpad.net/bugs/2053100>`_
+    for more details.