From 4075af6a1154246c9653549763e470bb74e7500f Mon Sep 17 00:00:00 2001 From: David Ames Date: Tue, 7 Jan 2020 15:21:55 -0800 Subject: [PATCH] Make ovs_use_veth a config option This was originally fixed in commit 7578326 but this caused problems. It was subsequently reverted in commit 6d2e9ee. This change uses a common DHCPAgentContext and takes care to check for a pre-existing setting in the dhcp_agent.ini. Only allowing a config change if there is no pre-existing setting. Please review and merge charm-helpers PR: https://github.com/juju/charm-helpers/pull/422 Partial-Bug: #1831935 func-test-pr: https://github.com/openstack-charmers/zaza-openstack-tests/pull/157 Change-Id: I4848a3246d3450540acb8d2f479dfa2e7767be60 --- config.yaml | 9 + .../charmhelpers/contrib/openstack/context.py | 166 +++++++++++++++++- hooks/neutron_ovs_context.py | 27 --- hooks/neutron_ovs_utils.py | 11 +- templates/icehouse/dhcp_agent.ini | 2 +- templates/mitaka/dhcp_agent.ini | 2 +- templates/ocata/dhcp_agent.ini | 2 +- tests/bundles/disco_stein.yaml | 89 ---------- tests/tests.yaml | 1 - unit_tests/test_neutron_ovs_context.py | 136 -------------- unit_tests/test_neutron_ovs_utils.py | 1 + 11 files changed, 184 insertions(+), 262 deletions(-) delete mode 100644 tests/bundles/disco_stein.yaml diff --git a/config.yaml b/config.yaml index c82b2c99..c8f38f4a 100644 --- a/config.yaml +++ b/config.yaml @@ -353,3 +353,12 @@ options: Can be used to avoid excessive memory consumption. WARNING: Should be NOT LESS than 25. (Available from Stein) + ovs-use-veth: + type: string + default: + description: | + "True" or "False" string value. It is safe to leave this option unset. + This option allows the DHCP agent to use a veth interface for OVS in + order to support kernels with limited namespace support. i.e. Trusty. + Changing the value after neutron DHCP agents are created will break + access. The charm will go into a blocked state if this is attempted. diff --git a/hooks/charmhelpers/contrib/openstack/context.py b/hooks/charmhelpers/contrib/openstack/context.py index b171a06d..8ab0f2cd 100644 --- a/hooks/charmhelpers/contrib/openstack/context.py +++ b/hooks/charmhelpers/contrib/openstack/context.py @@ -25,6 +25,10 @@ from subprocess import check_call, CalledProcessError import six +from charmhelpers.contrib.openstack.audits.openstack_security_guide import ( + _config_ini as config_ini +) + from charmhelpers.fetch import ( apt_install, filter_installed_packages, @@ -730,6 +734,10 @@ class AMQPContext(OSContextGenerator): if notification_format: ctxt['notification_format'] = notification_format + notification_topics = conf.get('notification-topics', None) + if notification_topics: + ctxt['notification_topics'] = notification_topics + send_notifications_to_logs = conf.get('send-notifications-to-logs', None) if send_notifications_to_logs: ctxt['send_notifications_to_logs'] = send_notifications_to_logs @@ -1940,7 +1948,7 @@ class VolumeAPIContext(InternalEndpointContext): as well as the catalog_info string that would be supplied. Returns a dict containing the volume_api_version and the volume_catalog_info. """ - rel = os_release(self.pkg, base='icehouse') + rel = os_release(self.pkg) version = '2' if CompareOpenStackReleases(rel) >= 'pike': version = '3' @@ -2140,7 +2148,7 @@ class VersionsContext(OSContextGenerator): self.pkg = pkg def __call__(self): - ostack = os_release(self.pkg, base='icehouse') + ostack = os_release(self.pkg) osystem = lsb_release()['DISTRIB_CODENAME'].lower() return { 'openstack_release': ostack, @@ -2240,3 +2248,157 @@ class HostInfoContext(OSContextGenerator): self.use_fqdn_hint_cb() if self.use_fqdn_hint_cb else False) } return ctxt + + +def validate_ovs_use_veth(*args, **kwargs): + """Validate OVS use veth setting for dhcp agents + + The ovs_use_veth setting is considered immutable as it will break existing + deployments. Historically, we set ovs_use_veth=True in dhcp_agent.ini. It + turns out this is no longer necessary. Ideally, all new deployments would + have this set to False. + + This function validates that the config value does not conflict with + previously deployed settings in dhcp_agent.ini. + + See LP Bug#1831935 for details. + + :returns: Status state and message + :rtype: Union[(None, None), (string, string)] + """ + existing_ovs_use_veth = ( + DHCPAgentContext.get_existing_ovs_use_veth()) + config_ovs_use_veth = DHCPAgentContext.parse_ovs_use_veth() + + # Check settings are set and not None + if existing_ovs_use_veth is not None and config_ovs_use_veth is not None: + # Check for mismatch between existing config ini and juju config + if existing_ovs_use_veth != config_ovs_use_veth: + # Stop the line to avoid breakage + msg = ( + "The existing setting for dhcp_agent.ini ovs_use_veth, {}, " + "does not match the juju config setting, {}. This may lead to " + "VMs being unable to receive a DHCP IP. Either change the " + "juju config setting or dhcp agents may need to be recreated." + .format(existing_ovs_use_veth, config_ovs_use_veth)) + log(msg, ERROR) + return ( + "blocked", + "Mismatched existing and configured ovs-use-veth. See log.") + + # Everything is OK + return None, None + + +class DHCPAgentContext(OSContextGenerator): + + def __call__(self): + """Return the DHCPAGentContext. + + Return all DHCP Agent INI related configuration. + ovs unit is attached to (as a subordinate) and the 'dns_domain' from + the neutron-plugin-api relations (if one is set). + + :returns: Dictionary context + :rtype: Dict + """ + + ctxt = {} + dnsmasq_flags = config('dnsmasq-flags') + if dnsmasq_flags: + ctxt['dnsmasq_flags'] = config_flags_parser(dnsmasq_flags) + ctxt['dns_servers'] = config('dns-servers') + + neutron_api_settings = NeutronAPIContext()() + + ctxt['debug'] = config('debug') + ctxt['instance_mtu'] = config('instance-mtu') + ctxt['ovs_use_veth'] = self.get_ovs_use_veth() + + ctxt['enable_metadata_network'] = config('enable-metadata-network') + ctxt['enable_isolated_metadata'] = config('enable-isolated-metadata') + + if neutron_api_settings.get('dns_domain'): + ctxt['dns_domain'] = neutron_api_settings.get('dns_domain') + + # Override user supplied config for these plugins as these settings are + # mandatory + if config('plugin') in ['nvp', 'nsx', 'n1kv']: + ctxt['enable_metadata_network'] = True + ctxt['enable_isolated_metadata'] = True + + return ctxt + + @staticmethod + def get_existing_ovs_use_veth(): + """Return existing ovs_use_veth setting from dhcp_agent.ini. + + :returns: Boolean value of existing ovs_use_veth setting or None + :rtype: Optional[Bool] + """ + DHCP_AGENT_INI = "/etc/neutron/dhcp_agent.ini" + existing_ovs_use_veth = None + # If there is a dhcp_agent.ini file read the current setting + if os.path.isfile(DHCP_AGENT_INI): + # config_ini does the right thing and returns None if the setting is + # commented. + existing_ovs_use_veth = ( + config_ini(DHCP_AGENT_INI)["DEFAULT"].get("ovs_use_veth")) + # Convert to Bool if necessary + if isinstance(existing_ovs_use_veth, six.string_types): + return bool_from_string(existing_ovs_use_veth) + return existing_ovs_use_veth + + @staticmethod + def parse_ovs_use_veth(): + """Parse the ovs-use-veth config setting. + + Parse the string config setting for ovs-use-veth and return a boolean + or None. + + bool_from_string will raise a ValueError if the string is not falsy or + truthy. + + :raises: ValueError for invalid input + :returns: Boolean value of ovs-use-veth or None + :rtype: Optional[Bool] + """ + _config = config("ovs-use-veth") + # An unset parameter returns None. Just in case we will also check for + # an empty string: "". Ironically, (the problem we are trying to avoid) + # "False" returns True and "" returns False. + if _config is None or not _config: + # Return None + return + # bool_from_string handles many variations of true and false strings + # as well as upper and lowercases including: + # ['y', 'yes', 'true', 't', 'on', 'n', 'no', 'false', 'f', 'off'] + return bool_from_string(_config) + + def get_ovs_use_veth(self): + """Return correct ovs_use_veth setting for use in dhcp_agent.ini. + + Get the right value from config or existing dhcp_agent.ini file. + Existing has precedence. Attempt to default to "False" without + disrupting existing deployments. Handle existing deployments and + upgrades safely. See LP Bug#1831935 + + :returns: Value to use for ovs_use_veth setting + :rtype: Bool + """ + # If this is Trust return True + release = lsb_release()['DISTRIB_CODENAME'].lower() + if CompareHostReleases(release) <= 'trusty': + return True + # If there is an existing setting return that + _existing = self.get_existing_ovs_use_veth() + if _existing is not None: + return _existing + # If the config value is unset return False + _config = self.parse_ovs_use_veth() + if _config is None: + # New better default + return False + # If the config value is set return it + else: + return _config diff --git a/hooks/neutron_ovs_context.py b/hooks/neutron_ovs_context.py index ff296ba9..31ac1fbe 100644 --- a/hooks/neutron_ovs_context.py +++ b/hooks/neutron_ovs_context.py @@ -33,7 +33,6 @@ from charmhelpers.core.host import ( ) from charmhelpers.contrib.openstack import context from charmhelpers.contrib.openstack.utils import ( - config_flags_parser, get_host_ip, ) from charmhelpers.contrib.network.ip import ( @@ -288,32 +287,6 @@ class ZoneContext(OSContextGenerator): return ctxt -class DHCPAgentContext(ZoneContext): - - def __call__(self): - """Return the 'default_availability_zone' from the principal that this - ovs unit is attached to (as a subordinate) and the 'dns_domain' from - the neutron-plugin-api relations (if one is set). - - :returns: {} if no relation set, or - {'availability_zone': availability_zone from principal relation} - """ - ctxt = super(DHCPAgentContext, self).__call__() - - dnsmasq_flags = config('dnsmasq-flags') - if dnsmasq_flags: - ctxt['dnsmasq_flags'] = config_flags_parser(dnsmasq_flags) - ctxt['dns_servers'] = config('dns-servers') - - neutron_api_settings = NeutronAPIContext()() - if neutron_api_settings.get('dns_domain'): - ctxt['dns_domain'] = neutron_api_settings.get('dns_domain') - - ctxt['instance_mtu'] = config('instance-mtu') - - return ctxt - - class L3AgentContext(OSContextGenerator): def __call__(self): diff --git a/hooks/neutron_ovs_utils.py b/hooks/neutron_ovs_utils.py index 4c2fcd17..8cc1fb8a 100644 --- a/hooks/neutron_ovs_utils.py +++ b/hooks/neutron_ovs_utils.py @@ -47,9 +47,9 @@ from charmhelpers.contrib.network.ovs import ( ) from charmhelpers.core.hookenv import ( config, - status_set, - log, DEBUG, + log, + status_set, ) from charmhelpers.contrib.openstack.neutron import ( parse_bridge_mappings, @@ -61,6 +61,8 @@ from charmhelpers.contrib.openstack.context import ( DataPortContext, WorkerConfigContext, parse_data_port_mappings, + DHCPAgentContext, + validate_ovs_use_veth, ) from charmhelpers.core.host import ( lsb_release, @@ -214,11 +216,11 @@ METADATA_RESOURCE_MAP = OrderedDict([ DHCP_RESOURCE_MAP = OrderedDict([ (NEUTRON_DHCP_AGENT_CONF, { 'services': ['neutron-dhcp-agent'], - 'contexts': [neutron_ovs_context.DHCPAgentContext()], + 'contexts': [DHCPAgentContext()], }), (NEUTRON_DNSMASQ_CONF, { 'services': ['neutron-dhcp-agent'], - 'contexts': [neutron_ovs_context.DHCPAgentContext()], + 'contexts': [DHCPAgentContext()], }), ]) DVR_RESOURCE_MAP = OrderedDict([ @@ -945,6 +947,7 @@ def assess_status_func(configs): required_interfaces['neutron-plugin-api'] = ['neutron-plugin-api'] return make_assess_status_func( configs, required_interfaces, + charm_func=validate_ovs_use_veth, services=services(), ports=None) diff --git a/templates/icehouse/dhcp_agent.ini b/templates/icehouse/dhcp_agent.ini index a49feef7..fea5d285 100644 --- a/templates/icehouse/dhcp_agent.ini +++ b/templates/icehouse/dhcp_agent.ini @@ -17,4 +17,4 @@ dnsmasq_config_file = /etc/neutron/dnsmasq.conf enable_metadata_network = True enable_isolated_metadata = True -ovs_use_veth = True +ovs_use_veth = {{ ovs_use_veth }} diff --git a/templates/mitaka/dhcp_agent.ini b/templates/mitaka/dhcp_agent.ini index 41caf556..43324b7f 100644 --- a/templates/mitaka/dhcp_agent.ini +++ b/templates/mitaka/dhcp_agent.ini @@ -30,4 +30,4 @@ dhcp_domain = {{ dns_domain }} enable_metadata_network = True enable_isolated_metadata = True -ovs_use_veth = True +ovs_use_veth = {{ ovs_use_veth }} diff --git a/templates/ocata/dhcp_agent.ini b/templates/ocata/dhcp_agent.ini index 796a61a4..bb3de51d 100644 --- a/templates/ocata/dhcp_agent.ini +++ b/templates/ocata/dhcp_agent.ini @@ -31,4 +31,4 @@ enable_metadata_network = True force_metadata = True enable_isolated_metadata = True -ovs_use_veth = True +ovs_use_veth = {{ ovs_use_veth }} diff --git a/tests/bundles/disco_stein.yaml b/tests/bundles/disco_stein.yaml deleted file mode 100644 index 580c667f..00000000 --- a/tests/bundles/disco_stein.yaml +++ /dev/null @@ -1,89 +0,0 @@ -series: disco - -machines: - '0': - constraints: mem=3072M - '1': - '2': - '3': - '4': - '5': - '6': - -applications: - nova-compute: - charm: cs:~openstack-charmers-next/nova-compute - num_units: 1 - to: - - '1' - nova-cloud-controller: - charm: cs:~openstack-charmers-next/nova-cloud-controller - num_units: 1 - options: - network-manager: Neutron - to: - - '2' - rabbitmq-server: - charm: cs:~openstack-charmers-next/rabbitmq-server - num_units: 1 - to: - - '3' - keystone: - num_units: 1 - charm: cs:~openstack-charmers-next/keystone - num_units: 1 - to: - - '4' - glance: - charm: cs:~openstack-charmers-next/glance - num_units: 1 - to: - - '5' - neutron-api: - charm: cs:~openstack-charmers-next/neutron-api - num_units: 1 - to: - - '6' - percona-cluster: - charm: cs:~openstack-charmers-next/percona-cluster - num_units: 1 - to: - - '0' - neutron-openvswitch: - charm: ../../../neutron-openvswitch - -relations: -- - 'neutron-openvswitch:amqp' - - 'rabbitmq-server:amqp' -- - 'neutron-openvswitch:neutron-plugin' - - 'nova-compute:neutron-plugin' -- - 'neutron-openvswitch:neutron-plugin-api' - - 'neutron-api:neutron-plugin-api' -- - 'neutron-api:identity-service' - - 'keystone:identity-service' -- - 'neutron-api:shared-db' - - 'percona-cluster:shared-db' -- - 'neutron-api:amqp' - - 'rabbitmq-server:amqp' -- - 'nova-compute:amqp' - - 'rabbitmq-server:amqp' -- - 'nova-compute:image-service' - - 'glance:image-service' -- - 'glance:identity-service' - - 'keystone:identity-service' -- - 'glance:shared-db' - - 'percona-cluster:shared-db' -- - 'glance:amqp' - - 'rabbitmq-server:amqp' -- - 'keystone:shared-db' - - 'percona-cluster:shared-db' -- - 'nova-cloud-controller:shared-db' - - 'percona-cluster:shared-db' -- - 'nova-cloud-controller:amqp' - - 'rabbitmq-server:amqp' -- - 'nova-cloud-controller:identity-service' - - 'keystone:identity-service' -- - 'nova-cloud-controller:cloud-compute' - - 'nova-compute:cloud-compute' -- - 'nova-cloud-controller:image-service' - - 'glance:image-service' diff --git a/tests/tests.yaml b/tests/tests.yaml index cf415ce3..d7a703ff 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -12,7 +12,6 @@ gate_bundles: - bionic_rocky - bionic_stein - bionic_train - - disco_stein dev_bundles: - bionic_train diff --git a/unit_tests/test_neutron_ovs_context.py b/unit_tests/test_neutron_ovs_context.py index 38de206f..e8d3b5fa 100644 --- a/unit_tests/test_neutron_ovs_context.py +++ b/unit_tests/test_neutron_ovs_context.py @@ -314,142 +314,6 @@ class ZoneContextTest(CharmTestCase): unit='nova-compute/0') -class DHCPAgentContextTest(CharmTestCase): - - def setUp(self): - super(DHCPAgentContextTest, self).setUp(context, TO_PATCH) - self.config.side_effect = self.test_config.get - - def tearDown(self): - super(DHCPAgentContextTest, self).tearDown() - - @patch.object(charmhelpers.contrib.openstack.context, 'relation_get') - @patch.object(charmhelpers.contrib.openstack.context, 'relation_ids') - @patch.object(charmhelpers.contrib.openstack.context, 'related_units') - def test_default_availability_zone_not_provided(self, _runits, _rids, - _rget): - _runits.return_value = ['neutron-api/0'] - _rids.return_value = ['rid2'] - rdata = { - 'neutron-security-groups': 'True', - 'enable-dvr': 'True', - 'l2-population': 'True', - 'overlay-netweork-type': 'vxlan', - 'network-device-mtu': 1500, - 'dns-domain': 'openstack.example.' - } - _rget.side_effect = lambda *args, **kwargs: rdata - self.relation_ids.return_value = ['rid1'] - self.related_units.return_value = ['nova-compute/0'] - self.relation_get.return_value = None - self.assertEqual( - context.DHCPAgentContext()(), - {'dns_domain': 'openstack.example.', - 'instance_mtu': None, - 'dns_servers': None} - ) - self.relation_ids.assert_called_with('neutron-plugin') - self.relation_get.assert_called_once_with( - 'default_availability_zone', - rid='rid1', - unit='nova-compute/0') - - @patch.object(charmhelpers.contrib.openstack.context, 'relation_get') - @patch.object(charmhelpers.contrib.openstack.context, 'relation_ids') - @patch.object(charmhelpers.contrib.openstack.context, 'related_units') - def test_default_availability_zone_provided(self, _runits, _rids, _rget): - _runits.return_value = ['neutron-api/0'] - _rids.return_value = ['rid2'] - rdata = { - 'neutron-security-groups': 'True', - 'enable-dvr': 'True', - 'l2-population': 'True', - 'overlay-netweork-type': 'vxlan', - 'network-device-mtu': 1500, - 'dns-domain': 'openstack.example.' - } - _rget.side_effect = lambda *args, **kwargs: rdata - self.test_config.set('dns-servers', '8.8.8.8,4.4.4.4') - self.relation_ids.return_value = ['rid1'] - self.related_units.return_value = ['nova-compute/0'] - self.relation_get.return_value = 'nova' - self.assertEqual( - context.DHCPAgentContext()(), - {'availability_zone': 'nova', - 'dns_domain': 'openstack.example.', - 'instance_mtu': None, - 'dns_servers': '8.8.8.8,4.4.4.4'} - ) - self.relation_ids.assert_called_with('neutron-plugin') - self.relation_get.assert_called_once_with( - 'default_availability_zone', - rid='rid1', - unit='nova-compute/0') - - @patch.object(charmhelpers.contrib.openstack.context, 'relation_get') - @patch.object(charmhelpers.contrib.openstack.context, 'relation_ids') - @patch.object(charmhelpers.contrib.openstack.context, 'related_units') - def test_no_dns_domain(self, _runits, _rids, _rget): - _runits.return_value = ['neutron-api/0'] - _rids.return_value = ['rid2'] - rdata = { - 'neutron-security-groups': 'True', - 'enable-dvr': 'True', - 'l2-population': 'True', - 'overlay-netweork-type': 'vxlan', - 'network-device-mtu': 1500, - } - _rget.side_effect = lambda *args, **kwargs: rdata - self.test_config.set('dns-servers', '8.8.8.8') - self.relation_ids.return_value = ['rid1'] - self.related_units.return_value = ['nova-compute/0'] - self.relation_get.return_value = 'nova' - self.assertEqual( - context.DHCPAgentContext()(), - {'availability_zone': 'nova', - 'instance_mtu': None, - 'dns_servers': '8.8.8.8'} - ) - self.relation_ids.assert_called_with('neutron-plugin') - self.relation_get.assert_called_once_with( - 'default_availability_zone', - rid='rid1', - unit='nova-compute/0') - - @patch.object(charmhelpers.contrib.openstack.context, 'relation_get') - @patch.object(charmhelpers.contrib.openstack.context, 'relation_ids') - @patch.object(charmhelpers.contrib.openstack.context, 'related_units') - def test_dnsmasq_flags(self, _runits, _rids, _rget): - _runits.return_value = ['neutron-api/0'] - _rids.return_value = ['rid2'] - rdata = { - 'neutron-security-groups': 'True', - 'enable-dvr': 'True', - 'l2-population': 'True', - 'overlay-netweork-type': 'vxlan', - 'network-device-mtu': 1500, - } - _rget.side_effect = lambda *args, **kwargs: rdata - self.relation_ids.return_value = ['rid1'] - self.related_units.return_value = ['nova-compute/0'] - self.relation_get.return_value = None - self.test_config.set('dnsmasq-flags', 'dhcp-userclass=set:ipxe,iPXE,' - 'dhcp-match=set:ipxe,175,' - 'server=1.2.3.4') - self.assertEqual( - context.DHCPAgentContext()(), - { - 'dnsmasq_flags': { - 'dhcp-userclass': 'set:ipxe,iPXE', - 'dhcp-match': 'set:ipxe,175', - 'server': '1.2.3.4', - }, - 'instance_mtu': None, - 'dns_servers': None, - } - ) - - class L3AgentContextTest(CharmTestCase): def setUp(self): diff --git a/unit_tests/test_neutron_ovs_utils.py b/unit_tests/test_neutron_ovs_utils.py index f4e48651..66657b76 100644 --- a/unit_tests/test_neutron_ovs_utils.py +++ b/unit_tests/test_neutron_ovs_utils.py @@ -860,6 +860,7 @@ class TestNeutronOVSUtils(CharmTestCase): make_assess_status_func.assert_called_once_with( 'test-config', {'Test': True}, + charm_func=nutils.validate_ovs_use_veth, services='s1', ports=None)