Merge ~barryprice/charm-canonical-livepatch/+git/canonical-livepatch-charm:master into ~livepatch-charmers/charm-canonical-livepatch:master

[Barry Price]

[Barry Price (barryprice)]

CI passing:

https://jenkins.canonical.com/is-mojo-ci/view/live-sts-is-charm-testing-jobs/job/live-is-charm-test-livepatch-trusty/24/console

https://jenkins.canonical.com/is-mojo-ci/view/live-sts-is-charm-testing-jobs/job/live-is-charm-test-livepatch-xenial/36/console

[Barry Price (barryprice)]

I think we're ready for review here. CI runs:

https://jenkins.canonical.com/is-mojo-ci/view/live-sts-is-charm-testing-jobs/job/live-is-charm-test-livepatch-trusty/25/console

https://jenkins.canonical.com/is-mojo-ci/view/live-sts-is-charm-testing-jobs/job/live-is-charm-test-livepatch-xenial/37/console

[Stuart Bishop (stub)]

I guess at least with livepatch being a snap we don't have to worry about supporting the old format. Might be worth letting them know that this output is being scraped and if they change it they break things.

The patch looks good. I see a lot of hookenv.log() calls logging errors at the default INFO level, which you might want to fix. I'm unsure how much it matters with current Juju.

review: Approve

[Paul Gear (paulgear)]

See Stuart's comments about errors. I think all but one need changing.

review: Needs Fixing

[Stuart Bishop (stub)]

logging errors doesn't cause the charm to fail.

948802a... by Barry Price on 2017-10-12

Address LP#1721915, use appropriate log levels

2674170... by Barry Price on 2017-10-12

Address LP#1721915, use appropriate log levels

[Paul Gear (paulgear)]

Some stylistic comments inline, but LGTM.

review: Approve

1bcbce6... by Barry Price on 2017-10-12

Use preferred return syntax

diff --git a/reactive/canonical_livepatch.py b/reactive/canonical_livepatch.py
index bb9d547..8b39eab 100644
--- a/reactive/canonical_livepatch.py
+++ b/reactive/canonical_livepatch.py
@@ -7,7 +7,6 @@ from subprocess import check_call, check_output, CalledProcessError
 from time import sleep
 from os import path, uname
 from yaml import load, dump
-from platform import release
 from distutils.version import LooseVersion
 
 
@@ -42,7 +41,10 @@ def wait_for_path(file_path, timeout=30):
         sleep(interval)
         seconds_waited += interval
         if seconds_waited >= timeout:
-            hookenv.log('Giving up waiting for {}'.format(file_path))
+            hookenv.log(
+                'Giving up waiting for {}'.format(file_path),
+                hookenv.ERROR
+            )
             return
     return
 
@@ -57,37 +59,44 @@ def unit_update(status=None, message=None):
     if status and message:
         hookenv.status_set(status, message)
     else:
