openstack
/
nova
Code Issues Proposed changes

Address nits from privsep series 

- Add missing coverage for 'iptables_set_rules' and 'iptables_get_rules'
- Use a class-level mock for common mocks
- Use 'fixtures.MockPatch' instead of 'fixtures.MonkeyPatch'
- Move exception handling for the 'ovs_plug' and 'ovs_unplug' functions
  to the functions themselves, allowing us to include the full command
  attempted in the exception message
- OvsConfigurationFailure is renamed to OVSConfigurationFailure

Change-Id: Ib8bb847598db2a6b8de95fdb222279cd22eaabcc
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>
This commit is contained in:
Stephen Finucane 2019-08-19 15:01:39 +01:00
parent 52af043940
commit 8f6c3049a4
6 changed files with 135 additions and 67 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
nova/exception.py
View File

@ -857,7 +857,7 @@ class VifDetailsMissingMacvtapParameters(Invalid):
" correct.")
class OvsConfigurationFailure(NovaException):
class OVSConfigurationFailure(NovaException):
msg_fmt = _("OVS configuration failed with: %(inner_exception)s.")

20
nova/network/linux_net.py
View File

@ -1577,15 +1577,8 @@ class LinuxOVSInterfaceDriver(LinuxNetInterfaceDriver):
if not nova.privsep.linux_net.device_exists(dev):
bridge = CONF.linuxnet_ovs_integration_bridge
try:
nova.privsep.linux_net.ovs_plug(CONF.ovs_vsctl_timeout,
bridge, dev, mac_address)
except Exception as e:
LOG.error('Unable to execute ovs-plug. Exception: '
'%(exception)s',
{'exception': e})
raise exception.OvsConfigurationFailure(inner_exception=e)
nova.privsep.linux_net.ovs_plug(CONF.ovs_vsctl_timeout,
bridge, dev, mac_address)
nova.privsep.linux_net.set_device_macaddr(
dev, mac_address)
nova.privsep.linux_net.set_device_mtu(dev, network.get('mtu'))
@ -1612,13 +1605,8 @@ class LinuxOVSInterfaceDriver(LinuxNetInterfaceDriver):
def unplug(self, network):
dev = self.get_dev(network)
bridge = CONF.linuxnet_ovs_integration_bridge
try:
nova.privsep.linux_net.ovs_unplug(CONF.ovs_vsctl_timeout,
bridge, dev)
except Exception as e:
LOG.error('Unable to execute ovs-unplug. Exception: %(exception)s',
{'exception': e})
raise exception.OvsConfigurationFailure(inner_exception=e)
nova.privsep.linux_net.ovs_unplug(CONF.ovs_vsctl_timeout, bridge, dev)
return dev

35
nova/privsep/linux_net.py
View File

@ -25,6 +25,7 @@ from oslo_concurrency import processutils
from oslo_log import log as logging
from oslo_utils import excutils
from nova import exception
import nova.privsep.linux_net
@ -380,15 +381,21 @@ def start_ra(conf_path, pid_path):
@nova.privsep.sys_admin_pctxt.entrypoint
def ovs_plug(timeout, bridge, dev, mac_address):
processutils.execute('ovs-vsctl', '--timeout=%s' % timeout,
'--', '--may-exist', 'add-port', bridge, dev,
'--', 'set', 'Interface', dev, 'type=internal',
'--', 'set', 'Interface', dev,
'external-ids:iface-id=%s' % dev,
'--', 'set', 'Interface', dev,
'external-ids:iface-status=active',
'--', 'set', 'Interface', dev,
'external-ids:attached-mac=%s' % mac_address)
cmd = ['ovs-vsctl', '--timeout=%s' % timeout,
'--', '--may-exist', 'add-port', bridge, dev,
'--', 'set', 'Interface', dev, 'type=internal',
'--', 'set', 'Interface', dev,
'external-ids:iface-id=%s' % dev,
'--', 'set', 'Interface', dev,
'external-ids:iface-status=active',
'--', 'set', 'Interface', dev,
'external-ids:attached-mac=%s' % mac_address]
try:
processutils.execute(*cmd)
except Exception as e:
LOG.error('Unable to execute %(cmd)s. Exception: %(exception)s',
{'cmd': cmd, 'exception': e})
raise exception.OVSConfigurationFailure(inner_exception=e)
@nova.privsep.sys_admin_pctxt.entrypoint
@ -402,5 +409,11 @@ def ovs_drop_nondhcp(bridge, mac_address):
@nova.privsep.sys_admin_pctxt.entrypoint
def ovs_unplug(timeout, bridge, dev):
processutils.execute('ovs-vsctl', '--timeout=%s' % timeout,
'--', '--if-exists', 'del-port', bridge, dev)
cmd = ['ovs-vsctl', '--timeout=%s' % timeout,
'--', '--if-exists', 'del-port', bridge, dev]
try:
processutils.execute(*cmd)
except Exception as e:
LOG.error('Unable to execute %(cmd)s. Exception: %(exception)s',
{'cmd': cmd, 'exception': e})
raise exception.OVSConfigurationFailure(inner_exception=e)

