From 399e3550886d091dcf022051da578d5a2c219654 Mon Sep 17 00:00:00 2001 From: Hamdy Khader Date: Tue, 18 Jun 2019 15:07:43 +0300 Subject: [PATCH] [Follow Up] OVS DPDK port representors support Pass VF MAC address to ovs to set in case of netdev VIFHostDevice. In case of DPDK representor port, Nova will pass through the VF PCI as a hostdev and os-vif should pass the MAC to ovs as it would be responsible for setting its MAC. Moreover, when not using dpdk, libvirt does the mac cleanup, since the VF is managed by libvirt, but when using DPDK, libvirt does not manage the VF so we need to cleanup MAC address in os-vif. Change-Id: I5368c318cc0cfd7b5644d3da0dccbce7a48d6a85 Closes-Bug: #1829734 --- os_vif/tests/unit/test_utils.py | 26 +++++++++++++ os_vif/utils.py | 18 +++++++++ vif_plug_ovs/ovs.py | 10 ++++- vif_plug_ovs/ovsdb/impl_vsctl.py | 7 +++- vif_plug_ovs/ovsdb/ovsdb_lib.py | 19 ++++++++++ .../tests/unit/ovsdb/test_impl_vsctl.py | 22 +++++++++++ .../tests/unit/ovsdb/test_ovsdb_lib.py | 38 +++++++++++++++++-- 7 files changed, 134 insertions(+), 6 deletions(-) create mode 100644 os_vif/tests/unit/test_utils.py create mode 100644 vif_plug_ovs/tests/unit/ovsdb/test_impl_vsctl.py diff --git a/os_vif/tests/unit/test_utils.py b/os_vif/tests/unit/test_utils.py new file mode 100644 index 00000000..c8cc909b --- /dev/null +++ b/os_vif/tests/unit/test_utils.py @@ -0,0 +1,26 @@ +# 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. + +import mock +import random + +from os_vif.tests.unit import base +from os_vif import utils + + +class TestGetRandomMac(base.TestCase): + + @mock.patch.object(random, 'getrandbits', return_value=0xa2) + def test_random_mac_generated(self, mock_rnd): + mac = utils.get_random_mac(['aa', 'bb', '00', 'dd', 'ee', 'ff']) + self.assertEqual('aa:bb:00:a2:a2:a2', mac) + mock_rnd.assert_called_with(8) diff --git a/os_vif/utils.py b/os_vif/utils.py index d74f96bb..4df75c2d 100644 --- a/os_vif/utils.py +++ b/os_vif/utils.py @@ -11,6 +11,24 @@ # under the License. +import random + + +def get_random_mac(base_mac): + """Get a random MAC address string of the specified base format. + The first 3 octets will remain unchanged and the others will be + randomly generated. + + :param base_mac: Base MAC address represented by an array of 6 strings/int + :returns: The MAC address string. + """ + + mac = [int(base_mac[0], 16), int(base_mac[1], 16), + int(base_mac[2], 16), random.getrandbits(8), + random.getrandbits(8), random.getrandbits(8)] + return ':'.join(["%02x" % byte for byte in mac]) + + def set_mask(data, mask): return data | mask diff --git a/vif_plug_ovs/ovs.py b/vif_plug_ovs/ovs.py index 9516f79f..0554195f 100644 --- a/vif_plug_ovs/ovs.py +++ b/vif_plug_ovs/ovs.py @@ -82,7 +82,15 @@ class OvsPlugin(plugin.PluginBase): cfg.BoolOpt('isolate_vif', default=False, help='Controls if VIF should be isolated when plugged ' 'to the ovs bridge. This should only be set to True ' - 'when using the neutron ovs ml2 agent.') + 'when using the neutron ovs ml2 agent.'), + cfg.StrOpt('cleanup_base_mac', + default='aa:16:3f:00:00:00', + help='The cleanup base MAC address to be used for removed ' + 'DPDK representor ports. ' + 'The first 3 octets will remain unchanged and the others ' + 'will be randomly generated . ' + 'A valid base MAC must have local assignment bit set and ' + 'multicast bit cleared in the first octet.') ) def __init__(self, config): diff --git a/vif_plug_ovs/ovsdb/impl_vsctl.py b/vif_plug_ovs/ovsdb/impl_vsctl.py index 9df1650f..a24fb04c 100644 --- a/vif_plug_ovs/ovsdb/impl_vsctl.py +++ b/vif_plug_ovs/ovsdb/impl_vsctl.py @@ -372,7 +372,12 @@ def _set_colval_args(*col_values): # op. Will try to find a better way to default this op to '=' for entry in col_values: if len(entry) == 2: - col, op, val = entry[0], '=', entry[1] + # Escape colons for MAC address since ovs-vsctl command tries + # to parse it as JSON and colons throw it off + if entry[0] == 'mac': + col, op, val = entry[0], '=', entry[1].replace(':', '\:') + else: + col, op, val = entry[0], '=', entry[1] else: col, op, val = entry if isinstance(val, collections.Mapping): diff --git a/vif_plug_ovs/ovsdb/ovsdb_lib.py b/vif_plug_ovs/ovsdb/ovsdb_lib.py index 2d5cc158..cb90552f 100644 --- a/vif_plug_ovs/ovsdb/ovsdb_lib.py +++ b/vif_plug_ovs/ovsdb/ovsdb_lib.py @@ -14,6 +14,7 @@ import sys from oslo_log import log as logging +from os_vif import utils as os_vif_utils from vif_plug_ovs import constants from vif_plug_ovs import linux_net from vif_plug_ovs.ovsdb import api as ovsdb_api @@ -29,6 +30,14 @@ class BaseOVS(object): self.connection = config.ovsdb_connection self.interface = config.ovsdb_interface self.ovsdb = ovsdb_api.get_instance(self) + self.cleanup_base_mac = config.cleanup_base_mac + + def _is_dpdk_representor_port(self, dev): + options = self.ovsdb.db_get('Interface', dev, 'options').execute() + if options.get("dpdk-devargs") and\ + "representor" in options.get("dpdk-devargs"): + return True + return False def _ovs_supports_mtu_requests(self): return self.ovsdb.has_table_column('Interface', 'mtu_request') @@ -101,6 +110,7 @@ class BaseOVS(object): PF_PCI=pf_pci, VF_NUM=vf_num) col_values.append(('options', {'dpdk-devargs': devargs_string})) + col_values.append(('mac', mac)) with self.ovsdb.transaction() as txn: txn.add(self.ovsdb.add_port(bridge, dev)) txn.add(self.ovsdb.db_set('Interface', dev, *col_values)) @@ -109,7 +119,16 @@ class BaseOVS(object): def update_ovs_vif_port(self, dev, mtu=None, interface_type=None): self.update_device_mtu(dev, mtu, interface_type=interface_type) + def _get_ovs_port_options(self, dev): + return self.ovsdb.db_get('Interface', dev, 'options').execute() + def delete_ovs_vif_port(self, bridge, dev, delete_netdev=True): + if self._is_dpdk_representor_port(dev): + # Cleanup dpdk representor MAC address by setting a random MAC + col_values = [('mac', os_vif_utils.get_random_mac( + self.cleanup_base_mac.split(':')))] + self.ovsdb.db_set('Interface', dev, *col_values).execute() + self.ovsdb.del_port(dev, bridge=bridge, if_exists=True).execute() if delete_netdev: linux_net.delete_net_dev(dev) diff --git a/vif_plug_ovs/tests/unit/ovsdb/test_impl_vsctl.py b/vif_plug_ovs/tests/unit/ovsdb/test_impl_vsctl.py new file mode 100644 index 00000000..0f518216 --- /dev/null +++ b/vif_plug_ovs/tests/unit/ovsdb/test_impl_vsctl.py @@ -0,0 +1,22 @@ +# 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. + +from os_vif.tests.unit import base +from vif_plug_ovs.ovsdb import impl_vsctl + + +class TestModuleLevelMethods(base.TestCase): + + def test__set_colval_args(self): + col_values = [('mac', "aa:aa:aa:aa:aa:aa")] + args = impl_vsctl._set_colval_args(*col_values) + self.assertEqual(['mac=aa\:aa\:aa\:aa\:aa\:aa'], args) diff --git a/vif_plug_ovs/tests/unit/ovsdb/test_ovsdb_lib.py b/vif_plug_ovs/tests/unit/ovsdb/test_ovsdb_lib.py index 19e12dc6..78ae6945 100644 --- a/vif_plug_ovs/tests/unit/ovsdb/test_ovsdb_lib.py +++ b/vif_plug_ovs/tests/unit/ovsdb/test_ovsdb_lib.py @@ -17,6 +17,7 @@ from oslo_concurrency import processutils from oslo_config import cfg from oslo_utils import uuidutils +from os_vif import utils from vif_plug_ovs import constants from vif_plug_ovs import linux_net from vif_plug_ovs.ovsdb import ovsdb_lib @@ -37,6 +38,9 @@ class BaseOVSTest(testtools.TestCase): test_vif_plug_ovs_group) CONF.register_opt(cfg.StrOpt('ovsdb_connection', default=None), test_vif_plug_ovs_group) + CONF.register_opt(cfg.StrOpt('cleanup_base_mac', + default='aa:16:3f:00:00:00'), + test_vif_plug_ovs_group) self.br = ovsdb_lib.BaseOVS(cfg.CONF.test_vif_plug_ovs) self.mock_db_set = mock.patch.object(self.br.ovsdb, 'db_set').start() self.mock_del_port = mock.patch.object(self.br.ovsdb, @@ -132,7 +136,8 @@ class BaseOVSTest(testtools.TestCase): values = [('external_ids', external_ids), ('type', interface_type), ('options', {'dpdk-devargs': - '0000:02:00.1,representor=[0]'})] + '0000:02:00.1,representor=[0]'}), + ('mac', 'ca:fe:ca:fe:ca:fe')] with mock.patch.object(self.br, 'update_device_mtu', return_value=True) as mock_update_device_mtu, \ mock.patch.object(self.br, '_ovs_supports_mtu_requests', @@ -158,19 +163,44 @@ class BaseOVSTest(testtools.TestCase): interface_type=constants.OVS_VHOSTUSER_INTERFACE_TYPE)]) @mock.patch.object(linux_net, 'delete_net_dev') - def test_delete_ovs_vif_port(self, mock_delete_net_dev): + @mock.patch.object(ovsdb_lib.BaseOVS, '_is_dpdk_representor_port', + return_value=False) + def test_delete_ovs_vif_port(self, mock_is_dpdk_representor_port, + mock_delete_net_dev): self.br.delete_ovs_vif_port('bridge', 'device') self.mock_del_port.assert_has_calls( [mock.call('device', bridge='bridge', if_exists=True)]) mock_delete_net_dev.assert_has_calls([mock.call('device')]) @mock.patch.object(linux_net, 'delete_net_dev') - def test_delete_ovs_vif_port_no_delete_netdev(self, mock_delete_net_dev): - self.br.delete_ovs_vif_port('bridge', 'device', delete_netdev=False) + @mock.patch.object(ovsdb_lib.BaseOVS, '_is_dpdk_representor_port', + return_value=False) + def test_delete_ovs_vif_port_no_delete_netdev(self, + mock_is_dpdk_representor_port, mock_delete_net_dev): + self.br.delete_ovs_vif_port('bridge', 'device', + delete_netdev=False) self.mock_del_port.assert_has_calls( [mock.call('device', bridge='bridge', if_exists=True)]) mock_delete_net_dev.assert_not_called() + @mock.patch.object(linux_net, 'delete_net_dev') + @mock.patch.object(utils, 'get_random_mac', + return_value='aa:16:3f:00:00:00') + @mock.patch.object(ovsdb_lib.BaseOVS, '_is_dpdk_representor_port', + return_value=True) + def test_delete_ovs_dpdk_representor_port(self, + mock_is_dpdk_representor_port, mock_get_random_mac, + mock_delete_net_dev): + self.br.delete_ovs_vif_port('bridge', 'device') + self.mock_del_port.assert_has_calls( + [mock.call('device', bridge='bridge', if_exists=True)]) + self.mock_db_set.assert_has_calls( + [mock.call('Interface', 'device', + ('mac', 'aa:16:3f:00:00:00'))]) + mock_delete_net_dev.assert_has_calls([mock.call('device')]) + mock_get_random_mac.assert_has_calls([mock.call( + ['aa', '16', '3f', '00', '00', '00'])]) + def test_ensure_ovs_bridge(self): self.br.ensure_ovs_bridge('bridge', constants.OVS_DATAPATH_SYSTEM) self.mock_add_br('bridge', may_exist=True,