cDOT multi-SVM driver may choose unsuitable physical port for LIFs

The algorithm used by the NetApp cDOT multi-SVM driver for choosing
a physical port on which to create logical interfaces (LIFs) is too
simplistic, which can result in the selection of ports that won't
work at all or which are far inferior to other choices.  This patch
improves the port selection query, sorts ports by speed, and allows
the user to override the port selection via a regex setting in the
manila.conf file.

Closes-Bug: #1434918
Change-Id: Idedcb0db23df3a745f0c1ced2652da008746f0a6

manila/share/drivers/netapp/dataontap/client/client_cmode.py

@@ -252,26 +252,68 @@ class NetAppCmodeClient(client_base.NetAppBaseClient):
                 in nodes_info_list.get_children()]
 
     @na_utils.trace
-    def get_node_data_port(self, node):
+    def list_node_data_ports(self, node):
-        """Get data port on the node."""
+        ports = self.get_node_data_ports(node)
+        return [port.get('port') for port in ports]
+
+    @na_utils.trace
+    def get_node_data_ports(self, node):
+        """Get applicable data ports on the node."""
         api_args = {
             'query': {
                 'net-port-info': {
                     'node': node,
-                    'port-type': 'physical',
+                    'link-status': 'up',
+                    'port-type': 'physical|if_group',
                     'role': 'data',
                 },
             },
+            'desired-attributes': {
+                'node-details-info': {
+                    'port': None,
+                    'node': None,
+                    'operational-speed': None,
+                    'ifgrp-port': None,
+                },
+            },
         }
-        port_info = self.send_request('net-port-get-iter', api_args)
+        result = self.send_request('net-port-get-iter', api_args)
-        try:
+        net_port_info_list = result.get_child_by_name(
-            port = port_info.get_child_by_name(
+            'attributes-list') or netapp_api.NaElement('none')
-                'attributes-list').get_child_by_name(
+
-                    'net-port-info').get_child_content('port')
+        ports = []
-        except AttributeError:
+        for port_info in net_port_info_list.get_children():
-            msg = _("Data port does not exist for node %s.")
+
-            raise exception.NetAppException(msg % node)
+            # Skip physical ports that are part of interface groups.
-        return port
+            if port_info.get_child_content('ifgrp-port'):
+                continue
+
+            port = {
+                'node': port_info.get_child_content('node'),
+                'port': port_info.get_child_content('port'),
+                'speed': port_info.get_child_content('operational-speed'),
+            }
+            ports.append(port)
+
+        return self._sort_data_ports_by_speed(ports)
+
+    @na_utils.trace
+    def _sort_data_ports_by_speed(self, ports):
+
+        def sort_key(port):
+            value = port.get('speed')
+            if not (value and isinstance(value, six.string_types)):
+                return 0
+            elif value.isdigit():
+                return int(value)
+            elif value == 'auto':
+                return 3
+            elif value == 'undef':
+                return 2
+            else:
+                return 1
+
+        return sorted(ports, key=sort_key, reverse=True)
 
     @na_utils.trace
     def list_aggregates(self):

manila/share/drivers/netapp/dataontap/cluster_mode/lib_multi_svm.py

@@ -28,7 +28,6 @@ from oslo_utils import excutils
 from manila import context
 from manila import exception
 from manila.i18n import _, _LE, _LW
-from manila.share.drivers.netapp.dataontap.client import api as netapp_api
 from manila.share.drivers.netapp.dataontap.cluster_mode import lib_base
 from manila.share.drivers.netapp import utils as na_utils
 from manila import utils
