From 29c79884e9f714b88e0d7fa015e4770fec6b70f5 Mon Sep 17 00:00:00 2001 From: naved001 Date: Fri, 22 Sep 2017 13:31:10 -0400 Subject: [PATCH 1/8] Introduce a method in the switch drivers that check if an operation is legal * this method is called by the API right before it queues any networking action. * for dell nos9 based switches, the checks ensures that a native network is always attached first before any trunked vlans, and the native network is also the last network to be removed. * also turn off the port when revert_port is called. * update get_port_info to turn on the port before querying it. --- hil/api.py | 7 +++++++ hil/ext/switches/dellnos9.py | 26 ++++++++++++++++++++++++-- hil/model.py | 12 ++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/hil/api.py b/hil/api.py index 2043dd79..42d921b4 100644 --- a/hil/api.py +++ b/hil/api.py @@ -420,6 +420,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, @@ -458,6 +461,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', None) + db.session.add(model.NetworkingAction(type='modify_port', nic=nic, channel=attachment.channel, diff --git a/hil/ext/switches/dellnos9.py b/hil/ext/switches/dellnos9.py index fc23ba5b..ad0cb139 100644 --- a/hil/ext/switches/dellnos9.py +++ b/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,24 @@ def validate(kwargs): def session(self): return self + def ensure_legal_operation(self, nic, op_type, channel): + table = model.NetworkAttachment + if channel != 'vlan/native' and op_type == 'connect' and \ + db.session.query(table).filter(table.nic_id == nic.id) \ + .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 is None and op_type == 'detach' and \ + db.session.query(table).filter(table.nic_id == nic.id) \ + .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 +100,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 +119,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 = {} @@ -179,7 +200,8 @@ 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#\n\n" """ - + if not self._is_port_on(interface): + self._port_on(interface) command = 'interfaces switchport %s %s' % \ (self.interface_type, interface) response = self._execute(interface, SHOW, command) diff --git a/hil/model.py b/hil/model.py index b4b70937..bc092cc1 100644 --- a/hil/model.py +++ b/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 + interface is Port 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. From 414ecd27886deeff80c887be04b088d53a45ce3d Mon Sep 17 00:00:00 2001 From: naved001 Date: Fri, 22 Sep 2017 14:12:39 -0400 Subject: [PATCH 2/8] Factor out the database query * update the docstring --- hil/ext/switches/dellnos9.py | 9 +++++---- hil/model.py | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/hil/ext/switches/dellnos9.py b/hil/ext/switches/dellnos9.py index ad0cb139..3a24afa9 100644 --- a/hil/ext/switches/dellnos9.py +++ b/hil/ext/switches/dellnos9.py @@ -63,16 +63,17 @@ def session(self): return self def ensure_legal_operation(self, nic, op_type, channel): + # get the network attachments for 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 \ - db.session.query(table).filter(table.nic_id == nic.id) \ - .filter(table.channel == 'vlan/native').count() == 0: + 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 is None and op_type == 'detach' and \ - db.session.query(table).filter(table.nic_id == nic.id) \ - .filter(table.channel != 'vlan/native').count() > 0: + 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" diff --git a/hil/model.py b/hil/model.py index bc092cc1..d5d06797 100644 --- a/hil/model.py +++ b/hil/model.py @@ -250,7 +250,7 @@ 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 - interface is Port object + 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 From c6622b2c5c56f3bf7969114b873bd564357404aa Mon Sep 17 00:00:00 2001 From: naved001 Date: Fri, 22 Sep 2017 16:05:32 -0400 Subject: [PATCH 3/8] Add unit tests for ensure_legal_operations * And also fix a bug that I came across while writing the unit tests. --- hil/api.py | 2 +- hil/ext/switches/dellnos9.py | 2 +- tests/unit/ext/switches/dellnos9.py | 104 ++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 tests/unit/ext/switches/dellnos9.py diff --git a/hil/api.py b/hil/api.py index 42d921b4..6db730f0 100644 --- a/hil/api.py +++ b/hil/api.py @@ -463,7 +463,7 @@ def node_detach_network(node, nic, network): "The network is not attached to the nic.") switch = nic.port.owner - switch.ensure_legal_operation(nic, 'detach', None) + switch.ensure_legal_operation(nic, 'detach', attachment.channel) db.session.add(model.NetworkingAction(type='modify_port', nic=nic, diff --git a/hil/ext/switches/dellnos9.py b/hil/ext/switches/dellnos9.py index 3a24afa9..47fdfe55 100644 --- a/hil/ext/switches/dellnos9.py +++ b/hil/ext/switches/dellnos9.py @@ -72,7 +72,7 @@ def ensure_legal_operation(self, nic, op_type, channel): # 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 is None and op_type == 'detach' and \ + 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. diff --git a/tests/unit/ext/switches/dellnos9.py b/tests/unit/ext/switches/dellnos9.py new file mode 100644 index 00000000..59f4522c --- /dev/null +++ b/tests/unit/ext/switches/dellnos9.py @@ -0,0 +1,104 @@ +# Copyright 2016 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. +"""Tests for the brocade switch driver""" + +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 test_ensure_legal_operations(): + """qweqweqwe""" + + # create a project and a network + api.project_create('anvil-nextgen') + network_create_simple('hammernet', '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.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') + network = api._must_find(model.Network, 'hammernet') + + # connecting a trunked network wihtout having a native should fail + with pytest.raises(BlockedError): + switch.ensure_legal_operation(nic, 'connect', 'vlan/1212') + + # put a trunked network in the database, and then try to remove native net + db.session.add(model.NetworkAttachment( + nic=nic, + network=network, + channel='vlan/1212')) + + with pytest.raises(BlockedError): + switch.ensure_legal_operation(nic, 'detach', 'vlan/native') \ No newline at end of file From 3e9ca11628965583ea389cde20c2240b572390cf Mon Sep 17 00:00:00 2001 From: naved001 Date: Fri, 22 Sep 2017 16:15:08 -0400 Subject: [PATCH 4/8] update the test docstring and copyright date --- tests/unit/ext/switches/dellnos9.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/ext/switches/dellnos9.py b/tests/unit/ext/switches/dellnos9.py index 59f4522c..9bfe193d 100644 --- a/tests/unit/ext/switches/dellnos9.py +++ b/tests/unit/ext/switches/dellnos9.py @@ -1,4 +1,4 @@ -# Copyright 2016 Massachusetts Open Cloud Contributors +# 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 @@ -11,7 +11,7 @@ # 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. -"""Tests for the brocade switch driver""" +"""Unit tests for dell switches running Dell Networking OS 9 (with REST API)""" import pytest @@ -62,7 +62,7 @@ def configure(): def test_ensure_legal_operations(): - """qweqweqwe""" + """Test to ensure that ensure_legal_operations works as expected""" # create a project and a network api.project_create('anvil-nextgen') @@ -101,4 +101,4 @@ def test_ensure_legal_operations(): channel='vlan/1212')) with pytest.raises(BlockedError): - switch.ensure_legal_operation(nic, 'detach', 'vlan/native') \ No newline at end of file + switch.ensure_legal_operation(nic, 'detach', 'vlan/native') From 001c410e2f83d9aad60640f89da8a9bae2301d12 Mon Sep 17 00:00:00 2001 From: naved001 Date: Fri, 22 Sep 2017 17:29:33 -0400 Subject: [PATCH 5/8] Add test to ensure that the error is raised from within the API too. --- tests/unit/ext/switches/dellnos9.py | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/tests/unit/ext/switches/dellnos9.py b/tests/unit/ext/switches/dellnos9.py index 9bfe193d..78024c13 100644 --- a/tests/unit/ext/switches/dellnos9.py +++ b/tests/unit/ext/switches/dellnos9.py @@ -67,6 +67,7 @@ def test_ensure_legal_operations(): # 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', @@ -84,21 +85,34 @@ def test_ensure_legal_operations(): "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') - network = api._must_find(model.Network, 'hammernet') - # connecting a trunked network wihtout having a native should fail + # 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') - # put a trunked network in the database, and then try to remove native net + with pytest.raises(BlockedError): + api.node_connect_network('compute-01', 'eth0', 'hammernet', 'vlan/40') + + # put a trunked and native network in the database, and then try to remove + # the native network first db.session.add(model.NetworkAttachment( nic=nic, - network=network, - channel='vlan/1212')) + network=api._must_find(model.Network, 'hammernet'), + channel='vlan/native')) + + db.session.add(model.NetworkAttachment( + nic=nic, + network=api._must_find(model.Network, 'pineapple'), + channel='vlan/40')) with pytest.raises(BlockedError): switch.ensure_legal_operation(nic, 'detach', 'vlan/native') + + with pytest.raises(BlockedError): + api.node_detach_network('compute-01', 'eth0', 'hammernet') From 77a5f347f1d5b09d126e87fd613431957c5ff907 Mon Sep 17 00:00:00 2001 From: naved001 Date: Mon, 25 Sep 2017 10:07:10 -0400 Subject: [PATCH 6/8] Add some positive tests for the API * add a method that mocks networking actions. usefull for testing without a real switch. --- tests/unit/ext/switches/dellnos9.py | 49 ++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/tests/unit/ext/switches/dellnos9.py b/tests/unit/ext/switches/dellnos9.py index 78024c13..2648129a 100644 --- a/tests/unit/ext/switches/dellnos9.py +++ b/tests/unit/ext/switches/dellnos9.py @@ -61,6 +61,30 @@ def configure(): 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""" @@ -99,20 +123,23 @@ def test_ensure_legal_operations(): with pytest.raises(BlockedError): api.node_connect_network('compute-01', 'eth0', 'hammernet', 'vlan/40') - # put a trunked and native network in the database, and then try to remove - # the native network first - db.session.add(model.NetworkAttachment( - nic=nic, - network=api._must_find(model.Network, 'hammernet'), - channel='vlan/native')) - - db.session.add(model.NetworkAttachment( - nic=nic, - network=api._must_find(model.Network, 'pineapple'), - channel='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() From 1431dfaf1b5d2dc4b5790929db95965028777f45 Mon Sep 17 00:00:00 2001 From: naved001 Date: Mon, 25 Sep 2017 10:58:01 -0400 Subject: [PATCH 7/8] Return default values for a port if it's off * In method _get_vlans, and _get_native_vlan, if the port is off, we simply return the standard defauts (that is, no vlans) instead of turning it on and then querying it. This is safe to do because a switchport can be in off state if it's not a part of any non-default vlans (i.e. all except vlan 1). --- hil/ext/switches/dellnos9.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hil/ext/switches/dellnos9.py b/hil/ext/switches/dellnos9.py index 47fdfe55..7e2e83d7 100644 --- a/hil/ext/switches/dellnos9.py +++ b/hil/ext/switches/dellnos9.py @@ -150,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) @@ -168,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: @@ -201,8 +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#\n\n" """ - if not self._is_port_on(interface): - self._port_on(interface) command = 'interfaces switchport %s %s' % \ (self.interface_type, interface) response = self._execute(interface, SHOW, command) From 8811824b651da581630d1035f30070888c6fffc9 Mon Sep 17 00:00:00 2001 From: naved001 Date: Mon, 25 Sep 2017 14:14:26 -0400 Subject: [PATCH 8/8] add bit about HIL taking care of turning on/off the port in the docs * and about how this switch requires a native vlan first before any trunked vlans are allowed to be attached. --- docs/network-drivers.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/network-drivers.md b/docs/network-drivers.md index 97510f99..6a57639b 100644 --- a/docs/network-drivers.md +++ b/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