remove use of brctl from vif_plug_linux_bridge
- This change replaces calls to brctl with calls to the ip_command interface. - This change adds a release note for the brctl removal. - This change removes the use of tee to disable ipv6 in the linux bridge plugin. Change-Id: Id03be72e22302a0954f3e47c116f389cb4304c03 Closes-Bug: #1801919
This commit is contained in:
Sean Mooney
parent
5027ce833c
commit
1f6fed6a69
10
releasenotes/notes/brctl-removal-a5b0e69b865afa57.yaml
Normal file
10
releasenotes/notes/brctl-removal-a5b0e69b865afa57.yaml
Normal file
@ -0,0 +1,10 @@
|
||||
---
|
||||
other:
|
||||
- |
|
||||
With this release, packagers of ``os-vif`` no longer need to create a
|
||||
depency on ``brctl``. ``brctl`` is largely considered obsolete and has
|
||||
been replaced with iproute2 by default in many linux distributions.
|
||||
RHEL 8 will not ship ``brctl`` in its default repos. As part of a larger
|
||||
effort to remove usage of ``brctl`` from OpenStack ``os-vif`` has
|
||||
replaced its usage of ``brctl`` with ``pyroute2``. This does not introduce
|
||||
any new requirements as ``pyroute2`` is already a requirement.
|
||||
@ -25,7 +25,6 @@ from os_vif.internal.command import ip as ip_lib
|
||||
from oslo_concurrency import lockutils
|
||||
from oslo_concurrency import processutils
|
||||
from oslo_log import log as logging
|
||||
from oslo_utils import excutils
|
||||
|
||||
from vif_plug_linux_bridge import privsep
|
||||
|
||||
@ -97,6 +96,59 @@ def ensure_bridge(bridge, interface, net_attrs=None, gateway=True,
|
||||
_ensure_bridge_filtering(bridge, gateway)
|
||||
|
||||
|
||||
# TODO(sean-k-mooney): extract into common module
|
||||
def _disable_ipv6(bridge):
|
||||
"""disable ipv6 for bridge if available, must be called from
|
||||
privsep context.
|
||||
:param bridge: string bridge name
|
||||
"""
|
||||
disv6 = ('/proc/sys/net/ipv6/conf/%s/disable_ipv6' %
|
||||
bridge)
|
||||
if os.path.exists(disv6):
|
||||
with open(disv6, 'w') as f:
|
||||
f.write('1')
|
||||
|
||||
|
||||
def _update_bridge_routes(interface, bridge):
|
||||
"""Updates routing table for a given bridge and interface.
|
||||
:param interface: string interface name
|
||||
:param bridge: string bridge name
|
||||
"""
|
||||
# TODO(sean-k-mooney): investigate deleting all this route
|
||||
# handling code. The vm tap devices should never have an ip,
|
||||
# this is old nova networks code and i dont think it will ever
|
||||
# be needed in os-vif.
|
||||
# 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
|
||||
# not to break existing connectivity on the interface
|
||||
old_routes = []
|
||||
out, err = processutils.execute('ip', 'route', 'show', 'dev',
|
||||
interface)
|
||||
for line in out.split('\n'):
|
||||
fields = line.split()
|
||||
if fields and 'via' in fields:
|
||||
old_routes.append(fields)
|
||||
processutils.execute('ip', 'route', 'del', *fields)
|
||||
out, err = processutils.execute('ip', 'addr', 'show', 'dev',
|
||||
interface, 'scope', 'global')
|
||||
for line in out.split('\n'):
|
||||
fields = line.split()
|
||||
if fields and fields[0] == 'inet':
|
||||
if fields[-2] in ('secondary', 'dynamic', ):
|
||||
params = fields[1:-2]
|
||||
else:
|
||||
params = fields[1:-1]
|
||||
processutils.execute(*_ip_bridge_cmd('del', params,
|
||||
fields[-1]),
|
||||
check_exit_code=[0, 2, 254])
|
||||
processutils.execute(*_ip_bridge_cmd('add', params,
|
||||
bridge),
|
||||
check_exit_code=[0, 2, 254])
|
||||
for fields in old_routes:
|
||||
processutils.execute('ip', 'route', 'add', *fields)
|
||||
|
||||
|
||||
@privsep.vif_plug.entrypoint
|
||||
def _ensure_bridge_privileged(bridge, interface, net_attrs, gateway,
|
||||
filtering=True, mtu=None):
|
||||
@ -118,70 +170,18 @@ def _ensure_bridge_privileged(bridge, interface, net_attrs, gateway,
|
||||
"""
|
||||
if not ip_lib.exists(bridge):
|
||||
LOG.debug('Starting Bridge %s', bridge)
|
||||
try:
|
||||
processutils.execute('brctl', 'addbr', bridge)
|
||||
except Exception:
|
||||
with excutils.save_and_reraise_exception() as ectx:
|
||||
ectx.reraise = not ip_lib.exists(bridge)
|
||||
processutils.execute('brctl', 'setfd', bridge, 0)
|
||||
# processutils.execute('brctl setageing %s 10' % bridge)
|
||||
processutils.execute('brctl', 'stp', bridge, 'off')
|
||||
disv6 = ('/proc/sys/net/ipv6/conf/%s/disable_ipv6' % bridge)
|
||||
if os.path.exists(disv6):
|
||||
processutils.execute('tee',
|
||||
disv6,
|
||||
process_input='1',
|
||||
check_exit_code=[0, 1])
|
||||
# (danwent) bridge device MAC address can't be set directly.
|
||||
# instead it inherits the MAC address of the first device on the
|
||||
# bridge, which will either be the vlan interface, or a
|
||||
# physical NIC.
|
||||
ip_lib.add(bridge, 'bridge')
|
||||
_disable_ipv6(bridge)
|
||||
ip_lib.set(bridge, state='up')
|
||||
|
||||
if interface:
|
||||
if interface and ip_lib.exists(interface):
|
||||
LOG.debug('Adding interface %(interface)s to bridge %(bridge)s',
|
||||
{'interface': interface, 'bridge': bridge})
|
||||
out, err = processutils.execute('brctl', 'addif', bridge,
|
||||
interface, check_exit_code=False)
|
||||
if (err and err != "device %s is already a member of a bridge; "
|
||||
"can't enslave it to bridge %s.\n" % (interface, bridge)):
|
||||
msg = _('Failed to add interface: %s') % err
|
||||
raise Exception(msg)
|
||||
|
||||
ip_lib.set(interface, state='up')
|
||||
ip_lib.set(interface, master=bridge, state='up',
|
||||
check_exit_code=[0, 2, 254])
|
||||
|
||||
_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
|
||||
# not to break existing connectivity on the interface
|
||||
old_routes = []
|
||||
out, err = processutils.execute('ip', 'route', 'show', 'dev',
|
||||
interface)
|
||||
for line in out.split('\n'):
|
||||
fields = line.split()
|
||||
if fields and 'via' in fields:
|
||||
old_routes.append(fields)
|
||||
processutils.execute('ip', 'route', 'del', *fields)
|
||||
out, err = processutils.execute('ip', 'addr', 'show', 'dev', interface,
|
||||
'scope', 'global')
|
||||
for line in out.split('\n'):
|
||||
fields = line.split()
|
||||
if fields and fields[0] == 'inet':
|
||||
if fields[-2] in ('secondary', 'dynamic', ):
|
||||
params = fields[1:-2]
|
||||
else:
|
||||
params = fields[1:-1]
|
||||
processutils.execute(*_ip_bridge_cmd('del', params,
|
||||
fields[-1]),
|
||||
check_exit_code=[0, 2, 254])
|
||||
processutils.execute(*_ip_bridge_cmd('add', params,
|
||||
bridge),
|
||||
check_exit_code=[0, 2, 254])
|
||||
for fields in old_routes:
|
||||
processutils.execute('ip', 'route', 'add', *fields)
|
||||
|
||||
_update_bridge_routes(interface, bridge)
|
||||
# NOTE(sean-k-mooney):
|
||||
# The bridge mtu cannont be set until after an
|
||||
# interface is added due to bug:
|
||||
|
||||
@ -11,7 +11,6 @@
|
||||
# under the License.
|
||||
|
||||
import mock
|
||||
import os.path
|
||||
import testtools
|
||||
|
||||
import fixtures
|
||||
@ -72,100 +71,80 @@ class LinuxNetTest(testtools.TestCase):
|
||||
mock_ip_set.assert_has_calls(set_calls)
|
||||
mock_set_mtu.assert_called_once_with('vlan123', 1500)
|
||||
|
||||
@mock.patch.object(processutils, "execute")
|
||||
@mock.patch.object(ip_lib, "exists", return_value=True)
|
||||
def test_ensure_bridge_exists(self, mock_dev_exists, mock_exec):
|
||||
@mock.patch.object(linux_net, "_ensure_bridge_privileged")
|
||||
@mock.patch.object(linux_net, "_ensure_bridge_filtering")
|
||||
def test_ensure_bridge(self, mock_filtering, mock_priv):
|
||||
linux_net.ensure_bridge("br0", None, filtering=False)
|
||||
|
||||
mock_exec.assert_not_called()
|
||||
mock_dev_exists.assert_called_once_with("br0")
|
||||
mock_priv.assert_called_once_with("br0", None, None, True,
|
||||
filtering=False, mtu=None)
|
||||
mock_filtering.assert_not_called()
|
||||
|
||||
linux_net.ensure_bridge("br0", None, filtering=True)
|
||||
mock_filtering.assert_called_once_with("br0", True)
|
||||
|
||||
@mock.patch.object(ip_lib, "exists", return_value=False)
|
||||
@mock.patch.object(processutils, "execute")
|
||||
def test_ensure_bridge_addbr_exception(self, mock_exec, mock_dev_exists):
|
||||
mock_exec.side_effect = ValueError()
|
||||
@mock.patch.object(ip_lib, "add")
|
||||
def test_ensure_bridge_addbr_exception(self, mock_add, mock_dev_exists):
|
||||
mock_add.side_effect = ValueError()
|
||||
with testtools.ExpectedException(ValueError):
|
||||
linux_net.ensure_bridge("br0", None, filtering=False)
|
||||
|
||||
@mock.patch.object(ip_lib, "set")
|
||||
@mock.patch.object(processutils, "execute")
|
||||
@mock.patch.object(ip_lib, "exists", side_effect=[False, True])
|
||||
def test_ensure_bridge_concurrent_add(self, mock_dev_exists, mock_exec,
|
||||
mock_ip_set):
|
||||
mock_exec.side_effect = [ValueError(), 0, 0, 0]
|
||||
linux_net.ensure_bridge("br0", None, filtering=False)
|
||||
|
||||
calls = [mock.call('brctl', 'addbr', 'br0'),
|
||||
mock.call('brctl', 'setfd', 'br0', 0),
|
||||
mock.call('brctl', 'stp', 'br0', "off")]
|
||||
mock_exec.assert_has_calls(calls)
|
||||
mock_dev_exists.assert_has_calls([mock.call("br0"), mock.call("br0")])
|
||||
mock_ip_set.assert_called_once_with('br0', state='up')
|
||||
|
||||
@mock.patch.object(ip_lib, "add")
|
||||
@mock.patch.object(ip_lib, "set")
|
||||
@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(ip_lib, "exists", return_value=False)
|
||||
def test_ensure_bridge_mtu_not_called(self, mock_dev_exists, mock_exec,
|
||||
mock_path_exists, mock_set_mtu, mock_ip_set):
|
||||
@mock.patch.object(linux_net, "_disable_ipv6")
|
||||
@mock.patch.object(linux_net, "_update_bridge_routes")
|
||||
@mock.patch.object(ip_lib, "exists")
|
||||
def test_ensure_bridge_priv_mtu_not_called(self, mock_dev_exists,
|
||||
mock_routes, mock_disable_ipv6, mock_set_mtu, mock_ip_set, mock_add):
|
||||
"""This test validates that mtus are updated only if an interface
|
||||
is added to the bridge
|
||||
"""
|
||||
mock_dev_exists.return_value = False
|
||||
linux_net._ensure_bridge_privileged("fake-bridge", None,
|
||||
None, False, mtu=1500)
|
||||
mock_set_mtu.assert_not_called()
|
||||
mock_ip_set.assert_called_once_with('fake-bridge', state='up')
|
||||
|
||||
@mock.patch.object(ip_lib, "add")
|
||||
@mock.patch.object(ip_lib, "set")
|
||||
@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(ip_lib, "exists", return_value=False)
|
||||
def test_ensure_bridge_mtu_order(self, mock_dev_exists, mock_exec,
|
||||
mock_path_exists, mock_set_mtu, mock_ip_set):
|
||||
@mock.patch.object(linux_net, "_disable_ipv6")
|
||||
@mock.patch.object(linux_net, "_update_bridge_routes")
|
||||
@mock.patch.object(ip_lib, "exists")
|
||||
def test_ensure_bridge_priv_mtu_order(self, mock_dev_exists, mock_routes,
|
||||
mock_disable_ipv6, mock_set_mtu, mock_ip_set, mock_add):
|
||||
"""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
|
||||
"""
|
||||
mock_dev_exists.return_value = next(lambda: (yield False),
|
||||
(yield True))
|
||||
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)
|
||||
calls = [mock.call('fake-bridge', state = 'up'),
|
||||
mock.call('fake-interface', state='up')]
|
||||
mock.call('fake-interface', master='fake-bridge', state='up',
|
||||
check_exit_code=[0, 2, 254])]
|
||||
mock_ip_set.assert_has_calls(calls)
|
||||
|
||||
@mock.patch.object(ip_lib, "set")
|
||||
@mock.patch.object(os.path, "exists", return_value=False)
|
||||
@mock.patch.object(processutils, "execute")
|
||||
@mock.patch.object(ip_lib, "exists", return_value=False)
|
||||
def test_ensure_bridge_new_ipv4(self, mock_dev_exists, mock_exec,
|
||||
mock_path_exists, mock_ip_set):
|
||||
linux_net.ensure_bridge("br0", None, filtering=False)
|
||||
@mock.patch('six.moves.builtins.open')
|
||||
@mock.patch("os.path.exists")
|
||||
def test__disable_ipv6(self, mock_exists, mock_open):
|
||||
|
||||
calls = [mock.call('brctl', 'addbr', 'br0'),
|
||||
mock.call('brctl', 'setfd', 'br0', 0),
|
||||
mock.call('brctl', 'stp', 'br0', "off")]
|
||||
mock_exec.assert_has_calls(calls)
|
||||
mock_dev_exists.assert_called_once_with("br0")
|
||||
mock_ip_set.assert_called_once_with('br0', state='up')
|
||||
exists_path = "/proc/sys/net/ipv6/conf/br0/disable_ipv6"
|
||||
mock_exists.return_value = False
|
||||
|
||||
@mock.patch.object(ip_lib, "set")
|
||||
@mock.patch.object(os.path, "exists", return_value=True)
|
||||
@mock.patch.object(processutils, "execute")
|
||||
@mock.patch.object(ip_lib, "exists", return_value=False)
|
||||
def test_ensure_bridge_new_ipv6(self, mock_dev_exists, mock_exec,
|
||||
mock_path_exists, mock_ip_set):
|
||||
linux_net.ensure_bridge("br0", None, filtering=False)
|
||||
linux_net._disable_ipv6("br0")
|
||||
mock_exists.assert_called_once_with(exists_path)
|
||||
mock_open.assert_not_called()
|
||||
|
||||
calls = [mock.call('brctl', 'addbr', 'br0'),
|
||||
mock.call('brctl', 'setfd', 'br0', 0),
|
||||
mock.call('brctl', 'stp', 'br0', "off"),
|
||||
mock.call('tee', '/proc/sys/net/ipv6/conf/br0/disable_ipv6',
|
||||
check_exit_code=[0, 1], process_input='1')]
|
||||
mock_exec.assert_has_calls(calls)
|
||||
mock_dev_exists.assert_called_once_with("br0")
|
||||
mock_ip_set.assert_called_once_with('br0', state='up')
|
||||
mock_exists.reset_mock()
|
||||
mock_exists.return_value = True
|
||||
linux_net._disable_ipv6("br0")
|
||||
mock_exists.assert_called_once_with(exists_path)
|
||||
mock_open.assert_called_once_with(exists_path, 'w')
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user