Merge pull request #882 from naved001/legal-switch-ops

Introduce a method in the switch drivers that check if an operation i…

docs/network-drivers.md

@@ -229,6 +229,9 @@ more valid interface types.
 The switch's API server either runs on port 8008 (HTTP) or 8888 (HTTPS), so be
 sure to specify that in the ``hostname``.
 
+This switch must have a native VLAN connected first before having any trunked
+VLANs. The switchport is turned on only when a native VLAN is connected.
+
 If you have multiple types of ports on the same switch, register the switch
 multiple times with different parameters for ``interface_type``.
 
@@ -247,6 +250,10 @@ The body of the api call request will look like:
 It accepts interface names the same way they would be accepted in the console
 of the switch, ex. ``1/3``.
 
+When a port is registered, ensure that it is turned off (otherwise it might be
+sitting on a default native vlan). HIL will then take care of turning on/off
+the port.
+
 ### Using multiple switches
 
 Networks managed by HIL may span multiple switches. No special configuration

hil/api.py

@@ -419,6 +419,9 @@ def _have_attachment(nic, query):
         raise errors.BadArgumentError(
             "Channel %r, is not legal for this network." % channel)
 
+    switch = nic.port.owner
+    switch.ensure_legal_operation(nic, 'connect', channel)
+
     db.session.add(model.NetworkingAction(type='modify_port',
                                           nic=nic,
                                           new_network=network,
@@ -457,6 +460,10 @@ def node_detach_network(node, nic, network):
     if attachment is None:
         raise errors.BadArgumentError(
             "The network is not attached to the nic.")
+
+    switch = nic.port.owner
+    switch.ensure_legal_operation(nic, 'detach', attachment.channel)
+
     db.session.add(model.NetworkingAction(type='modify_port',
                                           nic=nic,
                                           channel=attachment.channel,

hil/ext/switches/dellnos9.py

@@ -23,8 +23,9 @@
 import requests
 import schema
 
+from hil import model
 from hil.model import db, Switch, SwitchSession
-from hil.errors import BadArgumentError
+from hil.errors import BadArgumentError, BlockedError
 from hil.model import BigIntegerType
 from hil.network_allocator import get_network_allocator
 
@@ -61,6 +62,25 @@ def validate(kwargs):
     def session(self):
         return self
 
+    def ensure_legal_operation(self, nic, op_type, channel):
+        # get the network attachments for <nic> from the database
+        table = model.NetworkAttachment
+        query = db.session.query(table).filter(table.nic_id == nic.id)
+
+        if channel != 'vlan/native' and op_type == 'connect' and \
+           query.filter(table.channel == 'vlan/native').count() == 0:
+            # checks if it is trying to attach a trunked network, and then in
+            # in the db see if nic does not have any networks attached natively
+            raise BlockedError("Please attach a native network first")
+        elif channel == 'vlan/native' and op_type == 'detach' and \
+                query.filter(table.channel != 'vlan/native').count() > 0:
+            # if it is detaching a network, then check in the database if there
+            # are any trunked vlans.
+            raise BlockedError("Please remove all trunked Vlans"
+                               " before removing the native vlan")
+        else:
+            return
+
     @staticmethod
     def validate_port_name(port):
         """Valid port names for this switch are of the form 1/0/1 or 1/2"""
@@ -81,6 +101,7 @@ def modify_port(self, port, channel, new_network):
         if channel == 'vlan/native':
             if new_network is None:
                 self._remove_native_vlan(interface)
+                self._port_shutdown(interface)
             else:
                 self._set_native_vlan(interface, new_network)
         else:
@@ -99,6 +120,7 @@ def revert_port(self, port):
         self._remove_all_vlans_from_trunk(port)
         if self._get_native_vlan(port) is not None:
             self._remove_native_vlan(port)
+        self._port_shutdown(port)
 
     def get_port_networks(self, ports):
         response = {}
@@ -128,6 +150,8 @@ def _get_vlans(self, interface):
         # worked reliably in the first place) and then find our interface there
         # which is not feasible.
 
+        if not self._is_port_on(interface):
+            return []
         response = self._get_port_info(interface)
         # finds a comma separated list of integers starting with "T"
         match = re.search(r'T(\d+)((,\d+)?)*', response)
@@ -146,6 +170,8 @@ def _get_native_vlan(self, interface):
 
         Similar to _get_vlans()
         """
+        if not self._is_port_on(interface):
+            return None
         response = self._get_port_info(interface)
         match = re.search(r'NativeVlanId:(\d+)\.', response)
         if match is not None:
@@ -179,7 +205,6 @@ def _get_port_info(self, interface):
         \r\n\r\n Native Vlan Id: 1512.\r\n\r\n\r\n\r\n
         MOC-Dell-S3048-ON#</command>\n</output>\n"
         """
-
         command = 'interfaces switchport %s %s' % \
             (self.interface_type, interface)
         response = self._execute(interface, SHOW, command)

hil/model.py

@@ -246,6 +246,18 @@ def session(self):
         and have ``session`` just return ``self``.
         """
 
+    def ensure_legal_operation(self, nic, op_type, channel):
+        """Checks with the switch if the operation is legal before queueing it.
+
+        channel is network channel
+        nic is Nic object
+        op_type is type of operation (connect, detach)
+
+        Some drivers don't need this check at all. So the default behaviour is
+        to just return"""
+
+        return
+
 
 class SwitchSession(object):
     """A session object for a switch.

tests/unit/ext/switches/dellnos9.py

@@ -0,0 +1,145 @@
+# Copyright 2017 Massachusetts Open Cloud Contributors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an "AS
+# IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+# express or implied. See the License for the specific language
+# governing permissions and limitations under the License.
+"""Unit tests for dell switches running Dell Networking OS 9 (with REST API)"""
+
+import pytest
+
+from hil import model, api, config
+from hil.model import db
+from hil.test_common import config_testsuite, config_merge, fresh_database, \
+    fail_on_log_warnings, with_request_context, server_init, \
+    network_create_simple
+from hil.errors import BlockedError
+
+fail_on_log_warnings = pytest.fixture(autouse=True)(fail_on_log_warnings)
+fresh_database = pytest.fixture(fresh_database)
+server_init = pytest.fixture(server_init)
+with_request_context = pytest.yield_fixture(with_request_context)
+
+SWITCH_TYPE = 'http://schema.massopencloud.org/haas/v0/switches/dellnos9'
+
+
+@pytest.fixture
+def configure():
+    """Configure HIL"""
+    config_testsuite()
+    config_merge({
+        'auth': {
+            'require_authentication': 'True',
+        },
+        'extensions': {
+            'hil.ext.auth.null': '',
+            'hil.ext.switches.dellnos9': '',
+            'hil.ext.obm.mock': '',
+            'hil.ext.network_allocators.null': None,
+            'hil.ext.network_allocators.vlan_pool': '',
+        },
+        'hil.ext.network_allocators.vlan_pool': {
+            'vlans': '40-80',
+        },
+    })
+    config.load_extensions()
+
+
+default_fixtures = ['fail_on_log_warnings',
+                    'configure',
+                    'fresh_database',
+                    'server_init',
+                    'with_request_context']
+
+pytestmark = pytest.mark.usefixtures(*default_fixtures)
+
+
+def mock_networking_action():
+    """performs the required db operations and clears up the networking action
+    queue, so that we can queue more items to test the api.the
+
+    This is useful because calling deferred.apply_networking would require a
+    real switch
+    """
+    action = db.session.query(model.NetworkingAction) \
+        .order_by(model.NetworkingAction.id).one_or_none()
+
+    if action.new_network is None:
+            db.session.query(model.NetworkAttachment) \
+                .filter_by(nic=action.nic, channel=action.channel)\
+                .delete()
+    else:
+            db.session.add(model.NetworkAttachment(
+                nic=action.nic,
+                network=action.new_network,
+                channel=action.channel))
+
+    db.session.delete(action)
+    db.session.commit()
+
+
+def test_ensure_legal_operations():
+    """Test to ensure that ensure_legal_operations works as expected"""
+
+    # create a project and a network
+    api.project_create('anvil-nextgen')
+    network_create_simple('hammernet', 'anvil-nextgen')
+    network_create_simple('pineapple', 'anvil-nextgen')
+
+    # register a switch of type dellnos9 and add a port to it
+    api.switch_register('s3048',
+                        type=SWITCH_TYPE,
+                        username="switch_user",
+                        password="switch_pass",
+                        hostname="switchname",
+                        interface_type="GigabitEthernet")
+    api.switch_register_port('s3048', '1/3')
+    switch = api._must_find(model.Switch, 's3048')
+
+    # register a ndoe and a nic
+    api.node_register('compute-01', obm={
+                  "type": "http://schema.massopencloud.org/haas/v0/obm/mock",
+                  "host": "ipmihost",
+                  "user": "root",
+                  "password": "tapeworm"})
+    api.project_connect_node('anvil-nextgen', 'compute-01')
+    api.node_register_nic('compute-01', 'eth0', 'DE:AD:BE:EF:20:14')
+    nic = api._must_find(model.Nic, 'eth0')
+
+    api.port_connect_nic('s3048', '1/3', 'compute-01', 'eth0')
+
+    # connecting a trunked network wihtout having a native should fail.
+    # call the method directly and test the API too.
+    with pytest.raises(BlockedError):
+        switch.ensure_legal_operation(nic, 'connect', 'vlan/1212')
+
+    with pytest.raises(BlockedError):
+        api.node_connect_network('compute-01', 'eth0', 'hammernet', 'vlan/40')
+
+    # doing these operations in the correct order, that is native network first
+    # and then trunked, should work.
+    api.node_connect_network('compute-01', 'eth0', 'hammernet', 'vlan/native')
+    mock_networking_action()
+    api.node_connect_network('compute-01', 'eth0', 'pineapple', 'vlan/41')
+    mock_networking_action()
+
+    # removing these networks in the wrong order should not work.
+    with pytest.raises(BlockedError):
+        switch.ensure_legal_operation(nic, 'detach', 'vlan/native')
+
+    with pytest.raises(BlockedError):
+        api.node_detach_network('compute-01', 'eth0', 'hammernet')
+
+    # removing networks in the right order should work
+    api.node_detach_network('compute-01', 'eth0', 'pineapple')
+    mock_networking_action()
+    api.node_detach_network('compute-01', 'eth0', 'hammernet')
+    mock_networking_action()
+    db.session.close()