5
nova/tests/fixtures.py
View File

@ -98,9 +98,8 @@ class ServiceFixture(fixtures.Fixture):
# NOTE(mikal): we don't have root to manipulate iptables, so just
# zero that bit out.
self.useFixture(fixtures.MonkeyPatch(
'nova.network.linux_net.IptablesManager._apply',
lambda _: None))
self.useFixture(fixtures.MockPatch(
'nova.network.linux_net.IptablesManager._apply'))
with mock.patch('nova.context.get_admin_context',
return_value=self.ctxt):

18
nova/tests/unit/network/test_linux_net.py
View File

@ -16,7 +16,6 @@
import calendar
import datetime
import os
import re
import mock
import netifaces
@ -648,24 +647,17 @@ class LinuxNetworkTestCase(test.NoDBTestCase):
@mock.patch('nova.privsep.linux_net.device_exists',
return_value=False)
@mock.patch(
'nova.privsep.linux_net.ovs_plug',
side_effect=processutils.ProcessExecutionError('specific_error'))
@mock.patch('nova.privsep.linux_net.ovs_plug',
side_effect=exception.OVSConfigurationFailure('foo'))
def test_linux_ovs_driver_plug_exception(self, mock_plug,
mock_device_exists):
self.flags(fake_network=False)
driver = linux_net.LinuxOVSInterfaceDriver()
exc = self.assertRaises(exception.OvsConfigurationFailure,
driver.plug,
{'uuid': 'fake_network_uuid'}, 'fake_mac')
self.assertRegex(
str(exc),
re.compile("OVS configuration failed with: .*specific_error.*",
re.DOTALL))
self.assertIsInstance(exc.kwargs['inner_exception'],
processutils.ProcessExecutionError)
self.assertRaises(exception.OVSConfigurationFailure,
driver.plug, {'uuid': 'fake_network_uuid'},
'fake_mac')
mock_plug.assert_called_once()
mock_device_exists.assert_called_once()

122
nova/tests/unit/privsep/test_linux_net.py
View File

