livepatch-charmers team mailing list archive

[Barry Price <barry.price@xxxxxxxxxxxxx>]

Barry Price has proposed merging ~barryprice/canonical-livepatch-charm/+git/canonical-livepatch-charm:lp1778447 into canonical-livepatch-charm:master.

Commit message:
Address LP#1778447

Requested reviews:
  Livepatch charm developers (livepatch-charmers)
Related bugs:
  Bug #1778447 in Canonical Livepatch Charm: "Nagios check goes UNKNOWN when check-failed"
  https://bugs.launchpad.net/canonical-livepatch-charm/+bug/1778447

For more details, see:
https://code.launchpad.net/~barryprice/canonical-livepatch-charm/+git/canonical-livepatch-charm/+merge/348712
-- 
Your team Livepatch charm developers is requested to review the proposed merge of ~barryprice/canonical-livepatch-charm/+git/canonical-livepatch-charm:lp1778447 into canonical-livepatch-charm:master.

diff --git a/files/check_canonical-livepatch.py b/files/check_canonical-livepatch.py
index 4e6d54d..3a13062 100755
--- a/files/check_canonical-livepatch.py
+++ b/files/check_canonical-livepatch.py
@@ -3,7 +3,6 @@
 # Copyright (C) 2016 Canonical Ltd.
 
 import os
-import sys
 import nagios_plugin
 from subprocess import check_output, call
 
@@ -17,8 +16,7 @@ def check_package_installed():
     try:
         check_output(cmd, universal_newlines=True)
     except Exception:
-        print("canonical-livepatch snap is not installed")
-        sys.exit(2)
+        raise nagios_plugin.CriticalError("canonical-livepatch snap is not installed")
 
 
 ##############################################################################
@@ -65,12 +63,10 @@ def check_status():
 
     if err_lines:
         err = " ".join(err_lines)
-        print(err)
-        sys.exit(2)
+        raise nagios_plugin.CriticalError(err)
     elif wrn_lines:
         wrn = " ".join(wrn_lines)
-        print(wrn)
-        sys.exit(1)
+        raise nagios_plugin.WarnError(wrn)
 
 
 def lsb_release():
@@ -108,8 +104,9 @@ def is_container():
 def main():
     arch = os.uname()[4]
     if arch not in supported_archs:
-        print("canonical-livepatch not supported on this architecture ({}).".format(arch))
-        sys.exit(1)
+        raise nagios_plugin.CriticalError(
+            "canonical-livepatch not supported on this architecture ({}).".format(arch)
+        )
     elif is_container():
         print("canonical-livepatch not needed in OS containers.")
     else:
diff --git a/reactive/canonical_livepatch.py b/reactive/canonical_livepatch.py
index de797ec..ff7d803 100644
--- a/reactive/canonical_livepatch.py
+++ b/reactive/canonical_livepatch.py
@@ -6,8 +6,8 @@ from charmhelpers.core import hookenv
 from charmhelpers.contrib.charmsupport import nrpe
 from subprocess import check_call, check_output, CalledProcessError
 from time import sleep
-from os import path, uname
-from yaml import load, dump
+import os
+from yaml import load
 from distutils.version import LooseVersion
 
 
@@ -31,7 +31,7 @@ def wait_for_path(file_path, timeout=30):
     hookenv.log('Waiting for up to {}s for {}'.format(timeout, file_path))
     seconds_waited = 0
     interval = 1
-    while not (path.exists(file_path)):
+    while not (os.path.exists(file_path)):
         hookenv.log('{} does not exist - waiting {}s'.format(file_path, interval))
         sleep(interval)
         seconds_waited += interval
@@ -83,69 +83,16 @@ def get_patch_details():
     return kernel, patch_state
 
 
-def get_yaml_if_exists(path_to_yaml):
-    if path.exists(path_to_yaml):
-        with open(path_to_yaml, 'r') as stream:
-            try:
-                conf_yaml = load(stream)
-            except Exception:
-                hookenv.log('Unable to load YAML from {}'.format(path_to_yaml), hookenv.ERROR)
-    else:
-        conf_yaml = {}
-
-    return conf_yaml
-
-
-def configure_proxies(http_proxy=None, https_proxy=None, no_proxy=None):
-    config_path = '/var/snap/canonical-livepatch/common/config'
-    conf_yaml = get_yaml_if_exists(config_path)
-
-    if conf_yaml.get('http-proxy'):
-        if http_proxy is None:
-            del conf_yaml['http-proxy']
-        else:
-            conf_yaml['http-proxy'] = http_proxy
-    elif http_proxy:
-        conf_yaml['http-proxy'] = http_proxy
-
-    if conf_yaml.get('https-proxy'):
-        if https_proxy is None:
-            del conf_yaml['https-proxy']
-        else:
-            conf_yaml['https-proxy'] = https_proxy
-    elif https_proxy:
-        conf_yaml['https-proxy'] = https_proxy
-
-    if conf_yaml.get('no-proxy'):
-        if no_proxy is None:
-            del conf_yaml['no-proxy']
-        else:
-            conf_yaml['no-proxy'] = no_proxy
-    elif no_proxy:
-        conf_yaml['no-proxy'] = no_proxy
-
-    stream = open(config_path, 'w')
-    dump(conf_yaml, stream)
-
-
-def restart_livepatch():
-    # do a clean stop of the service first, 'restart' seems fragile right now
-    cmd = ['/bin/systemctl', 'stop', 'snap.canonical-livepatch.canonical-livepatchd.service']
-    hookenv.log('Stopping canonical-livepatch service')
-    try:
-        check_call(cmd, universal_newlines=True)
-    except CalledProcessError:
-        hookenv.log('Failed to stop the service', hookenv.ERROR)
-
-    # and now try to start it again, it may fail the first time!
-    cmd = ['/bin/systemctl', 'start', 'snap.canonical-livepatch.canonical-livepatchd.service']
-    hookenv.log('Starting canonical-livepatch service')
-    try:
-        check_call(cmd, universal_newlines=True)
-    except CalledProcessError:
-        hookenv.log('Failed to start the service - waiting 5s and then trying again', hookenv.ERROR)
-        sleep(5)
-        check_call(cmd, universal_newlines=True)
+def configure_proxies(proxy=None):
+    for proto in ('http-proxy', 'https-proxy'):
+        if proxy is None:
+            proxy = ''
+        cmd = ['/snap/bin/canonical-livepatch', 'config', '{}={}'.format(proto, proxy)]
+        hookenv.log('Configuring {} as {}'.format(proto, proxy))
+        try:
+            check_call(cmd, universal_newlines=True)
+        except CalledProcessError:
+            hookenv.log('Failed to set proxy', hookenv.ERROR)
 
 
 def activate_livepatch():
