From 68154a3a9a9ef778195eee537033f22fd8ae5858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Lal?= Date: Tue, 9 Mar 2021 12:58:44 +0100 Subject: [PATCH] Move _ovs_additional_ids() and sequence_functions() to charmhelpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sync charmhelpers. Replace _ovs_additional_ids() calls with generate_external_ids() from charmhelpers. Replace sequence_functions() with sequence_status_check_functions() from charmhelpers. This allows to share helper functions between charm-neutron-gateway and charm-neutron-openvswitch. Change-Id: I8fc3b5c9e33e539b8b9c1d188acb8c79e8758244 Signed-off-by: Przemysław Lal --- .../charmhelpers/contrib/charmsupport/nrpe.py | 4 +- .../contrib/network/ovs/__init__.py | 27 ++++- .../contrib/openstack/amulet/utils.py | 1 + .../contrib/openstack/cert_utils.py | 27 ++++- .../charmhelpers/contrib/openstack/context.py | 16 ++- hooks/charmhelpers/contrib/openstack/utils.py | 103 ++++++++++++++++-- hooks/charmhelpers/core/hookenv.py | 11 ++ hooks/charmhelpers/core/host.py | 80 ++++++++++++-- hooks/charmhelpers/fetch/ubuntu.py | 64 ++++++++--- hooks/neutron_utils.py | 73 +++---------- unit_tests/test_neutron_utils.py | 28 +++-- 11 files changed, 314 insertions(+), 120 deletions(-) diff --git a/hooks/charmhelpers/contrib/charmsupport/nrpe.py b/hooks/charmhelpers/contrib/charmsupport/nrpe.py index c87cf489..e4cb06bc 100644 --- a/hooks/charmhelpers/contrib/charmsupport/nrpe.py +++ b/hooks/charmhelpers/contrib/charmsupport/nrpe.py @@ -337,10 +337,8 @@ class NRPE(object): "command": nrpecheck.command, } # If we were passed max_check_attempts, add that to the relation data - try: + if nrpecheck.max_check_attempts is not None: nrpe_monitors[nrpecheck.shortname]['max_check_attempts'] = nrpecheck.max_check_attempts - except AttributeError: - pass # update-status hooks are configured to firing every 5 minutes by # default. When nagios-nrpe-server is restarted, the nagios server diff --git a/hooks/charmhelpers/contrib/network/ovs/__init__.py b/hooks/charmhelpers/contrib/network/ovs/__init__.py index 3004125a..e1a3cc57 100644 --- a/hooks/charmhelpers/contrib/network/ovs/__init__.py +++ b/hooks/charmhelpers/contrib/network/ovs/__init__.py @@ -25,7 +25,7 @@ from charmhelpers.contrib.network.ovs import ovsdb as ch_ovsdb from charmhelpers.fetch import apt_install from charmhelpers.core.hookenv import ( - log, WARNING, INFO, DEBUG + log, WARNING, INFO, DEBUG, charm_name ) from charmhelpers.core.host import ( CompareHostReleases, @@ -666,3 +666,28 @@ def patch_ports_on_bridge(bridge): # reference to PEP479 just doing a return will provide a emtpy iterator # and not None. return + + +def generate_external_ids(external_id_value=None): + """Generate external-ids dictionary that can be used to mark OVS bridges + and ports as managed by the charm. + + :param external_id_value: Value of the external-ids entry. + Note: 'managed' will be used if not specified. + :type external_id_value: Optional[str] + :returns: Dict with a single external-ids entry. + { + 'external-ids': { + charm-``charm_name``: ``external_id_value`` + } + } + :rtype: Dict[str, Dict[str]] + """ + external_id_key = "charm-{}".format(charm_name()) + external_id_value = ('managed' if external_id_value is None + else external_id_value) + return { + 'external-ids': { + external_id_key: external_id_value + } + } diff --git a/hooks/charmhelpers/contrib/openstack/amulet/utils.py b/hooks/charmhelpers/contrib/openstack/amulet/utils.py index 63aea1e3..0a14af7e 100644 --- a/hooks/charmhelpers/contrib/openstack/amulet/utils.py +++ b/hooks/charmhelpers/contrib/openstack/amulet/utils.py @@ -42,6 +42,7 @@ import pika import swiftclient from charmhelpers.core.decorators import retry_on_exception + from charmhelpers.contrib.amulet.utils import ( AmuletUtils ) diff --git a/hooks/charmhelpers/contrib/openstack/cert_utils.py b/hooks/charmhelpers/contrib/openstack/cert_utils.py index 24867497..703fc6ef 100644 --- a/hooks/charmhelpers/contrib/openstack/cert_utils.py +++ b/hooks/charmhelpers/contrib/openstack/cert_utils.py @@ -47,7 +47,7 @@ from charmhelpers.contrib.network.ip import ( ) from charmhelpers.core.host import ( - CA_CERT_DIR, + ca_cert_absolute_path, install_ca_cert, mkdir, write_file, @@ -307,6 +307,26 @@ def install_certs(ssl_dir, certs, chain=None, user='root', group='root'): content=bundle['key'], perms=0o640) +def get_cert_relation_ca_name(cert_relation_id=None): + """Determine CA certificate name as provided by relation. + + The filename on disk depends on the name chosen for the application on the + providing end of the certificates relation. + + :param cert_relation_id: (Optional) Relation id providing the certs + :type cert_relation_id: str + :returns: CA certificate filename without path nor extension + :rtype: str + """ + if cert_relation_id is None: + try: + cert_relation_id = relation_ids('certificates')[0] + except IndexError: + return '' + return '{}_juju_ca_cert'.format( + remote_service_name(relid=cert_relation_id)) + + def _manage_ca_certs(ca, cert_relation_id): """Manage CA certs. @@ -316,7 +336,7 @@ def _manage_ca_certs(ca, cert_relation_id): :type cert_relation_id: str """ config_ssl_ca = config('ssl_ca') - config_cert_file = '{}/{}.crt'.format(CA_CERT_DIR, CONFIG_CA_CERT_FILE) + config_cert_file = ca_cert_absolute_path(CONFIG_CA_CERT_FILE) if config_ssl_ca: log("Installing CA certificate from charm ssl_ca config to {}".format( config_cert_file), INFO) @@ -329,8 +349,7 @@ def _manage_ca_certs(ca, cert_relation_id): log("Installing CA certificate from certificate relation", INFO) install_ca_cert( ca.encode(), - name='{}_juju_ca_cert'.format( - remote_service_name(relid=cert_relation_id))) + name=get_cert_relation_ca_name(cert_relation_id)) def process_certificates(service_name, relation_id, unit, diff --git a/hooks/charmhelpers/contrib/openstack/context.py b/hooks/charmhelpers/contrib/openstack/context.py index c242d18d..b67dafda 100644 --- a/hooks/charmhelpers/contrib/openstack/context.py +++ b/hooks/charmhelpers/contrib/openstack/context.py @@ -74,7 +74,6 @@ from charmhelpers.core.host import ( pwgen, lsb_release, CompareHostReleases, - is_container, ) from charmhelpers.contrib.hahelpers.cluster import ( determine_apache_port, @@ -1596,16 +1595,21 @@ def _calculate_workers(): @returns int: number of worker processes to use ''' - multiplier = config('worker-multiplier') or DEFAULT_MULTIPLIER + multiplier = config('worker-multiplier') + + # distinguish an empty config and an explicit config as 0.0 + if multiplier is None: + multiplier = DEFAULT_MULTIPLIER + count = int(_num_cpus() * multiplier) - if multiplier > 0 and count == 0: + if count <= 0: + # assign at least one worker count = 1 - if config('worker-multiplier') is None and is_container(): + if config('worker-multiplier') is None: # NOTE(jamespage): Limit unconfigured worker-multiplier # to MAX_DEFAULT_WORKERS to avoid insane - # worker configuration in LXD containers - # on large servers + # worker configuration on large servers # Reference: https://pad.lv/1665270 count = min(count, MAX_DEFAULT_WORKERS) diff --git a/hooks/charmhelpers/contrib/openstack/utils.py b/hooks/charmhelpers/contrib/openstack/utils.py index f27aa6c9..d0bef935 100644 --- a/hooks/charmhelpers/contrib/openstack/utils.py +++ b/hooks/charmhelpers/contrib/openstack/utils.py @@ -483,9 +483,26 @@ def get_swift_codename(version): return None -@deprecate("moved to charmhelpers.contrib.openstack.utils.get_installed_os_version()", "2021-01", log=juju_log) def get_os_codename_package(package, fatal=True): - '''Derive OpenStack release codename from an installed package.''' + """Derive OpenStack release codename from an installed package. + + Initially, see if the openstack-release pkg is available (by trying to + install it) and use it instead. + + If it isn't then it falls back to the existing method of checking the + version of the package passed and then resolving the version from that + using lookup tables. + + Note: if possible, charms should use get_installed_os_version() to + determine the version of the "openstack-release" pkg. + + :param package: the package to test for version information. + :type package: str + :param fatal: If True (default), then die via error_out() + :type fatal: bool + :returns: the OpenStack release codename (e.g. ussuri) + :rtype: str + """ codename = get_installed_os_version() if codename: @@ -579,8 +596,22 @@ def get_os_version_package(pkg, fatal=True): def get_installed_os_version(): - apt_install(filter_installed_packages(['openstack-release']), fatal=False) - print("OpenStack Release: {}".format(openstack_release())) + """Determine the OpenStack release code name from openstack-release pkg. + + This uses the "openstack-release" pkg (if it exists) to return the + OpenStack release codename (e.g. usurri, mitaka, ocata, etc.) + + Note, it caches the result so that it is only done once per hook. + + :returns: the OpenStack release codename, if available + :rtype: Optional[str] + """ + @cached + def _do_install(): + apt_install(filter_installed_packages(['openstack-release']), + fatal=False, quiet=True) + + _do_install() return openstack_release().get('OPENSTACK_CODENAME') @@ -1717,7 +1748,10 @@ def make_assess_status_func(*args, **kwargs): def pausable_restart_on_change(restart_map, stopstart=False, - restart_functions=None): + restart_functions=None, + can_restart_now_f=None, + post_svc_restart_f=None, + pre_restarts_wait_f=None): """A restart_on_change decorator that checks to see if the unit is paused. If it is paused then the decorated function doesn't fire. @@ -1743,11 +1777,28 @@ def pausable_restart_on_change(restart_map, stopstart=False, function won't be called if the decorated function is never called. Note, retains backwards compatibility for passing a non-callable dictionary. - @param f: the function to decorate - @param restart_map: (optionally callable, which then returns the - restart_map) the restart map {conf_file: [services]} - @param stopstart: DEFAULT false; whether to stop, start or just restart - @returns decorator to use a restart_on_change with pausability + :param f: function to decorate. + :type f: Callable + :param restart_map: Optionally callable, which then returns the restart_map or + the restart map {conf_file: [services]} + :type restart_map: Union[Callable[[],], Dict[str, List[str,]] + :param stopstart: whether to stop, start or restart a service + :type stopstart: booleean + :param restart_functions: nonstandard functions to use to restart services + {svc: func, ...} + :type restart_functions: Dict[str, Callable[[str], None]] + :param can_restart_now_f: A function used to check if the restart is + permitted. + :type can_restart_now_f: Callable[[str, List[str]], boolean] + :param post_svc_restart_f: A function run after a service has + restarted. + :type post_svc_restart_f: Callable[[str], None] + :param pre_restarts_wait_f: A function callled before any restarts. + :type pre_restarts_wait_f: Callable[None, None] + :returns: decorator to use a restart_on_change with pausability + :rtype: decorator + + """ def wrap(f): # py27 compatible nonlocal variable. When py3 only, replace with @@ -1763,8 +1814,13 @@ def pausable_restart_on_change(restart_map, stopstart=False, if callable(restart_map) else restart_map # otherwise, normal restart_on_change functionality return restart_on_change_helper( - (lambda: f(*args, **kwargs)), __restart_map_cache['cache'], - stopstart, restart_functions) + (lambda: f(*args, **kwargs)), + __restart_map_cache['cache'], + stopstart, + restart_functions, + can_restart_now_f, + post_svc_restart_f, + pre_restarts_wait_f) return wrapped_f return wrap @@ -2418,3 +2474,26 @@ def get_api_application_status(): msg = 'Some units are not ready' juju_log(msg, 'DEBUG') return app_state, msg + + +def sequence_status_check_functions(*functions): + """Sequence the functions passed so that they all get a chance to run as + the charm status check functions. + + :param *functions: a list of functions that return (state, message) + :type *functions: List[Callable[[OSConfigRender], (str, str)]] + :returns: the Callable that takes configs and returns (state, message) + :rtype: Callable[[OSConfigRender], (str, str)] + """ + def _inner_sequenced_functions(configs): + state, message = 'unknown', '' + for f in functions: + new_state, new_message = f(configs) + state = workload_state_compare(state, new_state) + if message: + message = "{}, {}".format(message, new_message) + else: + message = new_message + return state, message + + return _inner_sequenced_functions diff --git a/hooks/charmhelpers/core/hookenv.py b/hooks/charmhelpers/core/hookenv.py index db7ce728..f6fa7fce 100644 --- a/hooks/charmhelpers/core/hookenv.py +++ b/hooks/charmhelpers/core/hookenv.py @@ -226,6 +226,17 @@ def relation_id(relation_name=None, service_or_unit=None): raise ValueError('Must specify neither or both of relation_name and service_or_unit') +def departing_unit(): + """The departing unit for the current relation hook. + + Available since juju 2.8. + + :returns: the departing unit, or None if the information isn't available. + :rtype: Optional[str] + """ + return os.environ.get('JUJU_DEPARTING_UNIT', None) + + def local_unit(): """Local unit ID""" return os.environ['JUJU_UNIT_NAME'] diff --git a/hooks/charmhelpers/core/host.py b/hooks/charmhelpers/core/host.py index f826f6fe..22e2e951 100644 --- a/hooks/charmhelpers/core/host.py +++ b/hooks/charmhelpers/core/host.py @@ -34,7 +34,7 @@ import itertools import six from contextlib import contextmanager -from collections import OrderedDict +from collections import OrderedDict, defaultdict from .hookenv import log, INFO, DEBUG, local_unit, charm_name from .fstab import Fstab from charmhelpers.osplatform import get_platform @@ -730,37 +730,84 @@ def restart_on_change(restart_map, stopstart=False, restart_functions=None): def restart_on_change_helper(lambda_f, restart_map, stopstart=False, - restart_functions=None): + restart_functions=None, + can_restart_now_f=None, + post_svc_restart_f=None, + pre_restarts_wait_f=None): """Helper function to perform the restart_on_change function. This is provided for decorators to restart services if files described in the restart_map have changed after an invocation of lambda_f(). - @param lambda_f: function to call. - @param restart_map: {file: [service, ...]} - @param stopstart: whether to stop, start or restart a service - @param restart_functions: nonstandard functions to use to restart services + This functions allows for a number of helper functions to be passed. + + `restart_functions` is a map with a service as the key and the + corresponding value being the function to call to restart the service. For + example if `restart_functions={'some-service': my_restart_func}` then + `my_restart_func` should a function which takes one argument which is the + service name to be retstarted. + + `can_restart_now_f` is a function which checks that a restart is permitted. It + should returna bool which indicates if a restart is allowed and should + take a service name (str) and a list of changed files (List[str]) as + arguments. + + `post_svc_restart_f` is a function which runs after a service has been + restarted. It takes the service name that was restarted as an argument. + + `pre_restarts_wait_f` is a function which is called before any restarts + occur. The use case for this is an application which wants to try and + stagger restarts between units. + + :param lambda_f: function to call. + :type lambda_f: Callable[[], ANY] + :param restart_map: {file: [service, ...]} + :type restart_map: Dict[str, List[str,]] + :param stopstart: whether to stop, start or restart a service + :type stopstart: booleean + :param restart_functions: nonstandard functions to use to restart services {svc: func, ...} - @returns result of lambda_f() + :type restart_functions: Dict[str, Callable[[str], None]] + :param can_restart_now_f: A function used to check if the restart is + permitted. + :type can_restart_now_f: Callable[[str, List[str]], boolean] + :param post_svc_restart_f: A function run after a service has + restarted. + :type post_svc_restart_f: Callable[[str], None] + :param pre_restarts_wait_f: A function callled before any restarts. + :type pre_restarts_wait_f: Callable[None, None] + :returns: result of lambda_f() + :rtype: ANY """ if restart_functions is None: restart_functions = {} checksums = {path: path_hash(path) for path in restart_map} r = lambda_f() + changed_files = defaultdict(list) + restarts = [] # create a list of lists of the services to restart - restarts = [restart_map[path] - for path in restart_map - if path_hash(path) != checksums[path]] + for path, services in restart_map.items(): + if path_hash(path) != checksums[path]: + restarts.append(services) + for svc in services: + changed_files[svc].append(path) # create a flat list of ordered services without duplicates from lists services_list = list(OrderedDict.fromkeys(itertools.chain(*restarts))) if services_list: + if pre_restarts_wait_f: + pre_restarts_wait_f() actions = ('stop', 'start') if stopstart else ('restart',) for service_name in services_list: + if can_restart_now_f: + if not can_restart_now_f(service_name, changed_files[service_name]): + continue if service_name in restart_functions: restart_functions[service_name](service_name) else: for action in actions: service(action, service_name) + if post_svc_restart_f: + post_svc_restart_f(service_name) return r @@ -1068,6 +1115,17 @@ def modulo_distribution(modulo=3, wait=30, non_zero_wait=False): return calculated_wait_time +def ca_cert_absolute_path(basename_without_extension): + """Returns absolute path to CA certificate. + + :param basename_without_extension: Filename without extension + :type basename_without_extension: str + :returns: Absolute full path + :rtype: str + """ + return '{}/{}.crt'.format(CA_CERT_DIR, basename_without_extension) + + def install_ca_cert(ca_cert, name=None): """ Install the given cert as a trusted CA. @@ -1083,7 +1141,7 @@ def install_ca_cert(ca_cert, name=None): ca_cert = ca_cert.encode('utf8') if not name: name = 'juju-{}'.format(charm_name()) - cert_file = '{}/{}.crt'.format(CA_CERT_DIR, name) + cert_file = ca_cert_absolute_path(name) new_hash = hashlib.md5(ca_cert).hexdigest() if file_hash(cert_file) == new_hash: return diff --git a/hooks/charmhelpers/fetch/ubuntu.py b/hooks/charmhelpers/fetch/ubuntu.py index b5953019..1de9cd52 100644 --- a/hooks/charmhelpers/fetch/ubuntu.py +++ b/hooks/charmhelpers/fetch/ubuntu.py @@ -13,6 +13,7 @@ # limitations under the License. from collections import OrderedDict +import os import platform import re import six @@ -20,6 +21,7 @@ import subprocess import sys import time +from charmhelpers import deprecate from charmhelpers.core.host import get_distrib_codename, get_system_env from charmhelpers.core.hookenv import ( @@ -251,13 +253,19 @@ def apt_cache(*_, **__): # Detect this situation, log a warning and make the call to # ``apt_pkg.init()`` to avoid the consumer Python interpreter from # crashing with a segmentation fault. - log('Support for use of upstream ``apt_pkg`` module in conjunction' - 'with charm-helpers is deprecated since 2019-06-25', level=WARNING) + @deprecate( + 'Support for use of upstream ``apt_pkg`` module in conjunction' + 'with charm-helpers is deprecated since 2019-06-25', + date=None, log=lambda x: log(x, level=WARNING)) + def one_shot_log(): + pass + + one_shot_log() sys.modules['apt_pkg'].init() return ubuntu_apt_pkg.Cache() -def apt_install(packages, options=None, fatal=False): +def apt_install(packages, options=None, fatal=False, quiet=False): """Install one or more packages. :param packages: Package(s) to install @@ -267,6 +275,8 @@ def apt_install(packages, options=None, fatal=False): :param fatal: Whether the command's output should be checked and retried. :type fatal: bool + :param quiet: if True (default), supress log message to stdout/stderr + :type quiet: bool :raises: subprocess.CalledProcessError """ if options is None: @@ -279,9 +289,10 @@ def apt_install(packages, options=None, fatal=False): cmd.append(packages) else: cmd.extend(packages) - log("Installing {} with options: {}".format(packages, - options)) - _run_apt_command(cmd, fatal) + if not quiet: + log("Installing {} with options: {}" + .format(packages, options)) + _run_apt_command(cmd, fatal, quiet=quiet) def apt_upgrade(options=None, fatal=False, dist=False): @@ -639,14 +650,17 @@ def _add_apt_repository(spec): :param spec: the parameter to pass to add_apt_repository :type spec: str """ + series = get_distrib_codename() if '{series}' in spec: - series = get_distrib_codename() spec = spec.replace('{series}', series) # software-properties package for bionic properly reacts to proxy settings - # passed as environment variables (See lp:1433761). This is not the case - # LTS and non-LTS releases below bionic. - _run_with_retries(['add-apt-repository', '--yes', spec], - cmd_env=env_proxy_settings(['https', 'http'])) + # set via apt.conf (see lp:1433761), however this is not the case for LTS + # and non-LTS releases before bionic. + if series in ('trusty', 'xenial'): + _run_with_retries(['add-apt-repository', '--yes', spec], + cmd_env=env_proxy_settings(['https', 'http'])) + else: + _run_with_retries(['add-apt-repository', '--yes', spec]) def _add_cloud_pocket(pocket): @@ -723,7 +737,7 @@ def _verify_is_ubuntu_rel(release, os_release): def _run_with_retries(cmd, max_retries=CMD_RETRY_COUNT, retry_exitcodes=(1,), - retry_message="", cmd_env=None): + retry_message="", cmd_env=None, quiet=False): """Run a command and retry until success or max_retries is reached. :param cmd: The apt command to run. @@ -738,11 +752,20 @@ def _run_with_retries(cmd, max_retries=CMD_RETRY_COUNT, retry_exitcodes=(1,), :type retry_message: str :param: cmd_env: Environment variables to add to the command run. :type cmd_env: Option[None, Dict[str, str]] + :param quiet: if True, silence the output of the command from stdout and + stderr + :type quiet: bool """ env = get_apt_dpkg_env() if cmd_env: env.update(cmd_env) + kwargs = {} + if quiet: + devnull = os.devnull if six.PY2 else subprocess.DEVNULL + kwargs['stdout'] = devnull + kwargs['stderr'] = devnull + if not retry_message: retry_message = "Failed executing '{}'".format(" ".join(cmd)) retry_message += ". Will retry in {} seconds".format(CMD_RETRY_DELAY) @@ -753,7 +776,7 @@ def _run_with_retries(cmd, max_retries=CMD_RETRY_COUNT, retry_exitcodes=(1,), retry_results = (None,) + retry_exitcodes while result in retry_results: try: - result = subprocess.check_call(cmd, env=env) + result = subprocess.check_call(cmd, env=env, **kwargs) except subprocess.CalledProcessError as e: retry_count = retry_count + 1 if retry_count > max_retries: @@ -763,7 +786,7 @@ def _run_with_retries(cmd, max_retries=CMD_RETRY_COUNT, retry_exitcodes=(1,), time.sleep(CMD_RETRY_DELAY) -def _run_apt_command(cmd, fatal=False): +def _run_apt_command(cmd, fatal=False, quiet=False): """Run an apt command with optional retries. :param cmd: The apt command to run. @@ -771,13 +794,22 @@ def _run_apt_command(cmd, fatal=False): :param fatal: Whether the command's output should be checked and retried. :type fatal: bool + :param quiet: if True, silence the output of the command from stdout and + stderr + :type quiet: bool """ if fatal: _run_with_retries( cmd, retry_exitcodes=(1, APT_NO_LOCK,), - retry_message="Couldn't acquire DPKG lock") + retry_message="Couldn't acquire DPKG lock", + quiet=quiet) else: - subprocess.call(cmd, env=get_apt_dpkg_env()) + kwargs = {} + if quiet: + devnull = os.devnull if six.PY2 else subprocess.DEVNULL + kwargs['stdout'] = devnull + kwargs['stderr'] = devnull + subprocess.call(cmd, env=get_apt_dpkg_env(), **kwargs) def get_upstream_version(package): diff --git a/hooks/neutron_utils.py b/hooks/neutron_utils.py index 9d6a1aab..b426de4f 100644 --- a/hooks/neutron_utils.py +++ b/hooks/neutron_utils.py @@ -41,6 +41,7 @@ from charmhelpers.contrib.network.ovs import ( full_restart, get_bridges_and_ports_map, is_linuxbridge_interface, + generate_external_ids, ) from charmhelpers.contrib.hahelpers.cluster import ( get_hacluster_config, @@ -55,7 +56,7 @@ from charmhelpers.contrib.openstack.utils import ( pause_unit, reset_os_release, resume_unit, - workload_state_compare, + sequence_status_check_functions, ) from charmhelpers.contrib.openstack.neutron import ( @@ -867,8 +868,8 @@ def configure_ovs(): .format(", ".join("{}: {}".format(b, ",".join(v)) for b, v in current_bridges_and_ports.items()))) - add_bridge(INT_BRIDGE, brdata=_ovs_additional_data()) - add_bridge(EXT_BRIDGE, brdata=_ovs_additional_data()) + add_bridge(INT_BRIDGE, brdata=generate_external_ids()) + add_bridge(EXT_BRIDGE, brdata=generate_external_ids()) ext_port_ctx = ExternalPortContext()() portmaps = DataPortContext()() @@ -884,15 +885,15 @@ def configure_ovs(): if not portmaps and ext_port_ctx and ext_port_ctx['ext_port']: _port = ext_port_ctx['ext_port'] add_bridge_port(EXT_BRIDGE, _port, - ifdata=_ovs_additional_data(EXT_BRIDGE), - portdata=_ovs_additional_data(EXT_BRIDGE)) + ifdata=generate_external_ids(EXT_BRIDGE), + portdata=generate_external_ids(EXT_BRIDGE)) log("DEPRECATION: using ext-port to set the port {} on the " "EXT_BRIDGE ({}) is deprecated. Please use data-port instead." .format(_port, EXT_BRIDGE), level=WARNING) for br in bridgemaps.values(): - add_bridge(br, brdata=_ovs_additional_data()) + add_bridge(br, brdata=generate_external_ids()) if not portmaps: continue @@ -900,14 +901,14 @@ def configure_ovs(): if _br == br: if not is_linuxbridge_interface(port): add_bridge_port(br, port, promisc=True, - ifdata=_ovs_additional_data(br), - portdata=_ovs_additional_data(br)) + ifdata=generate_external_ids(br), + portdata=generate_external_ids(br)) else: # NOTE(lourot): this will raise on focal+ and/or if the # system has no `ifup`. See lp:1877594 add_ovsbridge_linuxbridge( - br, port, ifdata=_ovs_additional_data(br), - portdata=_ovs_additional_data(br)) + br, port, ifdata=generate_external_ids(br), + portdata=generate_external_ids(br)) target = config('ipfix-target') bridges = [INT_BRIDGE, EXT_BRIDGE] @@ -934,31 +935,6 @@ def configure_ovs(): service_restart('os-charm-phy-nic-mtu') -def _ovs_additional_data(external_id_value=None): - """Returns OVS additional data from the given external-id value. - - The returned value is in the input format expected by the - charmhelpers.contrib.network.ovs functions. - - We set an external-id as OVS additional data on all the bridges and ports - that we create in order to mark them as managed by us. See - http://docs.openvswitch.org/en/latest/topics/integration/ - - :param external_id_value: Value to be set as external ID. For a bridge, - typically 'managed' (which is also the default if nothing is passed). - For a port, typically the name of the corresponding bridge. - :type external_id_value: str - :rtype: Dict[str,Dict[str,str]] - """ - external_id_value = ('managed' if external_id_value is None - else external_id_value) - return { - 'external-ids': { - 'charm-neutron-gateway': external_id_value - } - } - - def copy_file(src, dst, perms=None, force=False): """Copy file to destination and optionally set permissionss. @@ -1158,29 +1134,6 @@ def check_ext_port_data_port_config(configs): return 'unknown', '' -def sequence_functions(*functions): - """Sequence the functions passed so that they all get a chance to run as - the check_charm_func for the assess_status. - - :param *functions: a list of functions that return (state, message) - :type *functions: List[Callable[[OSConfigRender], (str, str)]] - :returns: the Callable that takes configs and returns (state, message) - :rtype: Callable[[OSConfigRender], (str, str)] - """ - def _inner_sequenced_functions(configs): - state, message = 'unknown', '' - for f in functions: - new_state, new_message = f(configs) - state = workload_state_compare(state, new_state) - if message: - message = "{}, {}".format(message, new_message) - else: - message = new_message - return state, message - - return _inner_sequenced_functions - - def assess_status(configs): """Assess status of current unit Decides what the state of the unit should be based on the current @@ -1216,8 +1169,8 @@ def assess_status_func(configs): required_interfaces = REQUIRED_INTERFACES.copy() required_interfaces.update(get_optional_interfaces()) active_services = [s for s in services() if s not in STOPPED_SERVICES] - charm_func = sequence_functions(check_optional_relations, - check_ext_port_data_port_config) + charm_func = sequence_status_check_functions( + check_optional_relations, check_ext_port_data_port_config) return make_assess_status_func( configs, required_interfaces, charm_func=charm_func, diff --git a/unit_tests/test_neutron_utils.py b/unit_tests/test_neutron_utils.py index 92351a86..2d7b948f 100644 --- a/unit_tests/test_neutron_utils.py +++ b/unit_tests/test_neutron_utils.py @@ -275,8 +275,11 @@ class TestNeutronUtils(CharmTestCase): self.os_release.return_value = 'juno' self.assertTrue('keepalived' in neutron_utils.get_packages()) + @patch('charmhelpers.contrib.network.ovs.charm_name') @patch('charmhelpers.contrib.openstack.context.config') - def test_configure_ovs_starts_service_if_required(self, mock_config): + def test_configure_ovs_starts_service_if_required( + self, mock_config, charm_name): + charm_name.return_value = "neutron-gateway" mock_config.side_effect = self.test_config.get self.config.return_value = 'ovs' self.service_running.return_value = False @@ -288,8 +291,10 @@ class TestNeutronUtils(CharmTestCase): neutron_utils.configure_ovs() self.assertFalse(self.full_restart.called) + @patch('charmhelpers.contrib.network.ovs.charm_name') @patch('charmhelpers.contrib.openstack.context.config') - def test_configure_ovs_ovs_ext_port(self, mock_config): + def test_configure_ovs_ovs_ext_port(self, mock_config, charm_name): + charm_name.return_value = "neutron-gateway" mock_config.side_effect = self.test_config.get self.config.side_effect = self.test_config.get self.test_config.set('plugin', 'ovs') @@ -308,8 +313,10 @@ class TestNeutronUtils(CharmTestCase): @patch('charmhelpers.contrib.openstack.context.list_nics', return_value=['eth0', 'eth0.100', 'eth0.200']) + @patch('charmhelpers.contrib.network.ovs.charm_name') @patch('charmhelpers.contrib.openstack.context.config') - def test_configure_ovs_ovs_data_port(self, mock_config, _nics): + def test_configure_ovs_ovs_data_port(self, mock_config, charm_name, _nics): + charm_name.return_value = "neutron-gateway" self.is_linuxbridge_interface.return_value = False mock_config.side_effect = self.test_config.get self.config.side_effect = self.test_config.get @@ -360,8 +367,11 @@ class TestNeutronUtils(CharmTestCase): @patch('charmhelpers.contrib.openstack.context.list_nics', return_value=['br-eth0']) + @patch('charmhelpers.contrib.network.ovs.charm_name') @patch('charmhelpers.contrib.openstack.context.config') - def test_configure_ovs_ovs_data_port_bridge(self, mock_config, _nics): + def test_configure_ovs_ovs_data_port_bridge( + self, mock_config, charm_name, _nics): + charm_name.return_value = "neutron-gateway" self.is_linuxbridge_interface.return_value = True mock_config.side_effect = self.test_config.get self.config.side_effect = self.test_config.get @@ -391,8 +401,10 @@ class TestNeutronUtils(CharmTestCase): portdata=expected_portdata)] self.add_ovsbridge_linuxbridge.assert_has_calls(calls) + @patch('charmhelpers.contrib.network.ovs.charm_name') @patch('charmhelpers.contrib.openstack.context.config') - def test_configure_ovs_enable_ipfix(self, mock_config): + def test_configure_ovs_enable_ipfix(self, mock_config, charm_name): + charm_name.return_value = "neutron-gateway" mock_config.side_effect = self.test_config.get self.config.side_effect = self.test_config.get self.test_config.set('plugin', 'ovs') @@ -405,9 +417,11 @@ class TestNeutronUtils(CharmTestCase): ]) @patch.object(neutron_utils, 'DataPortContext') + @patch('charmhelpers.contrib.network.ovs.charm_name') @patch('charmhelpers.contrib.openstack.context.config') def test_configure_ovs_ensure_ext_port_overriden( - self, mock_config, mock_data_port_context): + self, mock_config, charm_name, mock_data_port_context): + charm_name.return_value = "neutron-gateway" mock_config.side_effect = self.test_config.get self.config.side_effect = self.test_config.get self.test_config.set('plugin', 'ovs') @@ -1090,7 +1104,7 @@ class TestNeutronAgentReallocation(CharmTestCase): ) @patch.object(neutron_utils, 'get_optional_interfaces') - @patch.object(neutron_utils, 'sequence_functions') + @patch.object(neutron_utils, 'sequence_status_check_functions') @patch.object(neutron_utils, 'REQUIRED_INTERFACES') @patch.object(neutron_utils, 'services') @patch.object(neutron_utils, 'make_assess_status_func')