@ -16,12 +16,15 @@
import mock
from oslo_concurrency import processutils
import six
from nova import exception
import nova.privsep.linux_net
from nova import test
from nova.tests import fixtures
@mock.patch('oslo_concurrency.processutils.execute')
class LinuxNetTestCase(test.NoDBTestCase):
"""Test networking helpers."""
@ -29,21 +32,30 @@ class LinuxNetTestCase(test.NoDBTestCase):
super(LinuxNetTestCase, self).setUp()
self.useFixture(fixtures.PrivsepFixture())
@mock.patch('oslo_concurrency.processutils.execute',
return_value=('', ''))
def test_set_device_mtu_default(self, mock_exec):
nova.privsep.linux_net.set_device_mtu('fake-dev', None)
mock_exec.assert_has_calls([])
def test_bridge_add_interface(self, mock_execute):
nova.privsep.linux_net.bridge_add_interface('br0', 'eth0')
cmd = ['brctl', 'addif', 'br0', 'eth0']
mock_execute.assert_called_once_with(*cmd, check_exit_code=False)
@mock.patch('os.path.exists')
def test_device_exists(self, mock_exists, mock_execute):
nova.privsep.linux_net.device_exists('eth0')
mock_exists('/sys/class/net/eth0')
def test_set_device_mtu_default(self, mock_execute):
mock_execute.return_value = ('', '')
nova.privsep.linux_net.set_device_mtu('fake-dev', None)
mock_execute.assert_has_calls([])
def test_set_device_mtu_actual(self, mock_execute):
mock_execute.return_value = ('', '')
@mock.patch('oslo_concurrency.processutils.execute',
return_value=('', ''))
def test_set_device_mtu_actual(self, mock_exec):
nova.privsep.linux_net.set_device_mtu('fake-dev', 1500)
mock_exec.assert_has_calls([
mock_execute.assert_has_calls([
mock.call('ip', 'link', 'set', 'fake-dev', 'mtu',
1500, check_exit_code=[0, 2, 254])])
@mock.patch('oslo_concurrency.processutils.execute')
@mock.patch('nova.privsep.linux_net._set_device_enabled_inner')
def test_create_tap_dev(self, mock_enabled, mock_execute):
nova.privsep.linux_net.create_tap_dev('tap42')
@ -55,14 +67,12 @@ class LinuxNetTestCase(test.NoDBTestCase):
mock_enabled.assert_called_once_with('tap42')
@mock.patch('os.path.exists', return_value=True)
@mock.patch('oslo_concurrency.processutils.execute')
def test_create_tap_skipped_when_exists(self, mock_execute, mock_exists):
def test_create_tap_skipped_when_exists(self, mock_exists, mock_execute):
nova.privsep.linux_net.create_tap_dev('tap42')
mock_exists.assert_called_once_with('/sys/class/net/tap42')
mock_execute.assert_not_called()
@mock.patch('oslo_concurrency.processutils.execute')
@mock.patch('nova.privsep.linux_net._set_device_enabled_inner')
@mock.patch('nova.privsep.linux_net._set_device_macaddr_inner')
def test_create_tap_dev_mac(self, mock_set_macaddr, mock_enabled,
@ -78,7 +88,6 @@ class LinuxNetTestCase(test.NoDBTestCase):
mock_set_macaddr.assert_has_calls([
mock.call('tap42', '00:11:22:33:44:55')])
@mock.patch('oslo_concurrency.processutils.execute')
@mock.patch('nova.privsep.linux_net._set_device_enabled_inner')
def test_create_tap_dev_fallback_to_tunctl(self, mock_enabled,
mock_execute):
@ -94,7 +103,6 @@ class LinuxNetTestCase(test.NoDBTestCase):
])
mock_enabled.assert_called_once_with('tap42')
@mock.patch('oslo_concurrency.processutils.execute')
@mock.patch('nova.privsep.linux_net._set_device_enabled_inner')
def test_create_tap_dev_multiqueue(self, mock_enabled, mock_execute):
nova.privsep.linux_net.create_tap_dev(
@ -106,7 +114,6 @@ class LinuxNetTestCase(test.NoDBTestCase):
])
mock_enabled.assert_called_once_with('tap42')
@mock.patch('oslo_concurrency.processutils.execute')
def test_create_tap_dev_multiqueue_tunctl_raises(self, mock_execute):
# if creation of a tap by the means of ip command fails,
# create_tap_dev() will try to do that by the means of tunctl
@ -118,8 +125,7 @@ class LinuxNetTestCase(test.NoDBTestCase):
@mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
return_value=False)
@mock.patch('oslo_concurrency.processutils.execute')
def test_enable_ipv4_forwarding_required(self, mock_execute, mock_check):
def test_enable_ipv4_forwarding_required(self, mock_check, mock_execute):
nova.privsep.linux_net.enable_ipv4_forwarding()
mock_check.assert_called_once()
mock_execute.assert_called_once_with(
@ -127,13 +133,11 @@ class LinuxNetTestCase(test.NoDBTestCase):
@mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
return_value=True)
@mock.patch('oslo_concurrency.processutils.execute')
def test_enable_ipv4_forwarding_redundant(self, mock_execute, mock_check):
def test_enable_ipv4_forwarding_redundant(self, mock_check, mock_execute):
nova.privsep.linux_net.enable_ipv4_forwarding()
mock_check.assert_called_once()
mock_execute.assert_not_called()
@mock.patch('oslo_concurrency.processutils.execute')
def test_modify_ebtables_insert_rule(self, mock_execute):
table = 'filter'
rule = 'INPUT -p ARP -i %s --arp-ip-dst %s -j DROP'.split()
@ -143,7 +147,6 @@ class LinuxNetTestCase(test.NoDBTestCase):
cmd = ['ebtables', '--concurrent', '-t', table] + ['-I'] + rule
mock_execute.assert_called_once_with(*cmd, check_exit_code=[0])
@mock.patch('oslo_concurrency.processutils.execute')
def test_modify_ebtables_remove_rule(self, mock_execute):
table = 'filter'
rule = 'INPUT -p ARP -i %s --arp-ip-dst %s -j DROP'.split()
@ -153,9 +156,82 @@ class LinuxNetTestCase(test.NoDBTestCase):
cmd = ['ebtables', '--concurrent', '-t', table] + ['-D'] + rule
mock_execute.assert_called_once_with(*cmd, check_exit_code=[0])
@mock.patch('oslo_concurrency.processutils.execute')
def test_add_vlan(self, mock_execute):
nova.privsep.linux_net.add_vlan('eth0', 'vlan_name', 1)
cmd = ['ip', 'link', 'add', 'link', 'eth0', 'name', 'vlan_name',
'type', 'vlan', 'id', 1]
mock_execute.assert_called_once_with(*cmd, check_exit_code=[0, 2, 254])
def test_iptables_get_rules(self, mock_execute):
nova.privsep.linux_net.iptables_get_rules()
cmd = ['iptables-save', '-c']
mock_execute.assert_called_once_with(*cmd, attempts=5)
def test_iptables_get_rules_ipv6(self, mock_execute):
nova.privsep.linux_net.iptables_get_rules(ipv4=False)
cmd = ['ip6tables-save', '-c']
mock_execute.assert_called_once_with(*cmd, attempts=5)
def test_iptables_set_rules(self, mock_execute):
rules = [
"# Generated by iptables-save v1.8.2 on Mon Aug 19 11:25:48 2019",
"*security",
":INPUT ACCEPT [508089:729290563]",
":FORWARD ACCEPT [247333:239588306]",
":OUTPUT ACCEPT [340769:25538424]",
":FORWARD_direct - [0:0]",
":INPUT_direct - [0:0]",
":OUTPUT_direct - [0:0]",
"-A INPUT -j INPUT_direct",
"-A FORWARD -j FORWARD_direct",
"-A OUTPUT -j OUTPUT_direct",
"COMMIT",
"# Completed on Mon Aug 19 11:25:48 2019",
]
rules_str = six.b('\n'.join(rules))
nova.privsep.linux_net.iptables_set_rules(rules)
cmd = ['iptables-restore', '-c']
mock_execute.assert_called_once_with(*cmd, process_input=rules_str,
attempts=5)
def test_iptables_set_rules_ipv6(self, mock_execute):
rules = [
"# Generated by ip6tables-save v1.8.2 on Mon Aug 19 12:00:29 2019",
"*security",
":INPUT ACCEPT [56:10115]",
":FORWARD ACCEPT [0:0]",
":OUTPUT ACCEPT [147:15301]",
":FORWARD_direct - [0:0]",
":INPUT_direct - [0:0]",
":OUTPUT_direct - [0:0]",
"-A INPUT -j INPUT_direct",
"-A FORWARD -j FORWARD_direct",
"-A OUTPUT -j OUTPUT_direct",
"COMMIT",
"# Completed on Mon Aug 19 12:00:29 2019",
]
rules_str = six.b('\n'.join(rules))
nova.privsep.linux_net.iptables_set_rules(rules, ipv4=False)
cmd = ['ip6tables-restore', '-c']
mock_execute.assert_called_once_with(*cmd, process_input=rules_str,
attempts=5)
def test_ovs_plug__fail(self, mock_execute):
mock_execute.side_effect = processutils.ProcessExecutionError
exc = self.assertRaises(exception.OVSConfigurationFailure,
nova.privsep.linux_net.ovs_plug,
60, 'int-br', 'eth0', '00:14:22:01:23:45')
self.assertIsInstance(exc.kwargs['inner_exception'],
processutils.ProcessExecutionError)
def test_ovs_unplug__fail(self, mock_execute):
mock_execute.side_effect = processutils.ProcessExecutionError
exc = self.assertRaises(exception.OVSConfigurationFailure,
nova.privsep.linux_net.ovs_unplug,
60, 'int-br', 'eth0')
self.assertIsInstance(exc.kwargs['inner_exception'],
processutils.ProcessExecutionError)