@@ -172,7 +119,6 @@ def activate_livepatch():
             unit_update('blocked', 'Activation failed')
         else:
             set_flag('canonical-livepatch.active')
-            clear_flag('canonical-livepatch.restart-needed')
             unit_update()
     else:
         hookenv.log('Unable to activate canonical-livepatch as no key has been set', hookenv.ERROR)
@@ -185,7 +131,7 @@ register_trigger(when='config.changed.livepatch_proxy', clear_flag='livepatch-pr
 
 @when('snap.installed.canonical-livepatch')
 def livepatch_supported():
-    arch = uname()[4]
+    arch = os.uname()[4]
     opts = layer.options('snap')
     supported_archs = opts['canonical-livepatch'].pop('supported-architectures', None)
     if supported_archs and arch not in supported_archs:
@@ -197,16 +143,31 @@ def livepatch_supported():
     else:
         set_flag('canonical-livepatch.supported')
 
+
 @when('canonical-livepatch.supported')
 @when_not('livepatch-proxy.configured')
-def proxy_configure():
+def proxy_settings():
     # Configure proxies early, if required - LP#1761661
+
+    preferred_proxy = None  # default to None
+
+    for key, value in os.environ.items():
+        # use environment's https_proxy or http_proxy if either are set
+        if key == 'https_proxy':
+            preferred_proxy = value
+        elif key == 'http_proxy':
+            preferred_proxy = value
+
+    # but if we have a charm configured proxy, prefer that
     config = hookenv.config()
-    proxy_url = config.get('livepatch_proxy')
-    if proxy_url:
-        configure_proxies(http_proxy=proxy_url, https_proxy=proxy_url)
+    charm_config_proxy = config.get('livepatch_proxy')
+    if charm_config_proxy:
+        preferred_proxy = charm_config_proxy
+
+    # whatever we prefer (or None), configure it!
+    configure_proxies(preferred_proxy)
+
     set_flag('livepatch-proxy.configured')
-    set_flag('canonical-livepatch.restart-needed')
 
 
 @when('snap.installed.canonical-livepatch')
@@ -215,10 +176,10 @@ def proxy_configure():
 def canonical_livepatch_connect():
     # So if we've just installed snapd on a trusty system, we will not be on
     # the HWE kernel yet and unfortunately need to reboot first!
-    current = LooseVersion(uname()[2])
+    current = LooseVersion(os.uname()[2])
     required = LooseVersion('4.4')
     uptrack_path = '/usr/sbin/uptrack-upgrade'
-    if path.exists(uptrack_path):
+    if os.path.exists(uptrack_path):
         hookenv.log('Ksplice/Uptrack detected, please remove it and reboot')
         unit_update('blocked', 'Remove ksplice and then reboot')
     elif current < required:
@@ -247,20 +208,6 @@ def update_key():
     activate_livepatch()
 
 
-@when('canonical-livepatch.connected', 'canonical-livepatch.restart-needed')
-def handle_restart():
-    unit_update('maintenance', 'Restarting client')
-
-    # restart the system service to pick up the new config
-    restart_livepatch()
-
-    # Make sure the service is ready for us
-    wait_for_livepatch()
-
-    # force a key update, we may have been blocked before proxy was set
-    update_key()
-
-
 @when('nrpe-external-master.available')
 def configure_nagios(nagios):
     if hookenv.hook_name() == 'update-status':
@@ -280,7 +227,7 @@ def configure_nagios(nagios):
         '/usr/lib/nagios/plugins/check_canonical-livepatch.py',
     )
     # don't overwrite an existing status file
-    if not path.exists('/var/lib/nagios/canonical-livepatch-status.txt'):
+    if not os.path.exists('/var/lib/nagios/canonical-livepatch-status.txt'):
         file_to_units(
             'files/canonical-livepatch-status.txt',
             '/var/lib/nagios/canonical-livepatch-status.txt',