From e7488570e70d0a92cc3d8fa3b5853df7fbb33564 Mon Sep 17 00:00:00 2001 From: "Sean M. Collins" Date: Mon, 12 Oct 2015 12:43:02 -0400 Subject: [PATCH] Revert "Make OVS interface name hashing algorithm common and extend it" Based on discussion with Ihar and Cedric, this may break upgrades. Let's revert it for the time being and reassess. This reverts commit 3d0db13370eee09aaee5923c54d0e3188509c4e5. Change-Id: I69d74befb08e074c1bccd823cca0899de6ed57d6 Closes-Bug: 1504647 --- neutron/plugins/common/utils.py | 45 -------- .../agent/linuxbridge_neutron_agent.py | 42 ++----- .../openvswitch/agent/ovs_neutron_agent.py | 37 ++++++- neutron/tests/unit/plugins/common/__init__.py | 0 .../tests/unit/plugins/common/test_utils.py | 103 ------------------ .../agent/test_linuxbridge_neutron_agent.py | 25 +---- .../agent/test_ovs_neutron_agent.py | 11 ++ 7 files changed, 60 insertions(+), 203 deletions(-) delete mode 100644 neutron/tests/unit/plugins/common/__init__.py delete mode 100644 neutron/tests/unit/plugins/common/test_utils.py diff --git a/neutron/plugins/common/utils.py b/neutron/plugins/common/utils.py index 54c896388..61e7c164f 100644 --- a/neutron/plugins/common/utils.py +++ b/neutron/plugins/common/utils.py @@ -16,20 +16,12 @@ Common utilities and helper functions for Openstack Networking Plugins. """ -import hashlib import webob.exc -from oslo_log import log as logging -import six - from neutron.api.v2 import attributes -from neutron.common import constants as n_const from neutron.common import exceptions as n_exc -from neutron.i18n import _LI from neutron.plugins.common import constants as p_const -LOG = logging.getLogger(__name__) - def is_valid_vlan_tag(vlan): return p_const.MIN_VLAN_TAG <= vlan <= p_const.MAX_VLAN_TAG @@ -146,40 +138,3 @@ def create_port(core_plugin, context, port, check_allow_post=True): port.get('port', {}), check_allow_post=check_allow_post) return core_plugin.create_port(context, {'port': port_data}) - - -def get_interface_name(name, prefix='', postfix='', - max_len=n_const.DEVICE_NAME_MAX_LEN): - """Construct an interface name based on the prefix, postfix and name. - - The interface name can not exceed the maximum length passed in. Longer - names are hashed to help ensure uniqueness. - """ - requested_name = prefix + name + postfix - - if len(requested_name) <= max_len: - return requested_name - # We can't just truncate because interfaces may be distinguished - # by an ident at the end. A hash over the name should be unique. - # Leave part of the interface name on for easier identification - hashlen = 6 - - if (len(prefix + postfix) + hashlen) > max_len: - raise ValueError("Too long pre- and postfix provided. New name would " - "exceed given length for an interface name.") - - namelen = max_len - len(prefix) - hashlen - len(postfix) - if isinstance(name, six.text_type): - hashed_name = hashlib.sha1(name.encode('utf-8')) - else: - hashed_name = hashlib.sha1(name) - new_name = ('%(prefix)s%(truncated)s%(hash)s%(postfix)s' % - {'prefix': prefix, 'truncated': name[0:namelen], - 'hash': hashed_name.hexdigest()[0:hashlen], - 'postfix': postfix}) - LOG.info(_LI("The requested interface name %(requested_name)s exceeds the " - "%(limit)d character limitation. It was shortened to " - "%(new_name)s to fit."), - {'requested_name': requested_name, - 'limit': max_len, 'new_name': new_name}) - return new_name diff --git a/neutron/plugins/ml2/drivers/linuxbridge/agent/linuxbridge_neutron_agent.py b/neutron/plugins/ml2/drivers/linuxbridge/agent/linuxbridge_neutron_agent.py index f93379243..89f819be2 100644 --- a/neutron/plugins/ml2/drivers/linuxbridge/agent/linuxbridge_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/linuxbridge/agent/linuxbridge_neutron_agent.py @@ -48,7 +48,6 @@ from neutron.common import utils as n_utils from neutron import context from neutron.i18n import _LE, _LI, _LW from neutron.plugins.common import constants as p_const -from neutron.plugins.common import utils as p_utils from neutron.plugins.ml2.drivers.l2pop.rpc_manager \ import l2population_rpc as l2pop_rpc from neutron.plugins.ml2.drivers.linuxbridge.agent import arp_protect @@ -141,29 +140,12 @@ class LinuxBridgeManager(object): bridge_name = BRIDGE_NAME_PREFIX + network_id[0:11] return bridge_name - def get_vlan_device_name(self, physical_interface, vlan_id): + def get_subinterface_name(self, physical_interface, vlan_id): if not vlan_id: - raise ValueError("No VLAN ID specified!") - - vlan_len = len(str(vlan_id)) - if vlan_len > 4: - raise ValueError("Invalid VLAN ID! ID exceeds 4 digits!") - - vlan_postfix = '.%s' % vlan_id - - # Handling for too long physical_interface names: - # Ensure that vlan devices that belong to the same logical network - # use the same naming pattern despite the hashing algorithm that is - # used in such cases. E.g. - # Interface name: "very_long_name" should NOT result in - # "veryHASHED.1111" and "very_loHASHED.1" but rather in - # "veryHASHED.1111" and "veryHASHED.1". This can be accomplished with - # requesting a smaller device name length for small vlan ids. - max_len = constants.DEVICE_NAME_MAX_LEN - (4 - vlan_len) - vlan_name = p_utils.get_interface_name(physical_interface, - postfix=vlan_postfix, - max_len=max_len) - return vlan_name + LOG.warning(_LW("Invalid VLAN ID, will lead to incorrect " + "subinterface name")) + subinterface_name = '%s.%s' % (physical_interface, vlan_id) + return subinterface_name def get_tap_device_name(self, interface_id): if not interface_id: @@ -296,23 +278,23 @@ class LinuxBridgeManager(object): def ensure_vlan(self, physical_interface, vlan_id): """Create a vlan unless it already exists.""" - vlan_device = self.get_vlan_device_name(physical_interface, vlan_id) - if not ip_lib.device_exists(vlan_device): + interface = self.get_subinterface_name(physical_interface, vlan_id) + if not ip_lib.device_exists(interface): LOG.debug("Creating subinterface %(interface)s for " "VLAN %(vlan_id)s on interface " "%(physical_interface)s", - {'interface': vlan_device, 'vlan_id': vlan_id, + {'interface': interface, 'vlan_id': vlan_id, 'physical_interface': physical_interface}) if utils.execute(['ip', 'link', 'add', 'link', physical_interface, - 'name', vlan_device, 'type', 'vlan', 'id', + 'name', interface, 'type', 'vlan', 'id', vlan_id], run_as_root=True): return if utils.execute(['ip', 'link', 'set', - vlan_device, 'up'], run_as_root=True): + interface, 'up'], run_as_root=True): return - LOG.debug("Done creating subinterface %s", vlan_device) - return vlan_device + LOG.debug("Done creating subinterface %s", interface) + return interface def ensure_vxlan(self, segmentation_id): """Create a vxlan unless it already exists.""" diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py index 9f744b684..bb3ae1647 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py @@ -14,6 +14,7 @@ # under the License. import collections +import hashlib import signal import sys import time @@ -45,7 +46,6 @@ from neutron.common import utils as n_utils from neutron import context from neutron.i18n import _LE, _LI, _LW from neutron.plugins.common import constants as p_const -from neutron.plugins.common import utils as p_utils from neutron.plugins.ml2.drivers.l2pop.rpc_manager import l2population_rpc from neutron.plugins.ml2.drivers.openvswitch.agent.common \ import constants @@ -1028,6 +1028,33 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, self.tun_br.setup_default_table(self.patch_int_ofport, self.arp_responder_enabled) + def get_peer_name(self, prefix, name): + """Construct a peer name based on the prefix and name. + + The peer name can not exceed the maximum length allowed for a linux + device. Longer names are hashed to help ensure uniqueness. + """ + if len(prefix + name) <= n_const.DEVICE_NAME_MAX_LEN: + return prefix + name + # We can't just truncate because bridges may be distinguished + # by an ident at the end. A hash over the name should be unique. + # Leave part of the bridge name on for easier identification + hashlen = 6 + namelen = n_const.DEVICE_NAME_MAX_LEN - len(prefix) - hashlen + if isinstance(name, six.text_type): + hashed_name = hashlib.sha1(name.encode('utf-8')) + else: + hashed_name = hashlib.sha1(name) + new_name = ('%(prefix)s%(truncated)s%(hash)s' % + {'prefix': prefix, 'truncated': name[0:namelen], + 'hash': hashed_name.hexdigest()[0:hashlen]}) + LOG.warning(_LW("Creating an interface named %(name)s exceeds the " + "%(limit)d character limitation. It was shortened to " + "%(new_name)s to fit."), + {'name': name, 'limit': n_const.DEVICE_NAME_MAX_LEN, + 'new_name': new_name}) + return new_name + def setup_physical_bridges(self, bridge_mappings): '''Setup the physical network bridges. @@ -1061,10 +1088,10 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, self.phys_brs[physical_network] = br # interconnect physical and integration bridges using veth/patchs - int_if_name = p_utils.get_interface_name(bridge, - prefix=constants.PEER_INTEGRATION_PREFIX) - phys_if_name = p_utils.get_interface_name(bridge, - prefix=constants.PEER_PHYSICAL_PREFIX) + int_if_name = self.get_peer_name(constants.PEER_INTEGRATION_PREFIX, + bridge) + phys_if_name = self.get_peer_name(constants.PEER_PHYSICAL_PREFIX, + bridge) # Interface type of port for physical and integration bridges must # be same, so check only one of them. int_type = self.int_br.db_get_val("Interface", int_if_name, "type") diff --git a/neutron/tests/unit/plugins/common/__init__.py b/neutron/tests/unit/plugins/common/__init__.py deleted file mode 100644 index e69de29bb..000000000 diff --git a/neutron/tests/unit/plugins/common/test_utils.py b/neutron/tests/unit/plugins/common/test_utils.py deleted file mode 100644 index f6da5a8f6..000000000 --- a/neutron/tests/unit/plugins/common/test_utils.py +++ /dev/null @@ -1,103 +0,0 @@ -# Copyright (c) 2015 IBM Corp. -# -# 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 hashlib -import mock - - -from neutron.plugins.common import utils -from neutron.tests import base - - -LONG_NAME1 = "A_REALLY_LONG_INTERFACE_NAME1" -LONG_NAME2 = "A_REALLY_LONG_INTERFACE_NAME2" -SHORT_NAME = "SHORT" - -MOCKED_HASH = "mockedhash" - - -class MockSHA(object): - def hexdigest(self): - return MOCKED_HASH - - -class TestUtils(base.BaseTestCase): - - @mock.patch.object(hashlib, 'sha1', return_value=MockSHA()) - def test_get_interface_name(self, mock_sha1): - prefix = "pre-" - postfix = ".1111" - - if_default_long = utils.get_interface_name(LONG_NAME1) - if_default_short = utils.get_interface_name(SHORT_NAME) - - if_prefix_long = utils.get_interface_name(LONG_NAME1, prefix=prefix) - if_prefix_short = utils.get_interface_name(SHORT_NAME, prefix=prefix) - - if_postfix_long = utils.get_interface_name(LONG_NAME1, postfix=postfix) - if_postfix_short = utils.get_interface_name(SHORT_NAME, - postfix=postfix) - - if_prefix_postfix_long = utils.get_interface_name(LONG_NAME1, - prefix=prefix, - postfix=postfix) - if_prefix_postfix_short = utils.get_interface_name(SHORT_NAME, - prefix=prefix, - postfix=postfix) - - # Each combination is a tuple of the following values: - # the calculated name, the expected name" - combinations = [(if_default_long, "A_REALLY_mocked"), - (if_default_short, "SHORT"), - (if_prefix_long, "pre-A_REAmocked"), - (if_prefix_short, "pre-SHORT"), - (if_postfix_long, "A_REmocked.1111"), - (if_postfix_short, "SHORT.1111"), - (if_prefix_postfix_long, "pre-mocked.1111"), - (if_prefix_postfix_short, "pre-SHORT.1111")] - - for if_new_name, if_expected_new_name in combinations: - self.assertEqual(if_new_name, if_expected_new_name) - - def test_get_interface_uniqueness(self): - prefix = "prefix-" - if_prefix1 = utils.get_interface_name(LONG_NAME1, prefix=prefix) - if_prefix2 = utils.get_interface_name(LONG_NAME2, prefix=prefix) - self.assertNotEqual(if_prefix1, if_prefix2) - - def test_get_interface_long_post_and_prefix(self): - """Prefix and postfix alone overcome the max character limit.""" - - long_prefix = "long_pre" - long_postfix = "long_pos" - much_too_long_prefix = "much_too_long_prefix" - much_too_long_postfix = "much_too_long_postfix" - - self.assertEqual("long_preSHORT", utils.get_interface_name(SHORT_NAME, - prefix=long_prefix)) - self.assertEqual("SHORTlong_pos", utils.get_interface_name(SHORT_NAME, - postfix=long_postfix)) - self.assertRaises(ValueError, utils.get_interface_name, SHORT_NAME, - prefix=long_prefix, postfix=long_postfix) - - self.assertRaises(ValueError, utils.get_interface_name, SHORT_NAME, - prefix=much_too_long_prefix) - self.assertRaises(ValueError, utils.get_interface_name, SHORT_NAME, - postfix=much_too_long_postfix) - - @mock.patch.object(hashlib, 'sha1', return_value=MockSHA()) - def test_get_interface_max_len(self, mock_sha1): - self.assertTrue(len(utils.get_interface_name(LONG_NAME1)) == 15) - self.assertTrue(len(utils.get_interface_name(LONG_NAME1, max_len=10)) - == 10) diff --git a/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_linuxbridge_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_linuxbridge_neutron_agent.py index cf16d44f6..d5eea954e 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_linuxbridge_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_linuxbridge_neutron_agent.py @@ -24,7 +24,6 @@ from neutron.agent.linux import utils from neutron.common import constants from neutron.common import exceptions from neutron.plugins.common import constants as p_const -from neutron.plugins.common import utils as p_utils from neutron.plugins.ml2.drivers.linuxbridge.agent.common \ import constants as lconst from neutron.plugins.ml2.drivers.linuxbridge.agent \ @@ -428,25 +427,11 @@ class TestLinuxBridgeManager(base.BaseTestCase): self.assertEqual(self.lbm.get_bridge_name(nw_id), "brq") - def test_get_vlan_device_name(self): - with mock.patch.object(p_utils, "get_interface_name", - return_value="eth0.1") as mock_get_vlan: - self.assertEqual(self.lbm.get_vlan_device_name("eth0", 1), - "eth0.1") - mock_get_vlan.assert_called_with("eth0", postfix=".1", max_len=12) - - with mock.patch.object(p_utils, "get_interface_name", - return_value="eth0.1111") as mock_get_vlan: - self.assertEqual(self.lbm.get_vlan_device_name("eth0", 1111), - "eth0.1111") - mock_get_vlan.assert_called_with("eth0", postfix=".1111", - max_len=15) - invalid_vlan = 11111 - self.assertRaises(ValueError, self.lbm.get_vlan_device_name, "eth0", - invalid_vlan) - self.assertRaises(ValueError, self.lbm.get_vlan_device_name, "eth0", 0) - self.assertRaises(ValueError, self.lbm.get_vlan_device_name, "eth0", - None) + def test_get_subinterface_name(self): + self.assertEqual(self.lbm.get_subinterface_name("eth0", "0"), + "eth0.0") + self.assertEqual(self.lbm.get_subinterface_name("eth0", ""), + "eth0.") def test_get_tap_device_name(self): if_id = "123456789101112" diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py index c3539c1c1..654b42943 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py @@ -921,6 +921,17 @@ class TestOvsNeutronAgent(object): self.assertEqual(self.agent.phys_ofports["physnet1"], "phy_ofport") + def test_get_peer_name(self): + bridge1 = "A_REALLY_LONG_BRIDGE_NAME1" + bridge2 = "A_REALLY_LONG_BRIDGE_NAME2" + self.agent.use_veth_interconnection = True + self.assertEqual(len(self.agent.get_peer_name('int-', bridge1)), + n_const.DEVICE_NAME_MAX_LEN) + self.assertEqual(len(self.agent.get_peer_name('int-', bridge2)), + n_const.DEVICE_NAME_MAX_LEN) + self.assertNotEqual(self.agent.get_peer_name('int-', bridge1), + self.agent.get_peer_name('int-', bridge2)) + def test_setup_tunnel_br(self): self.tun_br = mock.Mock() with mock.patch.object(self.agent.int_br, -- 2.45.2