From e9fc1de43be90a20eb8134fa1bc7901a97e1eba9 Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Fri, 19 Mar 2021 17:47:25 +0100 Subject: [PATCH] Process subordinate releases packages map For principal - subordinate plugin type relations where the principal Python payload imports code from packages managed by a subordinate, upgrades can be problematic. This change will allow a subordinate charm that have opted into the feature to inform its principal about all implemented release - packages combinations ahead of time. With this information in place the principal can do the upgrade in one operation without risk of charm relation RPC type processing at a critical moment. Also sync c-h. Closes-Bug: #1806111 Change-Id: I95567d5d047eb64842436e671b74a633e6f509f4 --- bindep.txt | 4 + charmhelpers/contrib/charmsupport/nrpe.py | 4 +- .../contrib/openstack/amulet/utils.py | 1 + charmhelpers/contrib/openstack/cert_utils.py | 27 +- charmhelpers/contrib/openstack/context.py | 16 +- charmhelpers/contrib/openstack/utils.py | 157 +++++++++++- charmhelpers/core/hookenv.py | 20 ++ charmhelpers/core/host.py | 236 +++++++++++++++--- charmhelpers/fetch/ubuntu.py | 64 +++-- hooks/keystone_utils.py | 24 +- unit_tests/test_keystone_hooks.py | 57 +++-- unit_tests/test_keystone_utils.py | 70 +++++- 12 files changed, 569 insertions(+), 111 deletions(-) create mode 100644 bindep.txt diff --git a/bindep.txt b/bindep.txt new file mode 100644 index 00000000..ba2ccb4b --- /dev/null +++ b/bindep.txt @@ -0,0 +1,4 @@ +libxml2-dev [platform:dpkg test] +libxslt1-dev [platform:dpkg test] +build-essential [platform:dpkg test] +zlib1g-dev [platform:dpkg test] diff --git a/charmhelpers/contrib/charmsupport/nrpe.py b/charmhelpers/contrib/charmsupport/nrpe.py index c87cf489..e4cb06bc 100644 --- a/charmhelpers/contrib/charmsupport/nrpe.py +++ b/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/charmhelpers/contrib/openstack/amulet/utils.py b/charmhelpers/contrib/openstack/amulet/utils.py index 63aea1e3..0a14af7e 100644 --- a/charmhelpers/contrib/openstack/amulet/utils.py +++ b/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/charmhelpers/contrib/openstack/cert_utils.py b/charmhelpers/contrib/openstack/cert_utils.py index 24867497..703fc6ef 100644 --- a/charmhelpers/contrib/openstack/cert_utils.py +++ b/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/charmhelpers/contrib/openstack/context.py b/charmhelpers/contrib/openstack/context.py index c242d18d..b67dafda 100644 --- a/charmhelpers/contrib/openstack/context.py +++ b/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/charmhelpers/contrib/openstack/utils.py b/charmhelpers/contrib/openstack/utils.py index f27aa6c9..5d93f790 100644 --- a/charmhelpers/contrib/openstack/utils.py +++ b/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 @@ -2145,6 +2201,23 @@ def container_scoped_relations(): return relations +def container_scoped_relation_get(attribute=None): + """Get relation data from all container scoped relations. + + :param attribute: Name of attribute to get + :type attribute: Optional[str] + :returns: Iterator with relation data + :rtype: Iterator[Optional[any]] + """ + for endpoint_name in container_scoped_relations(): + for rid in relation_ids(endpoint_name): + for unit in related_units(rid): + yield relation_get( + attribute=attribute, + unit=unit, + rid=rid) + + def is_db_ready(use_current_context=False, rel_name=None): """Check remote database is ready to be used. @@ -2418,3 +2491,63 @@ 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 + + +SubordinatePackages = namedtuple('SubordinatePackages', ['install', 'purge']) + + +def get_subordinate_release_packages(os_release, package_type='deb'): + """Iterate over subordinate relations and get package information. + + :param os_release: OpenStack release to look for + :type os_release: str + :param package_type: Package type (one of 'deb' or 'snap') + :type package_type: str + :returns: Packages to install and packages to purge or None + :rtype: SubordinatePackages[set,set] + """ + install = set() + purge = set() + + for rdata in container_scoped_relation_get('releases-packages-map'): + rp_map = json.loads(rdata or '{}') + # The map provided by subordinate has OpenStack release name as key. + # Find package information from subordinate matching requested release + # or the most recent release prior to requested release by sorting the + # keys in reverse order. This follows established patterns in our + # charms for templates and reactive charm implementations, i.e. as long + # as nothing has changed the definitions for the prior OpenStack + # release is still valid. + for release in sorted(rp_map.keys(), reverse=True): + if (CompareOpenStackReleases(release) <= os_release and + package_type in rp_map[release]): + for name, container in ( + ('install', install), + ('purge', purge)): + for pkg in rp_map[release][package_type].get(name, []): + container.add(pkg) + break + return SubordinatePackages(install, purge) diff --git a/charmhelpers/core/hookenv.py b/charmhelpers/core/hookenv.py index db7ce728..778aa4b6 100644 --- a/charmhelpers/core/hookenv.py +++ b/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'] @@ -1611,3 +1622,12 @@ def _contains_range(addresses): addresses.startswith(".") or ",." in addresses or " ." in addresses) + + +def is_subordinate(): + """Check whether charm is subordinate in unit metadata. + + :returns: True if unit is subordniate, False otherwise. + :rtype: bool + """ + return metadata().get('subordinate') is True diff --git a/charmhelpers/core/host.py b/charmhelpers/core/host.py index f826f6fe..d25e6c59 100644 --- a/charmhelpers/core/host.py +++ b/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 @@ -694,74 +694,223 @@ class ChecksumError(ValueError): pass -def restart_on_change(restart_map, stopstart=False, restart_functions=None): - """Restart services based on configuration files changing +class restart_on_change(object): + """Decorator and context manager to handle restarts. - This function is used a decorator, for example:: + Usage: - @restart_on_change({ - '/etc/ceph/ceph.conf': [ 'cinder-api', 'cinder-volume' ] - '/etc/apache/sites-enabled/*': [ 'apache2' ] - }) - def config_changed(): - pass # your code here + @restart_on_change(restart_map, ...) + def function_that_might_trigger_a_restart(...) + ... - In this example, the cinder-api and cinder-volume services - would be restarted if /etc/ceph/ceph.conf is changed by the - ceph_client_changed function. The apache2 service would be - restarted if any file matching the pattern got changed, created - or removed. Standard wildcards are supported, see documentation - for the 'glob' module for more information. + Or: - @param restart_map: {path_file_name: [service_name, ...] - @param stopstart: DEFAULT false; whether to stop, start OR restart - @param restart_functions: nonstandard functions to use to restart services - {svc: func, ...} - @returns result from decorated function + with restart_on_change(restart_map, ...): + do_stuff_that_might_trigger_a_restart() + ... """ - def wrap(f): + + def __init__(self, restart_map, stopstart=False, restart_functions=None, + can_restart_now_f=None, post_svc_restart_f=None, + pre_restarts_wait_f=None): + """ + :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, ...} + :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] + """ + self.restart_map = restart_map + self.stopstart = stopstart + self.restart_functions = restart_functions + self.can_restart_now_f = can_restart_now_f + self.post_svc_restart_f = post_svc_restart_f + self.pre_restarts_wait_f = pre_restarts_wait_f + + def __call__(self, f): + """Work like a decorator. + + Returns a wrapped function that performs the restart if triggered. + + :param f: The function that is being wrapped. + :type f: Callable[[Any], Any] + :returns: the wrapped function + :rtype: Callable[[Any], Any] + """ @functools.wraps(f) def wrapped_f(*args, **kwargs): return restart_on_change_helper( - (lambda: f(*args, **kwargs)), restart_map, stopstart, - restart_functions) + (lambda: f(*args, **kwargs)), + self.restart_map, + stopstart=self.stopstart, + restart_functions=self.restart_functions, + can_restart_now_f=self.can_restart_now_f, + post_svc_restart_f=self.post_svc_restart_f, + pre_restarts_wait_f=self.pre_restarts_wait_f) return wrapped_f - return wrap + + def __enter__(self): + """Enter the runtime context related to this object. """ + self.checksums = _pre_restart_on_change_helper(self.restart_map) + + def __exit__(self, exc_type, exc_val, exc_tb): + """Exit the runtime context related to this object. + + The parameters describe the exception that caused the context to be + exited. If the context was exited without an exception, all three + arguments will be None. + """ + if exc_type is None: + _post_restart_on_change_helper( + self.checksums, + self.restart_map, + stopstart=self.stopstart, + restart_functions=self.restart_functions, + can_restart_now_f=self.can_restart_now_f, + post_svc_restart_f=self.post_svc_restart_f, + pre_restarts_wait_f=self.pre_restarts_wait_f) + # All is good, so return False; any exceptions will propagate. + return False 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 return a 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 + """ + checksums = _pre_restart_on_change_helper(restart_map) + r = lambda_f() + _post_restart_on_change_helper(checksums, + restart_map, + stopstart, + restart_functions, + can_restart_now_f, + post_svc_restart_f, + pre_restarts_wait_f) + return r + + +def _pre_restart_on_change_helper(restart_map): + """Take a snapshot of file hashes. + + :param restart_map: {file: [service, ...]} + :type restart_map: Dict[str, List[str,]] + :returns: Dictionary of file paths and the files checksum. + :rtype: Dict[str, str] + """ + return {path: path_hash(path) for path in restart_map} + + +def _post_restart_on_change_helper(checksums, + restart_map, + stopstart=False, + restart_functions=None, + can_restart_now_f=None, + post_svc_restart_f=None, + pre_restarts_wait_f=None): + """Check whether files have changed. + + :param checksums: Dictionary of file paths and the files checksum. + :type checksums: Dict[str, str] + :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, ...} + :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] """ 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) - return r + if post_svc_restart_f: + post_svc_restart_f(service_name) def pwgen(length=None): @@ -1068,6 +1217,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 +1243,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/charmhelpers/fetch/ubuntu.py b/charmhelpers/fetch/ubuntu.py index b5953019..1de9cd52 100644 --- a/charmhelpers/fetch/ubuntu.py +++ b/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/keystone_utils.py b/hooks/keystone_utils.py index 6e58e8d4..1b704b60 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -58,6 +58,7 @@ from charmhelpers.contrib.openstack.utils import ( get_api_application_status, get_os_codename_install_source, get_snaps_install_info_from_origin, + get_subordinate_release_packages, install_os_snaps, is_unit_paused_set, make_assess_status_func, @@ -669,21 +670,26 @@ def api_port(service): def determine_packages(): - release = CompareOpenStackReleases(os_release('keystone')) + release = os_release('keystone') + cmp_release = CompareOpenStackReleases(release) # currently all packages match service names if snap_install_requested(): pkgs = deepcopy(BASE_PACKAGES_SNAP) - if enable_memcache(release=os_release('keystone')): + if enable_memcache(release=release): pkgs = pkgs + ['memcached'] + pkgs = set(pkgs).union(get_subordinate_release_packages( + release, package_type='snap').install) return sorted(pkgs) else: packages = set(services()).union(BASE_PACKAGES) - if release >= 'rocky': + if cmp_release >= 'rocky': packages = [p for p in packages if not p.startswith('python-')] packages.extend(PY3_PACKAGES) elif run_in_apache(): packages.add('libapache2-mod-wsgi') + packages = set(packages).union(get_subordinate_release_packages( + release).install) return sorted(packages) @@ -694,12 +700,16 @@ def determine_purge_packages(): :returns: list of package names ''' - release = CompareOpenStackReleases(os_release('keystone')) - if release >= 'rocky': + release = os_release('keystone') + cmp_release = CompareOpenStackReleases(release) + pkgs = [] + if cmp_release >= 'rocky': pkgs = [p for p in BASE_PACKAGES if p.startswith('python-')] pkgs.extend(['python-keystone', 'python-memcache']) - return pkgs - return [] + pkgs = set(pkgs).union( + get_subordinate_release_packages( + release).purge) + return sorted(pkgs) def remove_old_packages(): diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index 8c973edc..1f38aab7 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/unit_tests/test_keystone_hooks.py @@ -16,6 +16,8 @@ import importlib import os import sys +import charmhelpers.contrib.openstack.utils as os_utils + from mock import call, patch, MagicMock, ANY from test_utils import CharmTestCase @@ -117,17 +119,21 @@ class KeystoneRelationTests(CharmTestCase): self.ssh_user = 'juju_keystone' self.snap_install_requested.return_value = False + @patch.object(utils, 'get_subordinate_release_packages') @patch.object(hooks, 'maybe_do_policyd_overrides') @patch.object(utils, 'os_release') @patch.object(hooks, 'service_stop', lambda *args: None) @patch.object(hooks, 'service_start', lambda *args: None) def test_install_hook(self, os_release, - mock_maybe_do_policyd_overrides): + mock_maybe_do_policyd_overrides, + mock_get_subordinate_release_packages): os_release.return_value = 'havana' self.run_in_apache.return_value = False repo = 'cloud:precise-grizzly' self.test_config.set('openstack-origin', repo) + mock_get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) hooks.install() self.assertTrue(self.execd_preinstall.called) self.configure_installation_source.assert_called_with(repo) @@ -140,17 +146,21 @@ class KeystoneRelationTests(CharmTestCase): mock_maybe_do_policyd_overrides.assert_called_once_with( ANY, "keystone", restart_handler=ANY) + @patch.object(utils, 'get_subordinate_release_packages') @patch.object(hooks, 'maybe_do_policyd_overrides') @patch.object(utils, 'os_release') @patch.object(hooks, 'service_stop', lambda *args: None) @patch.object(hooks, 'service_start', lambda *args: None) def test_install_hook_apache2(self, os_release, - mock_maybe_do_policyd_overrides): + mock_maybe_do_policyd_overrides, + mock_get_subordinate_release_packages): os_release.return_value = 'havana' self.run_in_apache.return_value = True repo = 'cloud:xenial-newton' self.test_config.set('openstack-origin', repo) + mock_get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) hooks.install() self.assertTrue(self.execd_preinstall.called) self.configure_installation_source.assert_called_with(repo) @@ -617,6 +627,7 @@ class KeystoneRelationTests(CharmTestCase): cmd = ['a2dissite', 'openstack_https_frontend'] self.check_call.assert_called_with(cmd) + @patch.object(utils, 'get_subordinate_release_packages') @patch.object(hooks, 'bootstrap_keystone') @patch.object(hooks, 'ensure_all_service_accounts_protected_for_pci_dss_options') @@ -638,7 +649,8 @@ class KeystoneRelationTests(CharmTestCase): update, mock_maybe_do_policyd_overrides, mock_protect_service_accounts, - mock_bootstrap_keystone): + mock_bootstrap_keystone, + mock_get_subordinate_release_packages): os_release.return_value = 'havana' mock_is_db_initialised.return_value = True mock_is_db_ready.return_value = True @@ -647,6 +659,8 @@ class KeystoneRelationTests(CharmTestCase): self.services.return_value = ['apache2'] self.filter_installed_packages.return_value = ['something'] + mock_get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) hooks.upgrade_charm() self.assertTrue(self.apt_install.called) self.assertTrue(update.called) @@ -658,6 +672,7 @@ class KeystoneRelationTests(CharmTestCase): ANY, "keystone", restart_handler=ANY) mock_protect_service_accounts.assert_called_once_with() + @patch.object(utils, 'get_subordinate_release_packages') @patch.object(hooks, 'bootstrap_keystone') @patch.object(hooks, 'ensure_all_service_accounts_protected_for_pci_dss_options') @@ -669,17 +684,19 @@ class KeystoneRelationTests(CharmTestCase): @patch('keystone_utils.log') @patch('keystone_utils.relation_ids') @patch.object(hooks, 'stop_manager_instance') - def test_upgrade_charm_leader_no_packages(self, - mock_stop_manager_instance, - mock_relation_ids, - mock_log, - mock_is_db_initialised, - mock_is_db_ready, - os_release, - update, - mock_maybe_do_policyd_overrides, - mock_protect_service_accounts, - mock_bootstrap_keystone): + def test_upgrade_charm_leader_no_packages( + self, + mock_stop_manager_instance, + mock_relation_ids, + mock_log, + mock_is_db_initialised, + mock_is_db_ready, + os_release, + update, + mock_maybe_do_policyd_overrides, + mock_protect_service_accounts, + mock_bootstrap_keystone, + mock_get_subordinate_release_packages): os_release.return_value = 'havana' mock_is_db_initialised.return_value = True mock_is_db_ready.return_value = True @@ -688,6 +705,8 @@ class KeystoneRelationTests(CharmTestCase): self.services.return_value = ['apache2'] self.filter_installed_packages.return_value = [] + mock_get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) hooks.upgrade_charm() self.assertFalse(self.apt_install.called) self.assertTrue(update.called) @@ -817,6 +836,7 @@ class KeystoneRelationTests(CharmTestCase): # Still updates relations self.assertTrue(self.relation_ids.called) + @patch.object(utils, 'get_subordinate_release_packages') @patch.object(hooks, 'bootstrap_keystone') @patch.object(hooks, 'maybe_do_policyd_overrides') @patch.object(hooks, 'update_all_identity_relation_units') @@ -830,11 +850,14 @@ class KeystoneRelationTests(CharmTestCase): mock_log, os_release, update, mock_maybe_do_policyd_overrides, - mock_bootstrap_keystone): + mock_bootstrap_keystone, + mock_get_subordinate_release_packages): os_release.return_value = 'havana' self.filter_installed_packages.return_value = ['something'] self.is_elected_leader.return_value = False + mock_get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) hooks.upgrade_charm() self.assertTrue(self.apt_install.called) self.assertTrue(self.log.called) @@ -844,6 +867,7 @@ class KeystoneRelationTests(CharmTestCase): mock_maybe_do_policyd_overrides.assert_called_once_with( ANY, "keystone", restart_handler=ANY) + @patch.object(utils, 'get_subordinate_release_packages') @patch.object(hooks, 'bootstrap_keystone') @patch.object(hooks, 'maybe_do_policyd_overrides') @patch.object(hooks, 'update_all_identity_relation_units') @@ -860,11 +884,14 @@ class KeystoneRelationTests(CharmTestCase): update, mock_maybe_do_policyd_overrides, mock_bootstrap_keystone, + mock_get_subordinate_release_packages, ): os_release.return_value = 'havana' self.filter_installed_packages.return_value = [] self.is_elected_leader.return_value = False + mock_get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) hooks.upgrade_charm() self.assertFalse(self.apt_install.called) self.assertTrue(self.log.called) diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index 1baa2972..6c37f500 100644 --- a/unit_tests/test_keystone_utils.py +++ b/unit_tests/test_keystone_utils.py @@ -21,6 +21,8 @@ import os import subprocess import time +import charmhelpers.contrib.openstack.utils as os_utils + from test_utils import CharmTestCase import keystone_types as ks_types @@ -190,20 +192,35 @@ class TestKeystoneUtils(CharmTestCase): result = utils.determine_ports() self.assertEqual(result, ['80', '81']) + @patch.object(utils, 'get_subordinate_release_packages') @patch('charmhelpers.contrib.openstack.utils.config') - def test_determine_packages(self, _config): + def test_determine_packages( + self, + _config, + _get_subordinate_release_packages): self.os_release.return_value = 'havana' self.snap_install_requested.return_value = False _config.return_value = None + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) result = utils.determine_packages() ex = utils.BASE_PACKAGES + ['keystone', 'python-keystoneclient'] self.assertEqual(set(ex), set(result)) + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(['sub-pkg']), set()) + self.assertIn('sub-pkg', utils.determine_packages()) + @patch.object(utils, 'get_subordinate_release_packages') @patch('charmhelpers.contrib.openstack.utils.config') - def test_determine_packages_queens(self, _config): + def test_determine_packages_queens( + self, + _config, + _get_subordinate_release_packages): self.os_release.return_value = 'queens' self.snap_install_requested.return_value = False _config.return_value = None + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) result = utils.determine_packages() ex = utils.BASE_PACKAGES + [ 'keystone', 'python-keystoneclient', 'memcached', @@ -211,39 +228,69 @@ class TestKeystoneUtils(CharmTestCase): ] self.assertEqual(set(ex), set(result)) + @patch.object(utils, 'get_subordinate_release_packages') @patch('charmhelpers.contrib.openstack.utils.config') - def test_determine_packages_rocky(self, _config): + def test_determine_packages_rocky( + self, + _config, + _get_subordinate_release_packages): self.os_release.return_value = 'rocky' self.snap_install_requested.return_value = False _config.return_value = None + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) result = utils.determine_packages() ex = list(set( [p for p in utils.BASE_PACKAGES if not p.startswith('python-')] + ['memcached'] + utils.PY3_PACKAGES)) self.assertEqual(set(ex), set(result)) + @patch.object(utils, 'get_subordinate_release_packages') @patch('charmhelpers.contrib.openstack.utils.config') - def test_determine_packages_snap_install(self, _config): + def test_determine_packages_snap_install( + self, + _config, + _get_subordinate_release_packages): self.os_release.return_value = 'mitaka' self.snap_install_requested.return_value = True _config.return_value = None + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) result = utils.determine_packages() ex = utils.BASE_PACKAGES_SNAP + ['memcached'] self.assertEqual(set(ex), set(result)) + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(['sub-snap']), set()) + self.assertIn('sub-snap', utils.determine_packages()) - def test_determine_purge_packages(self): + @patch.object(utils, 'get_subordinate_release_packages') + def test_determine_purge_packages(self, _get_subordinate_release_packages): 'Ensure no packages are identified for purge prior to rocky' self.os_release.return_value = 'queens' + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) self.assertEqual(utils.determine_purge_packages(), []) + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set(['sub-purge'])) + self.assertEqual(utils.determine_purge_packages(), ['sub-purge']) - def test_determine_purge_packages_rocky(self): + @patch.object(utils, 'get_subordinate_release_packages') + def test_determine_purge_packages_rocky( + self, + _get_subordinate_release_packages): 'Ensure python packages are identified for purge at rocky' self.os_release.return_value = 'rocky' + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) self.assertEqual(utils.determine_purge_packages(), - [p for p in utils.BASE_PACKAGES - if p.startswith('python-')] + - ['python-keystone', 'python-memcache']) + sorted([p for p in utils.BASE_PACKAGES + if p.startswith('python-')] + + ['python-keystone', 'python-memcache'])) + _get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set(['sub-purge'])) + self.assertIn('sub-purge', utils.determine_purge_packages()) + @patch.object(utils, 'get_subordinate_release_packages') @patch.object(utils, 'bootstrap_keystone') @patch.object(utils, 'is_elected_leader') @patch.object(utils, 'disable_unused_apache_sites') @@ -254,13 +301,16 @@ class TestKeystoneUtils(CharmTestCase): def test_openstack_upgrade_leader( self, migrate_database, determine_packages, run_in_apache, os_path_exists, disable_unused_apache_sites, - mock_is_elected_leader, mock_bootstrap_keystone): + mock_is_elected_leader, mock_bootstrap_keystone, + mock_get_subordinate_release_packages): configs = MagicMock() self.test_config.set('openstack-origin', 'cloud:xenial-newton') self.os_release.return_value = 'ocata' determine_packages.return_value = [] os_path_exists.return_value = True run_in_apache.return_value = True + mock_get_subordinate_release_packages.return_value = \ + os_utils.SubordinatePackages(set(), set()) utils.do_openstack_upgrade(configs)