@@ -121,7 +120,7 @@ class NetAppCmodeMultiSVMFileStorageLibrary(
             self._create_vserver_lifs(vserver_name,
                                       vserver_client,
                                       network_info)
-        except netapp_api.NaApiError:
+        except Exception:
             with excutils.save_and_reraise_exception():
                 LOG.error(_LE("Failed to create network interface(s)."))
                 self._client.delete_vserver(vserver_name, vserver_client)
@@ -145,7 +144,7 @@ class NetAppCmodeMultiSVMFileStorageLibrary(
 
         for node, net_info in node_network_info:
             net_id = net_info['id']
-            port = self._client.get_node_data_port(node)
+            port = self._get_node_data_port(node)
             ip = net_info['ip_address']
             self._create_lif_if_nonexistent(vserver_name,
                                             net_id,
@@ -156,6 +155,18 @@ class NetAppCmodeMultiSVMFileStorageLibrary(
                                             netmask,
                                             vserver_client)
 
+    @na_utils.trace
+    def _get_node_data_port(self, node):
+        port_names = self._client.list_node_data_ports(node)
+        pattern = self.configuration.netapp_port_name_search_pattern
+        matched_port_names = [port_name for port_name in port_names
+                              if re.match(pattern, port_name)]
+        if not matched_port_names:
+            raise exception.NetAppException(
+                _('Could not find eligible network ports on node %s on which '
+                  'to create Vserver LIFs.') % node)
+        return matched_port_names[0]
+
     @na_utils.trace
     def _create_lif_if_nonexistent(self, vserver_name, allocation_id, vlan,
                                    node, port, ip, netmask, vserver_client):

manila/share/drivers/netapp/options.py

@@ -69,6 +69,10 @@ netapp_provisioning_opts = [
     cfg.StrOpt('netapp_vserver_name_template',
                default='os_%s',
                help='Name template to use for new Vserver.'),
+    cfg.StrOpt('netapp_port_name_search_pattern',
+               default='(.*)',
+               help='Pattern for overriding the selection of network ports '
+                    'on which to create Vserver LIFs.'),
     cfg.StrOpt('netapp_lif_name_template',
                default='os_%(net_allocation_id)s',
                help='Logical interface (LIF) name template'),

manila/tests/share/drivers/netapp/dataontap/client/fakes.py

@@ -327,7 +327,7 @@ NET_PORT_GET_ITER_RESPONSE = etree.XML("""
         <node>%(node_name)s</node>
         <operational-duplex>full</operational-duplex>
         <operational-flowcontrol>none</operational-flowcontrol>
-        <operational-speed>1000</operational-speed>
+        <operational-speed>10</operational-speed>
         <port>e0a</port>
         <port-type>physical</port-type>
         <role>data</role>
@@ -345,7 +345,7 @@ NET_PORT_GET_ITER_RESPONSE = etree.XML("""
         <node>%(node_name)s</node>
         <operational-duplex>full</operational-duplex>
         <operational-flowcontrol>none</operational-flowcontrol>
-        <operational-speed>1000</operational-speed>
+        <operational-speed>100</operational-speed>
         <port>e0b</port>
         <port-type>physical</port-type>
         <role>data</role>
@@ -381,7 +381,7 @@ NET_PORT_GET_ITER_RESPONSE = etree.XML("""
         <node>%(node_name)s</node>
         <operational-duplex>full</operational-duplex>
         <operational-flowcontrol>none</operational-flowcontrol>
-        <operational-speed>1000</operational-speed>
+        <operational-speed>10000</operational-speed>
         <port>e0d</port>
         <port-type>physical</port-type>
         <role>data</role>
@@ -391,7 +391,34 @@ NET_PORT_GET_ITER_RESPONSE = etree.XML("""
   </results>
 """ % {'node_name': NODE_NAME})
 
-PORTS = ('e0a', 'e0b', 'e0c', 'e0d')
+SPEED_SORTED_PORTS = (
+    {'node': NODE_NAME, 'port': 'e0d', 'speed': '10000'},
+    {'node': NODE_NAME, 'port': 'e0c', 'speed': '1000'},
+    {'node': NODE_NAME, 'port': 'e0b', 'speed': '100'},
+    {'node': NODE_NAME, 'port': 'e0a', 'speed': '10'},
+)
+PORT_NAMES = ('e0a', 'e0b', 'e0c', 'e0d')
+SPEED_SORTED_PORT_NAMES = ('e0d', 'e0c', 'e0b', 'e0a')
+
+UNSORTED_PORTS_ALL_SPEEDS = (
+    {'node': NODE_NAME, 'port': 'port6', 'speed': 'undef'},
+    {'node': NODE_NAME, 'port': 'port3', 'speed': '100'},
+    {'node': NODE_NAME, 'port': 'port1', 'speed': '10000'},
+    {'node': NODE_NAME, 'port': 'port4', 'speed': '10'},
+    {'node': NODE_NAME, 'port': 'port7'},
+    {'node': NODE_NAME, 'port': 'port2', 'speed': '1000'},
+    {'node': NODE_NAME, 'port': 'port5', 'speed': 'auto'},
+)
+
+SORTED_PORTS_ALL_SPEEDS = (
+    {'node': NODE_NAME, 'port': 'port1', 'speed': '10000'},
+    {'node': NODE_NAME, 'port': 'port2', 'speed': '1000'},
+    {'node': NODE_NAME, 'port': 'port3', 'speed': '100'},
+    {'node': NODE_NAME, 'port': 'port4', 'speed': '10'},
+    {'node': NODE_NAME, 'port': 'port5', 'speed': 'auto'},
+    {'node': NODE_NAME, 'port': 'port6', 'speed': 'undef'},
+    {'node': NODE_NAME, 'port': 'port7'},
+)
 
 NET_INTERFACE_GET_ITER_RESPONSE = etree.XML("""
   <results status="passed">

manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py

@@ -455,27 +455,65 @@ class NetAppClientCmodeTestCase(test.TestCase):
 
         self.assertListEqual([], result)
 
-    def test_get_node_data_port(self):
+    def test_list_node_data_ports(self):
+
+        self.mock_object(self.client,
+                         'get_node_data_ports',
+                         mock.Mock(return_value=fake.SPEED_SORTED_PORTS))
+
+        result = self.client.list_node_data_ports(fake.NODE_NAME)
+
+        self.assertSequenceEqual(fake.SPEED_SORTED_PORT_NAMES, result)
+
+    def test_get_node_data_ports(self):
 
         api_response = netapp_api.NaElement(fake.NET_PORT_GET_ITER_RESPONSE)
         self.mock_object(self.client,
                          'send_request',
                          mock.Mock(return_value=api_response))
 
-        result = self.client.get_node_data_port(fake.NODE_NAME)
+        result = self.client.get_node_data_ports(fake.NODE_NAME)
 
-        self.assertEqual(fake.PORTS[0], result)
+        net_port_get_iter_args = {
+            'query': {
+                'net-port-info': {
+                    'node': fake.NODE_NAME,
+                    'link-status': 'up',
+                    'port-type': 'physical|if_group',
+                    'role': 'data',
+                },
+            },
+            'desired-attributes': {
+                'node-details-info': {
+                    'port': None,
+                    'node': None,
+                    'operational-speed': None,
+                    'ifgrp-port': None,
+                },
+            },
+        }
 
-    def test_get_node_data_port_not_found(self):
+        self.assertSequenceEqual(fake.SPEED_SORTED_PORTS, result)
+        self.client.send_request.assert_has_calls([
+            mock.call('net-port-get-iter', net_port_get_iter_args)])
+
+    def test_get_node_data_ports_not_found(self):
 
         api_response = netapp_api.NaElement(fake.NO_RECORDS_RESPONSE)
         self.mock_object(self.client,
                          'send_request',
                          mock.Mock(return_value=api_response))
 
-        self.assertRaises(exception.NetAppException,
+        result = self.client.get_node_data_ports(fake.NODE_NAME)
-                          self.client.get_node_data_port,
+
-                          fake.NODE_NAME)
+        self.assertSequenceEqual([], result)
+
+    def test_sort_data_ports_by_speed(self):
+
+        result = self.client._sort_data_ports_by_speed(
+            fake.UNSORTED_PORTS_ALL_SPEEDS)
+
+        self.assertSequenceEqual(fake.SORTED_PORTS_ALL_SPEEDS, result)
 
     def test_list_aggregates(self):
 

manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_multi_svm.py

@@ -17,6 +17,7 @@ Unit tests for the NetApp Data ONTAP cDOT multi-SVM storage driver library.
 
 import copy
 
+import ddt
 import mock
 from oslo_log import log
 
@@ -30,6 +31,7 @@ from manila import test
 from manila.tests.share.drivers.netapp.dataontap import fakes as fake
 
 
+@ddt.ddt
 class NetAppFileStorageLibraryTestCase(test.TestCase):
 
     def setUp(self):
@@ -237,7 +239,9 @@ class NetAppFileStorageLibraryTestCase(test.TestCase):
             vserver_id,
             {'vserver_name': vserver_name})
 
-    def test_create_vserver_if_nonexistent_lif_creation_failure(self):
+    @ddt.data(netapp_api.NaApiError, exception.NetAppException)
+    def test_create_vserver_if_nonexistent_lif_creation_failure(self,
+                                                                lif_exception):
 
         vserver_id = fake.NETWORK_INFO['server_id']
         vserver_name = fake.VSERVER_NAME_TEMPLATE % vserver_id
@@ -257,9 +261,9 @@ class NetAppFileStorageLibraryTestCase(test.TestCase):
                          mock.Mock(return_value=fake.AGGREGATES))
         self.mock_object(self.library,
                          '_create_vserver_lifs',
-                         mock.Mock(side_effect=netapp_api.NaApiError))
+                         mock.Mock(side_effect=lif_exception))
 
-        self.assertRaises(netapp_api.NaApiError,
+        self.assertRaises(lif_exception,
                           self.library._create_vserver_if_nonexistent,
                           fake.NETWORK_INFO)
 
@@ -284,8 +288,8 @@ class NetAppFileStorageLibraryTestCase(test.TestCase):
         self.mock_object(self.library._client,
                          'list_cluster_nodes',
                          mock.Mock(return_value=fake.CLUSTER_NODES))
-        self.mock_object(self.library._client,
+        self.mock_object(self.library,
-                         'get_node_data_port',
+                         '_get_node_data_port',
                          mock.Mock(return_value=fake.NODE_DATA_PORT))
         self.mock_object(self.library, '_create_lif_if_nonexistent')
 
@@ -313,6 +317,30 @@ class NetAppFileStorageLibraryTestCase(test.TestCase):
                 fake.NETWORK_INFO_NETMASK,
                 'fake_vserver_client')])
 
+    def test_get_node_data_port(self):
+
+        self.mock_object(self.client,
+                         'list_node_data_ports',
+                         mock.Mock(return_value=fake.NODE_DATA_PORTS))
+        self.library.configuration.netapp_port_name_search_pattern = 'e0c'
+
+        result = self.library._get_node_data_port(fake.CLUSTER_NODE)
+
+        self.assertEqual('e0c', result)
+        self.library._client.list_node_data_ports.assert_has_calls([
+            mock.call(fake.CLUSTER_NODE)])
+
+    def test_get_node_data_port_no_match(self):
+
+        self.mock_object(self.client,
+                         'list_node_data_ports',
+                         mock.Mock(return_value=fake.NODE_DATA_PORTS))
+        self.library.configuration.netapp_port_name_search_pattern = 'ifgroup1'
+
+        self.assertRaises(exception.NetAppException,
+                          self.library._get_node_data_port,
+                          fake.CLUSTER_NODE)
+
     def test_create_lif_if_nonexistent(self):
 
         vserver_client = mock.Mock()

manila/tests/share/drivers/netapp/dataontap/fakes.py

@@ -41,8 +41,10 @@ TOTAL_CAPACITY = 20000000000
 AGGREGATES = ('manila_aggr_1', 'manila_aggr_2')
 ROOT_VOLUME_AGGREGATE = 'manila1'
 ROOT_VOLUME = 'root'
+CLUSTER_NODE = 'cluster1_01'
 CLUSTER_NODES = ('cluster1_01', 'cluster1_02')
 NODE_DATA_PORT = 'e0c'
+NODE_DATA_PORTS = ('e0c', 'e0d')
 LIF_NAME_TEMPLATE = 'os_%(net_allocation_id)s'
 SHARE_TYPE_ID = '26e89a5b-960b-46bb-a8cf-0778e653098f'
 SHARE_TYPE_NAME = 'fake_share_type'

manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py

@@ -15,6 +15,7 @@
 Mock unit tests for the NetApp driver protocols CIFS class module.
 """
 
+import ddt
 import mock
 from oslo_log import log
 
@@ -26,6 +27,7 @@ from manila.tests.share.drivers.netapp.dataontap.protocols \
     import fakes as fake
 
 
+@ddt.ddt
 class NetAppClusteredCIFSHelperTestCase(test.TestCase):
 
     def setUp(self):
@@ -169,28 +171,26 @@ class NetAppClusteredCIFSHelperTestCase(test.TestCase):
         target = self.helper.get_target({'export_location': ''})
         self.assertEqual('', target)
 
-    def test_get_export_location(self):
+    @ddt.data(
+        {
+            'location': r'\\%s\%s' % (fake.SHARE_ADDRESS_1, fake.SHARE_NAME),
+            'ip': fake.SHARE_ADDRESS_1,
+            'share_name': fake.SHARE_NAME,
+        }, {
+            'location': r'//%s/%s' % (fake.SHARE_ADDRESS_1, fake.SHARE_NAME),
+            'ip': fake.SHARE_ADDRESS_1,
+            'share_name': fake.SHARE_NAME,
+        },
+        {'location': '', 'ip': '', 'share_name': ''},
+        {'location': 'invalid', 'ip': '', 'share_name': ''},
+    )
+    @ddt.unpack
+    def test_get_export_location(self, location, ip, share_name):
 
-        host_ip, share_name = self.helper._get_export_location(fake.CIFS_SHARE)
+        share = fake.CIFS_SHARE.copy()
-        self.assertEqual(fake.SHARE_ADDRESS_1, host_ip)
+        share['export_location'] = location
-        self.assertEqual(fake.SHARE_NAME, share_name)
 
-    def test_get_export_location_legacy_forward_slashes(self):
+        result_ip, result_share_name = self.helper._get_export_location(share)
 
-        fake_share = fake.CIFS_SHARE.copy()
+        self.assertEqual(ip, result_ip)
-        fake_share['export_location'] = fake_share['export_location'].replace(
+        self.assertEqual(share_name, result_share_name)
-            '\\', '/')
-
-        host_ip, share_name = self.helper._get_export_location(fake_share)
-        self.assertEqual(fake.SHARE_ADDRESS_1, host_ip)
-        self.assertEqual(fake.SHARE_NAME, share_name)
-
-    def test_get_export_location_missing_location(self):
-
-        fake_share = fake.CIFS_SHARE.copy()
-        fake_share['export_location'] = ''
-
-        host_ip, share_name = self.helper._get_export_location(fake_share)
-
-        self.assertEqual('', host_ip)
-        self.assertEqual('', share_name)