Further deprecation of ext-port config option
Deprecate ext-port in favour of data-port and bridge-mappings. From now on ext-port value will be ignored if data-port is specified in the config. Log an error in the unit's log and display it in the unit's status so that the user is aware of misconfiguration. Update and add new unit tests to account for the introduced changes. Add a new functional test case that verifies correct handling of data-port and ext-port config options. Signed-off-by: Przemysław Lal <przemyslaw.lal@canonical.com> Change-Id: I4c6febbb56f9a61ff2519b60d2a746c9580a8f2d
This commit is contained in:
Przemysław Lal
parent
19a4537fc4
commit
30903919bf
@ -170,3 +170,8 @@ configured later using neutron net-create).
|
||||
This replaces the previous system of using ext-port, which always created a bridge
|
||||
called br-ex for external networks which was used implicitly by external router
|
||||
interfaces.
|
||||
|
||||
Note: If data-port is specified, the value of ext-port is ignored. This
|
||||
prevents misconfiguration of the charm. Aditionaly, in this scenario an error
|
||||
message is logged and the unit is marked as blocked in order to notify about
|
||||
the misconfiguration.
|
||||
|
||||
@ -30,6 +30,7 @@ from charmhelpers.contrib.openstack.utils import (
|
||||
os_application_version_set,
|
||||
CompareOpenStackReleases,
|
||||
os_release,
|
||||
sequence_status_check_functions,
|
||||
)
|
||||
from charmhelpers.core.unitdata import kv
|
||||
from collections import OrderedDict
|
||||
@ -605,6 +606,11 @@ def configure_ovs():
|
||||
|
||||
Note that the ext-port is deprecated and data-port/bridge-mappings are
|
||||
preferred.
|
||||
|
||||
Thus, if data-port is set, then ext-port is ignored (and if set, then
|
||||
it is removed from the set of bridges unless it is defined in
|
||||
bridge-mappings/data-port). A warning is issued, if both data-port and
|
||||
ext-port are set.
|
||||
"""
|
||||
status_set('maintenance', 'Configuring ovs')
|
||||
if not service_running('openvswitch-switch'):
|
||||
@ -619,13 +625,21 @@ def configure_ovs():
|
||||
add_bridge(INT_BRIDGE, brdata=brdata)
|
||||
add_bridge(EXT_BRIDGE, brdata=brdata)
|
||||
|
||||
# If data-port is defined in the config, we can ignore ext-port value
|
||||
# and log an error to the unit's log
|
||||
if config('data-port') and config('ext-port'):
|
||||
log("Both ext-port and data-port are set. ext-port is deprecated"
|
||||
" and is not used when data-port is set.", level=ERROR)
|
||||
|
||||
ext_port_ctx = None
|
||||
if use_dvr():
|
||||
ext_port_ctx = ExternalPortContext()()
|
||||
if ext_port_ctx and ext_port_ctx['ext_port']:
|
||||
add_bridge_port(EXT_BRIDGE, ext_port_ctx['ext_port'],
|
||||
ifdata=generate_external_ids(EXT_BRIDGE),
|
||||
portdata=generate_external_ids(EXT_BRIDGE))
|
||||
# Set ext-port only if data-port isn't defined.
|
||||
if not config('data-port') and ext_port_ctx and ext_port_ctx['ext_port']:
|
||||
add_bridge_port(
|
||||
EXT_BRIDGE, ext_port_ctx['ext_port'],
|
||||
ifdata=generate_external_ids(EXT_BRIDGE),
|
||||
portdata=generate_external_ids(EXT_BRIDGE))
|
||||
|
||||
modern_ovs = ovs_has_late_dpdk_init()
|
||||
|
||||
@ -868,6 +882,22 @@ def enable_local_dhcp():
|
||||
return not is_container() and config('enable-local-dhcp-and-metadata')
|
||||
|
||||
|
||||
def check_ext_port_data_port_config(configs):
|
||||
"""Checks that if data-port is set (other than None) then if ext-port is
|
||||
also set, add a warning to the status line.
|
||||
|
||||
:param configs: an OSConfigRender() instance.
|
||||
:type configs: OSConfigRender
|
||||
:returns: (status, message)
|
||||
:rtype: (str, str)
|
||||
"""
|
||||
if config('data-port') and config('ext-port'):
|
||||
return ("blocked", "ext-port set when data-port set: see config.yaml")
|
||||
# return 'unknown' as the lowest priority to not clobber an existing
|
||||
# status.
|
||||
return None, None
|
||||
|
||||
|
||||
def assess_status(configs):
|
||||
"""Assess status of current unit
|
||||
Decides what the state of the unit should be based on the current
|
||||
@ -909,7 +939,8 @@ def assess_status_func(configs, exclude_services=None):
|
||||
required_interfaces['neutron-plugin-api'] = ['neutron-plugin-api']
|
||||
return make_assess_status_func(
|
||||
configs, required_interfaces,
|
||||
charm_func=validate_ovs_use_veth,
|
||||
charm_func=sequence_status_check_functions(
|
||||
validate_ovs_use_veth, check_ext_port_data_port_config),
|
||||
services=services(exclude_services),
|
||||
ports=None)
|
||||
|
||||
|
||||
@ -58,9 +58,11 @@ tests:
|
||||
- zaza.openstack.charm_tests.neutron.tests.NeutronNetworkingTest
|
||||
- zaza.openstack.charm_tests.neutron.tests.NeutronOpenvSwitchTest
|
||||
- zaza.openstack.charm_tests.neutron.tests.NeutronOvsVsctlTest
|
||||
- zaza.openstack.charm_tests.neutron.tests.NeutronBridgePortMappingTest
|
||||
- migrate-ovn:
|
||||
- zaza.openstack.charm_tests.neutron.tests.NeutronNetworkingTest
|
||||
- zaza.openstack.charm_tests.neutron.tests.NeutronOvsVsctlTest
|
||||
- zaza.openstack.charm_tests.neutron.tests.NeutronBridgePortMappingTest
|
||||
- zaza.openstack.charm_tests.ovn.tests.OVSOVNMigrationTest
|
||||
- zaza.openstack.charm_tests.neutron.tests.NeutronNetworkingTest
|
||||
tests_options:
|
||||
|
||||
@ -1017,6 +1017,45 @@ class TestNeutronOVSUtils(CharmTestCase):
|
||||
call('br-ex', '127.0.0.1:80'),
|
||||
])
|
||||
|
||||
@patch.object(nutils, 'use_dvr')
|
||||
@patch('charmhelpers.contrib.network.ovs.charm_name')
|
||||
@patch('charmhelpers.contrib.openstack.context.config')
|
||||
def test_configure_ovs_ensure_ext_port_ignored(
|
||||
self, mock_config, mock_charm_name, mock_use_dvr):
|
||||
mock_use_dvr.return_value = True
|
||||
mock_charm_name.return_value = "neutron-openvswitch"
|
||||
mock_config.side_effect = self.test_config.get
|
||||
self.config.side_effect = self.test_config.get
|
||||
# configure ext-port and data-port at the same time
|
||||
self.test_config.set('ext-port', 'p0')
|
||||
self.ExternalPortContext.return_value = DummyContext(
|
||||
return_value={'ext_port': 'p0'})
|
||||
self.test_config.set('data-port', 'br-data:p1')
|
||||
self.test_config.set('bridge-mappings', 'net0:br-data')
|
||||
nutils.configure_ovs()
|
||||
# assert that p0 wasn't added to br-ex
|
||||
self.assertNotIn(call('br-ex', 'p0', ifdata=ANY, portdata=ANY),
|
||||
self.add_bridge_port.call_args_list)
|
||||
|
||||
@patch.object(nutils, 'use_dvr')
|
||||
@patch('charmhelpers.contrib.network.ovs.charm_name')
|
||||
@patch('charmhelpers.contrib.openstack.context.config')
|
||||
def test_configure_ovs_ensure_ext_port_used(
|
||||
self, mock_config, mock_charm_name, mock_use_dvr):
|
||||
mock_use_dvr.return_value = True
|
||||
mock_charm_name.return_value = "neutron-openvswitch"
|
||||
mock_config.side_effect = self.test_config.get
|
||||
self.config.side_effect = self.test_config.get
|
||||
self.test_config.set('ext-port', 'p0')
|
||||
self.ExternalPortContext.return_value = DummyContext(
|
||||
return_value={'ext_port': 'p0'})
|
||||
# leave data-port empty to simulate legacy config
|
||||
self.test_config.set('data-port', '')
|
||||
nutils.configure_ovs()
|
||||
# assert that p0 was added to br-ex
|
||||
self.assertIn(call('br-ex', 'p0', ifdata=ANY, portdata=ANY),
|
||||
self.add_bridge_port.call_args_list)
|
||||
|
||||
@patch.object(neutron_ovs_context, 'SharedSecretContext')
|
||||
def test_get_shared_secret(self, _dvr_secret_ctxt):
|
||||
_dvr_secret_ctxt.return_value = \
|
||||
@ -1034,7 +1073,25 @@ class TestNeutronOVSUtils(CharmTestCase):
|
||||
nutils.VERSION_PACKAGE
|
||||
)
|
||||
|
||||
@patch('charmhelpers.contrib.openstack.context.config')
|
||||
def test_check_ext_port_data_port_config(self, mock_config):
|
||||
mock_config.side_effect = self.test_config.get
|
||||
self.config.side_effect = self.test_config.get
|
||||
configs = [
|
||||
# conflicting, incorrect
|
||||
(('data-port', 'br-data:p0'), ('ext-port', 'p1'), 'blocked'),
|
||||
# deperacted but still correct
|
||||
(('data-port', ''), ('ext-port', 'p1'), None),
|
||||
# correct, modern
|
||||
(('data-port', 'br-data:p0'), ('ext-port', ''), None)]
|
||||
for (dp, ep, expected) in configs:
|
||||
self.test_config.set(*dp)
|
||||
self.test_config.set(*ep)
|
||||
status = nutils.check_ext_port_data_port_config(mock_config)
|
||||
self.assertIn(expected, status)
|
||||
|
||||
@patch.object(nutils, 'REQUIRED_INTERFACES')
|
||||
@patch.object(nutils, 'sequence_status_check_functions')
|
||||
@patch.object(nutils, 'services')
|
||||
@patch.object(nutils, 'determine_ports')
|
||||
@patch.object(nutils, 'make_assess_status_func')
|
||||
@ -1044,19 +1101,24 @@ class TestNeutronOVSUtils(CharmTestCase):
|
||||
make_assess_status_func,
|
||||
determine_ports,
|
||||
services,
|
||||
sequence_functions,
|
||||
REQUIRED_INTERFACES):
|
||||
services.return_value = 's1'
|
||||
determine_ports.return_value = 'p1'
|
||||
enable_nova_metadata.return_value = False
|
||||
sequence_functions.return_value = 'sequence_return'
|
||||
REQUIRED_INTERFACES.copy.return_value = {'Test': True}
|
||||
nutils.assess_status_func('test-config')
|
||||
# ports=None whilst port checks are disabled.
|
||||
make_assess_status_func.assert_called_once_with(
|
||||
'test-config',
|
||||
{'Test': True},
|
||||
charm_func=nutils.validate_ovs_use_veth,
|
||||
charm_func='sequence_return',
|
||||
services='s1',
|
||||
ports=None)
|
||||
sequence_functions.assert_called_once_with(
|
||||
nutils.validate_ovs_use_veth,
|
||||
nutils.check_ext_port_data_port_config)
|
||||
|
||||
def test_pause_unit_helper(self):
|
||||
with patch.object(nutils, '_pause_resume_helper') as prh:
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user