-        hookenv.status_set('active', 'Effective kernel {}'.format(
-            get_equiv_kernel_version())
+        patch_details = get_patch_details()
+        hookenv.status_set(
+                'active',
+                'Running kernel {}, patchState: {}'.format(*patch_details)
         )
 
 
-def get_equiv_kernel_version():
-    # default to actual running kernel
-    version_string = release()
-    livepatch_status = ''
+def get_patch_details():
+    kernel = patch_state = 'unknown'
 
     cmd = ['/snap/bin/canonical-livepatch', 'status']
     try:
-        livepatch_status = check_output(cmd, universal_newlines=True)
+        livepatch_state = check_output(cmd, universal_newlines=True)
     except CalledProcessError as e:
-        hookenv.log('Unable to get status: {}'.format(str(e)))
-        return version_string
+        hookenv.log('Unable to get status: {}'.format(str(e)), hookenv.ERROR)
+        return kernel, patch_state
 
     # status will usually pass YAML (but not always!)
     try:
-        status_yaml = load(livepatch_status)
+        status_yaml = load(livepatch_state)
     except Exception:
-        hookenv.log('Unable to parse status yaml')
-        return version_string
+        hookenv.log('Unable to parse status yaml', hookenv.ERROR)
+        return kernel, patch_state
 
     # even if we got YAML, be paranoid
     try:
-        version_string = status_yaml['kernel']
+        for k in status_yaml['status']:
+            if k['running'] is True:
+                kernel = k['kernel']
+                patch_state = k['livepatch']['patchState']
     except Exception:
-        hookenv.log('Unable to find kernel line in status yaml')
+        hookenv.log(
+            'Unable to find patching details in status yaml',
+            hookenv.ERROR
+        )
+        return kernel, patch_state
 
-    return version_string
+    return kernel, patch_state
 
 
 def get_yaml_if_exists(path_to_yaml):
@@ -97,7 +106,8 @@ def get_yaml_if_exists(path_to_yaml):
                 conf_yaml = load(stream)
             except Exception:
                 hookenv.log(
-                    'Unable to load YAML from {}'.format(path_to_yaml)
+                    'Unable to load YAML from {}'.format(path_to_yaml),
+                    hookenv.ERROR
                 )
     else:
         conf_yaml = {}
@@ -148,7 +158,7 @@ def restart_livepatch():
     try:
         check_call(cmd, universal_newlines=True)
     except CalledProcessError:
-        hookenv.log('Failed to stop the service')
+        hookenv.log('Failed to stop the service', hookenv.ERROR)
 
     # and now try to start it again, it may fail the first time!
     cmd = [
@@ -161,7 +171,8 @@ def restart_livepatch():
         check_call(cmd, universal_newlines=True)
     except CalledProcessError:
         hookenv.log(
-            'Failed to start the service - waiting 5s and then trying again'
+            'Failed to start the service - waiting 5s and then trying again',
+            hookenv.ERROR
         )
         sleep(5)
         check_call(cmd, universal_newlines=True)
@@ -182,7 +193,10 @@ def activate_livepatch():
         try:
             check_output(cmd, universal_newlines=True)
         except CalledProcessError as e:
-            hookenv.log('Unable to deactivate: {}'.format(str(e)))
+            hookenv.log(
+                'Unable to deactivate: {}'.format(str(e)),
+                hookenv.ERROR
+            )
 
         cmd = [
             '/snap/bin/canonical-livepatch',
@@ -193,7 +207,10 @@ def activate_livepatch():
         try:
             check_output(cmd, universal_newlines=True)
         except CalledProcessError as e:
-            hookenv.log('Unable to activate: {}'.format(str(e)))
+            hookenv.log(
+                'Unable to activate: {}'.format(str(e)),
+                hookenv.ERROR
+            )
             remove_state('canonical-livepatch.active')
             unit_update('blocked', 'Activation failed')
         else:
@@ -201,7 +218,8 @@ def activate_livepatch():
             unit_update()
     else:
         hookenv.log(
-            'Unable to activate canonical-livepatch as no key has been set'
+            'Unable to activate canonical-livepatch as no key has been set',
+            hookenv.ERROR
         )
         remove_state('canonical-livepatch.active')
         unit_update(
@@ -214,10 +232,20 @@ def activate_livepatch():
 def livepatch_supported():
     arch = uname()[4]
     opts = layer.options('snap')
-    supported_archs = opts['canonical-livepatch'].pop('supported-architectures', None)
+    supported_archs = opts['canonical-livepatch'].pop(
+        'supported-architectures',
+        None
+    )
     if supported_archs and arch not in supported_archs:
-        hookenv.log('Livepatch does not currently support this architecture: {}'.format(arch))
-        unit_update('blocked', 'Architecture {} is not supported by livepatch'.format(arch))
+        hookenv.log(
+            'Livepatch does not currently support {} architecture'.format(
+                arch
+            )
+        )
+        unit_update(
+            'blocked',
+            'Architecture {} is not supported by livepatch'.format(arch)
+        )
     if is_container():
         hookenv.log('OS container detected, livepatch is not needed')
         unit_update('blocked', 'Livepatch is not needed in OS containers')