livepatch-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Approve

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.

Diff comments:

> diff --git a/reactive/canonical_livepatch.py b/reactive/canonical_livepatch.py
> index bb9d547..fbdaf01 100644
> --- a/reactive/canonical_livepatch.py
> +++ b/reactive/canonical_livepatch.py
> @@ -57,37 +56,41 @@ 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)))

This should really be ERROR level ( hookenv.log('msg', hookenv.ERROR) )

> -        return version_string
> +        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')

Another error. One day it might even matter what you choose.

> -        return version_string
> +        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')

And another error. Maybe there are more.

> +        return(kernel, patch_state)
>  
> -    return version_string
> +    return(kernel, patch_state)
>  
>  
>  def get_yaml_if_exists(path_to_yaml):
> @@ -214,10 +217,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
> +            )
> +        )

yeah, another error level message. I guess the charm is pretty silent until things mess up :)

> +        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')


-- 
https://code.launchpad.net/~barryprice/canonical-livepatch-charm/+git/canonical-livepatch-charm/+merge/331851
Your team Livepatch charm developers is subscribed to branch canonical-livepatch-charm:master.