From e15e8dc46e54f37c3ff105eb8233ad65f23d0db3 Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Thu, 1 Jun 2017 16:31:48 +0000 Subject: [PATCH] set mtu on all code paths - This change ensures that the mtu is always set by the linux bridge plugin if provided. related-bug: #bug/1684326 Change-Id: I9f901dcf67823a6dd78b9b18cb3e06a3f6a13e37 --- vif_plug_linux_bridge/linux_bridge.py | 4 +- vif_plug_linux_bridge/linux_net.py | 29 ++++++++---- .../tests/unit/test_linux_net.py | 44 +++++++++++++++---- .../tests/unit/test_plugin.py | 22 +++++++--- 4 files changed, 74 insertions(+), 25 deletions(-) diff --git a/vif_plug_linux_bridge/linux_bridge.py b/vif_plug_linux_bridge/linux_bridge.py index aa124655..4a2f7733 100644 --- a/vif_plug_linux_bridge/linux_bridge.py +++ b/vif_plug_linux_bridge/linux_bridge.py @@ -95,9 +95,9 @@ class LinuxBridgePlugin(plugin.PluginBase): network = vif.network bridge_name = vif.bridge_name if not network.multi_host and network.should_provide_bridge: + mtu = network.mtu or self.config.network_device_mtu if network.should_provide_vlan: iface = self.config.vlan_interface or network.bridge_interface - mtu = network.mtu or self.config.network_device_mtu linux_net.ensure_vlan_bridge(network.vlan, bridge_name, iface, mtu=mtu) else: @@ -105,7 +105,7 @@ class LinuxBridgePlugin(plugin.PluginBase): # only put in iptables rules if Neutron not filtering install_filters = not vif.has_traffic_filtering linux_net.ensure_bridge(bridge_name, iface, - filtering=install_filters) + filtering=install_filters, mtu=mtu) def unplug(self, vif, instance_info): # Nothing required to unplug a port for a VIF using standard diff --git a/vif_plug_linux_bridge/linux_net.py b/vif_plug_linux_bridge/linux_net.py index 9792dc66..8e0f63ee 100644 --- a/vif_plug_linux_bridge/linux_net.py +++ b/vif_plug_linux_bridge/linux_net.py @@ -39,8 +39,12 @@ def device_exists(device): def _set_device_mtu(dev, mtu): """Set the device MTU.""" - processutils.execute('ip', 'link', 'set', dev, 'mtu', mtu, - check_exit_code=[0, 2, 254]) + if mtu: + processutils.execute('ip', 'link', 'set', dev, 'mtu', mtu, + check_exit_code=[0, 2, 254]) + else: + LOG.debug("MTU not set on %(interface_name)s interface", + {'interface_name': dev}) def _ip_bridge_cmd(action, params, device): @@ -85,33 +89,32 @@ def _ensure_vlan_privileged(vlan_num, bridge_interface, mac_address, mtu): check_exit_code=[0, 2, 254]) processutils.execute('ip', 'link', 'set', interface, 'up', check_exit_code=[0, 2, 254]) - if mtu: # NOTE(vish): set mtu every time to ensure that changes to mtu get # propogated _set_device_mtu(interface, mtu) - else: - LOG.debug("MTU not set on %(interface_name)s interface", - {'interface_name': interface}) + return interface @lockutils.synchronized('nova-lock_bridge', external=True) def ensure_bridge(bridge, interface, net_attrs=None, gateway=True, - filtering=True): - _ensure_bridge_privileged(bridge, interface, net_attrs, gateway, filtering) + filtering=True, mtu=None): + _ensure_bridge_privileged(bridge, interface, net_attrs, gateway, + filtering=filtering, mtu=mtu) if filtering: _ensure_bridge_filtering(bridge, gateway) @privsep.vif_plug.entrypoint def _ensure_bridge_privileged(bridge, interface, net_attrs, gateway, - filtering=True): + filtering=True, mtu=None): """Create a bridge unless it already exists. :param interface: the interface to create the bridge on. :param net_attrs: dictionary with attributes used to create bridge. :param gateway: whether or not the bridge is a gateway. :param filtering: whether or not to create filters on the bridge. + :param mtu: MTU of bridge. If net_attrs is set, it will add the net_attrs['gateway'] to the bridge using net_attrs['broadcast'] and net_attrs['cidr']. It will also add @@ -156,6 +159,8 @@ def _ensure_bridge_privileged(bridge, interface, net_attrs, gateway, out, err = processutils.execute('ip', 'link', 'set', interface, 'up', check_exit_code=False) + _set_device_mtu(interface, mtu) + # NOTE(vish): This will break if there is already an ip on the # interface, so we move any ips to the bridge # NOTE(danms): We also need to copy routes to the bridge so as @@ -186,6 +191,12 @@ def _ensure_bridge_privileged(bridge, interface, net_attrs, gateway, for fields in old_routes: processutils.execute('ip', 'route', 'add', *fields) + # NOTE(sean-k-mooney): + # The bridge mtu cannont be set until after an + # interface is added due to bug: + # https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1399064 + _set_device_mtu(bridge, mtu) + def _ensure_bridge_filtering(bridge, gateway): # This method leaves privsep usage to iptables manager diff --git a/vif_plug_linux_bridge/tests/unit/test_linux_net.py b/vif_plug_linux_bridge/tests/unit/test_linux_net.py index e6ca0894..f7d5fdfd 100644 --- a/vif_plug_linux_bridge/tests/unit/test_linux_net.py +++ b/vif_plug_linux_bridge/tests/unit/test_linux_net.py @@ -46,6 +46,11 @@ class LinuxNetTest(testtools.TestCase): expected = ['ip', 'link', 'set', 'fakedev', 'mtu', 1500] execute.assert_called_with(*expected, check_exit_code=mock.ANY) + @mock.patch.object(processutils, "execute") + def test_set_device_invalid_mtu(self, mock_exec): + linux_net._set_device_mtu(dev='fakedev', mtu=None) + mock_exec.assert_not_called() + @mock.patch.object(processutils, "execute") @mock.patch.object(linux_net, "device_exists", return_value=False) @mock.patch.object(linux_net, "_set_device_mtu") @@ -67,15 +72,6 @@ class LinuxNetTest(testtools.TestCase): mock_exec.assert_has_calls(calls) mock_set_mtu.assert_called_once_with('vlan123', 1500) - @mock.patch.object(processutils, "execute") - @mock.patch.object(linux_net, "device_exists", return_value=False) - @mock.patch.object(linux_net, "_set_device_mtu") - def test_ensure_vlan_invalid_mtu(self, mock_set_mtu, - mock_dev_exists, mock_exec): - linux_net._ensure_vlan_privileged(123, 'fake-bridge', - mac_address='fake-mac', mtu=None) - self.assertFalse(mock_set_mtu.called) - @mock.patch.object(processutils, "execute") @mock.patch.object(linux_net, "device_exists", return_value=True) def test_ensure_bridge_exists(self, mock_dev_exists, mock_exec): @@ -104,6 +100,36 @@ class LinuxNetTest(testtools.TestCase): mock_exec.assert_has_calls(calls) mock_dev_exists.assert_has_calls([mock.call("br0"), mock.call("br0")]) + @mock.patch.object(linux_net, "_set_device_mtu") + @mock.patch.object(os.path, "exists", return_value=False) + @mock.patch.object(processutils, "execute") + @mock.patch.object(linux_net, "device_exists", return_value=False) + def test_ensure_bridge_mtu_not_called(self, mock_dev_exists, mock_exec, + mock_path_exists, mock_set_mtu): + """This test validates that mtus are updated only if an interface + is added to the bridge + """ + linux_net._ensure_bridge_privileged("fake-bridge", None, + None, False, mtu=1500) + mock_set_mtu.assert_not_called() + + @mock.patch.object(linux_net, "_set_device_mtu") + @mock.patch.object(os.path, "exists", return_value=False) + @mock.patch.object(processutils, "execute", return_value=("", "")) + @mock.patch.object(linux_net, "device_exists", return_value=False) + def test_ensure_bridge_mtu_order(self, mock_dev_exists, mock_exec, + mock_path_exists, mock_set_mtu): + """This test validates that when adding an interface + to a bridge, the interface mtu is updated first + followed by the bridge. This is required to work around + https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1399064 + """ + linux_net._ensure_bridge_privileged("fake-bridge", "fake-interface", + None, False, mtu=1500) + calls = [mock.call('fake-interface', 1500), + mock.call('fake-bridge', 1500)] + mock_set_mtu.assert_has_calls(calls) + @mock.patch.object(os.path, "exists", return_value=False) @mock.patch.object(processutils, "execute") @mock.patch.object(linux_net, "device_exists", return_value=False) diff --git a/vif_plug_linux_bridge/tests/unit/test_plugin.py b/vif_plug_linux_bridge/tests/unit/test_plugin.py index d7921c34..8a975d9d 100644 --- a/vif_plug_linux_bridge/tests/unit/test_plugin.py +++ b/vif_plug_linux_bridge/tests/unit/test_plugin.py @@ -51,15 +51,23 @@ class PluginTest(testtools.TestCase): mock_ensure_bridge.assert_not_called() mock_ensure_vlan_bridge.assert_not_called() + def test_plug_bridge_create_br_mtu_in_model(self): + self._test_plug_bridge_create_br(mtu=1234) + + def test_plug_bridge_create_br_mtu_from_config(self): + self._test_plug_bridge_create_br() + @mock.patch.object(linux_net, 'ensure_vlan_bridge') @mock.patch.object(linux_net, 'ensure_bridge') - def test_plug_bridge_create_br(self, mock_ensure_bridge, - mock_ensure_vlan_bridge): + def _test_plug_bridge_create_br(self, mock_ensure_bridge, + mock_ensure_vlan_bridge, + mtu=None): network = objects.network.Network( id='437c6db5-4e6f-4b43-b64b-ed6a11ee5ba7', bridge='br0', bridge_interface='eth0', - should_provide_bridge=True) + should_provide_bridge=True, + mtu=mtu) vif = objects.vif.VIFBridge( id='b679325f-ca89-4ee0-a8be-6db1409b69ea', @@ -72,13 +80,17 @@ class PluginTest(testtools.TestCase): plugin = linux_bridge.LinuxBridgePlugin.load("linux_bridge") plugin.plug(vif, self.instance) - mock_ensure_bridge.assert_called_with("br0", "eth0", filtering=False) + mock_ensure_bridge.assert_called_with("br0", "eth0", + filtering=False, + mtu=mtu or 1500) mock_ensure_vlan_bridge.assert_not_called() mock_ensure_bridge.reset_mock() vif.has_traffic_filtering = False plugin.plug(vif, self.instance) - mock_ensure_bridge.assert_called_with("br0", "eth0", filtering=True) + mock_ensure_bridge.assert_called_with("br0", "eth0", + filtering=True, + mtu=mtu or 1500) def test_plug_bridge_create_br_vlan_mtu_in_model(self): self._test_plug_bridge_create_br_vlan(mtu=1234)