livepatch-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Approve

Yup, but see comments.

Diff comments:

> diff --git a/files/check_canonical-livepatch.py b/files/check_canonical-livepatch.py
> index 7878169..86d41ba 100755
> --- a/files/check_canonical-livepatch.py
> +++ b/files/check_canonical-livepatch.py
> @@ -5,70 +5,108 @@
>  import os
>  import nagios_plugin
>  from subprocess import check_output, call
> +from yaml import safe_load
>  
>  supported_archs = ['x86_64']
>  
>  
> -##############################################################################
> -
> -def check_package_installed():
> +def check_snap_installed():
> +    """Confirm the snap is installed, raise an error if not"""
>      cmd = ['snap', 'list', 'canonical-livepatch']
>      try:
>          check_output(cmd, universal_newlines=True)
>      except Exception:
> -        raise nagios_plugin.CriticalError("canonical-livepatch snap is not installed")
> +        raise nagios_plugin.CriticalError('canonical-livepatch snap is not installed')
>  
>  
> -##############################################################################
> +def load_status():
> +    """Load the cached status from disk, return it as a string"""
> +    livepatch_output_path = '/var/lib/nagios/canonical-livepatch-status.txt'
>  
> -def check_vmlinuz():
> -    vmlinuz = '/vmlinuz'
> -    if os.path.exists(vmlinuz):
> -        full_kernel_path = os.path.realpath(vmlinuz)
> -    elif os.path.exists('/boot' + vmlinuz):
> -        vmlinuz = '/boot' + vmlinuz
> -        full_kernel_path = os.path.realpath(vmlinuz)
> -    else:
> -        return 'no /vmlinuz or /boot/vmlinuz'
> -    kernel_filename = os.path.basename(full_kernel_path)
> -    # remove 'vmlinuz'-' from start:
> -    kernel_version = '-'.join(kernel_filename.split('-', 1)[1:])
> -    # check for '-generic-pae' kernels that need two removes:
> -    if '-generic-pae' in kernel_version:
> -        kernel_version = '-'.join(kernel_version.split('-')[:-1])
> -    # remove e.g. '-generic' from end:
> -    kernel_version = '-'.join(kernel_version.split('-')[:-1])
> -    return kernel_version.strip()
> +    with open(livepatch_output_path, 'r') as canonical_livepatch_log:
> +        livepatch_status = canonical_livepatch_log.read()
> +
> +    return livepatch_status
> +
> +
> +def check_serious_errors():
> +    """In serious error cases, the output will not be YAML"""
> +    livepatch_status = load_status()
> +
> +    # if it's empty, something's definitely wrong
> +    if livepatch_status == '':
> +        raise nagios_plugin.CriticalError('No output from canonical-livepatch status')
> +
> +    # in some cases, it's obviously not valid YAML
> +    try:
> +        livepatch_yaml = safe_load(livepatch_status)
> +    except Exception:
> +        raise nagios_plugin.CriticalError(livepatch_status.replace('\n', ' '))
> +
> +    # in other cases, it's less obvious until we try to parse
> +    try:
> +        livepatch_yaml.get('status')
> +    except Exception:
> +        raise nagios_plugin.CriticalError(livepatch_status.replace('\n', ' '))

This function will pass if the load_status returns something like 'there_is_no_status: foo'. livepatch_yaml.get('status') works because it is a dict, but returns None because there is no status key.

>  
>  
> -##############################################################################
> +def active_kernel_version():
> +    """Return the active kernel version, from livepatch's perspective"""
> +    livepatch_status = load_status()
> +    status_yaml = safe_load(livepatch_status)
> +    for kernel in status_yaml.get('status'):
> +        if kernel.get('running') is True:
> +            return kernel.get('kernel')

Yup, this is how you write return in Python.

> +
>  
>  def check_status():
> -    livepatch_output_path = '/var/lib/nagios/canonical-livepatch-status.txt'
> -    err_lines = []
> -    wrn_lines = []
> +    """Check the cached status, raise an error if we find any issues"""
> +    livepatch_status = load_status()
> +    errors = []
>  
> -    with open(livepatch_output_path, 'r') as canonical_livepatch_log:
> -        for line in canonical_livepatch_log:
> -            line = line.strip()
> -            if 'State:' in line:
> -                if 'apply-failed' in line:
> -                    err_lines.append('Livepatch failed to apply patches.')
> -                elif 'check-failed' in line:
> -                    err_lines.append('Livepatch failed to check the remote service for patches.')
> -                elif 'unknown' in line:
> -                    err_lines.append('Livepatch reports an unknown error.')
> -                elif 'kernel-upgrade-required' in line:
> -                    err_lines.append('A kernel upgrade (and reboot) is required.')
> -            elif 'Machine is not enabled' in line:
> -                err_lines.append('Machine is not enabled.')
> -
> -    if err_lines:
> -        err = " ".join(err_lines)
> -        raise nagios_plugin.CriticalError(err)
> -    elif wrn_lines:
> -        wrn = " ".join(wrn_lines)
> -        raise nagios_plugin.WarnError(wrn)
> +    status_yaml = safe_load(livepatch_status)
> +
> +    for kernel in status_yaml.get('status'):
> +        if kernel.get('running') is True:
> +            check_state = kernel.get('livepatch').get('checkState')
> +            patch_state = kernel.get('livepatch').get('patchState')
> +
> +    check_state_error = check_check_state(check_state)
> +    patch_state_error = check_patch_state(patch_state)
> +
> +    if check_state_error:
> +        errors.append(check_state_error)
> +    if patch_state_error:
> +        errors.append(patch_state_error)
> +
> +    if errors:
> +        raise nagios_plugin.CriticalError(' '.join(errors))
> +
> +
> +def check_check_state(check_state):
> +    """Check for issues with checkState, including unexpected output"""
> +    if check_state in ['checked', 'needs-check']:
> +        pass
> +    elif check_state == 'check-failed':
> +        return('Livepatch failed to check the remote service for patches.')
> +    else:
> +        return('Unknown check state: {}'.format(check_state))

return is a keyword, so return 'result' rather than return('result'). If you want the brackets for clarity, there should be whitespace between the return keyword and the opening brace. Although flake8 doesn't pick this up, so maybe I'm just having an opinion.

> +
> +
> +def check_patch_state(patch_state):
> +    """Check for issues with patchState, including unexpected output"""
> +    if patch_state in ['applied', 'nothing-to-apply']:
> +        pass
> +    elif patch_state == 'unapplied':
> +        return('Livepatch has not applied the downloaded patches.')
> +    elif patch_state == 'applied-with-bug':
> +        return('Livepatch has detected a kernel bug while applying patches.')
> +    elif patch_state == 'apply-failed':
> +        return('Livepatch failed to apply patches.')
> +    elif patch_state == 'kernel-upgrade-required':
> +        return('A kernel upgrade (and reboot) is required.')
> +    else:
> +        return('Unknown patch state: {}'.format(patch_state))

returns don't look like Python again.

>  
>  
>  def lsb_release():


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