Merge lp:~thedac/charm-helpers/os-snaps into lp:charm-helpers

[David Ames]

Use track/channel
Determine installed version of snap
Support snap upgrades

[Pen Gale (pengale)]

"make test" runs for me , and code looks good, other than a null value getting passed in a way that I think will cause trouble.

I'd be +1 on this after that is addressed (either by being changed, or letting me know why I'm wrong to worry :-) )

review: Abstain

[lp:~thedac/charm-helpers/os-snaps]

770. By David Ames on 2017-07-24

Avoid NoneType error

[David Ames (thedac)]

Pete,

Thanks for that catch.

First some context, my goal was to re-use as many existing helper functions as possible to implement the snap integration as possible in order to avoid more changes to every charm.

os_release (which calls get_os_codename_package) get's called early often before the package has been installed. Thus the try/except when we expect no output from snap list. So we needed to handle this edge case.

In practice openstack_upgrade_available would not get called *before* the package has actually been installed. However, we should not leave that potential bug so I now have openstack_upgrade_available returning false if get_os_codename_package returns None.

Let me know what you think.

[Pen Gale (pengale)]

The latest change addresses my concern, and I am +1 to merging.

(I am still a little bit worried that swallowing the exception will mask a system problem with the snap command -- it's always nice to let an operator know when their tools are broken -- but you know this codebase better than I do, so I don't think that my worry should block a merge.)

review: Approve

=== modified file 'charmhelpers/contrib/openstack/utils.py'
--- charmhelpers/contrib/openstack/utils.py	2017-07-12 15:52:38 +0000
+++ charmhelpers/contrib/openstack/utils.py	2017-07-24 18:57:46 +0000
@@ -94,6 +94,7 @@
 
 from charmhelpers.fetch.snap import (
     snap_install,
+    snap_refresh,
     SNAP_CHANNELS,
 )
 
@@ -408,7 +409,16 @@
     '''Derive OpenStack release codename from an installed package.'''
 
     if snap_install_requested():
-        return None
+        cmd = ['snap', 'list', package]
+        try:
+            out = subprocess.check_output(cmd)
+        except subprocess.CalledProcessError as e:
+            return None
+        lines = out.split('\n')
+        for line in lines:
+            if package in line:
+                # Second item in list is Version
+                return line.split()[1]
 
     import apt_pkg as apt
 
@@ -623,13 +633,12 @@
                          a newer version of package.
     """
 
-    # TODO make upgrades snap aware
-    if snap_install_requested():
-        return False
-
     import apt_pkg as apt
     src = config('openstack-origin')
     cur_vers = get_os_version_package(package)
+    if not cur_vers:
+        # The package has not been installed yet do not attempt upgrade
+        return False
     if "swift" in package:
         codename = get_os_codename_install_source(src)
         avail_vers = get_os_version_codename_swift(codename)
@@ -2058,7 +2067,7 @@
 
     @param snaps: List of snaps
     @param src: String of openstack-origin or source of the form
-        snap:channel-series-release
+        snap:channel-series-track
     @param mode: String classic, devmode or jailmode
     @returns: Dictionary of snaps with channels and modes
     """
@@ -2068,13 +2077,14 @@
         return {}
 
     _src = src[5:]
-    channel, series, release = _src.split('-')
+    _channel, _series, _release = _src.split('-')
+    channel = '--channel={}/{}'.format(_release, _channel)
 
     return {snap: {'channel': channel, 'mode': mode}
             for snap in snaps}
 
 
-def install_os_snaps(snaps):
+def install_os_snaps(snaps, refresh=False):
     """Install OpenStack snaps from channel and with mode
 
     @param snaps: Dictionary of snaps with channels and modes of the form:
@@ -2091,7 +2101,13 @@
             return flag
         return '--{}'.format(flag)
 
-    for snap in snaps.keys():
-        snap_install(snap,
-                     _ensure_flag(snaps[snap]['channel']),
-                     _ensure_flag(snaps[snap]['mode']))
+    if refresh:
+        for snap in snaps.keys():
+            snap_refresh(snap,
+                         _ensure_flag(snaps[snap]['channel']),
+                         _ensure_flag(snaps[snap]['mode']))
+    else:
+        for snap in snaps.keys():
+            snap_install(snap,
+                         _ensure_flag(snaps[snap]['channel']),
+                         _ensure_flag(snaps[snap]['mode']))

=== modified file 'tests/contrib/openstack/test_openstack_utils.py'
--- tests/contrib/openstack/test_openstack_utils.py	2017-07-20 13:30:54 +0000
+++ tests/contrib/openstack/test_openstack_utils.py	2017-07-24 18:57:46 +0000
@@ -1935,7 +1935,7 @@
         mode = 'jailmode'
         src = 'snap:beta-xenial-ocata'
         expected = {snaps[0]: {'mode': mode,
-                               'channel': 'beta'}}
+                               'channel': '--channel=ocata/beta'}}
         self.assertEqual(
             expected,
             openstack.get_snaps_install_info_from_origin(snaps, src,
@@ -1950,7 +1950,7 @@
             openstack.get_snaps_install_info_from_origin(
                 snaps, src, mode=mode))
         mock_snap_install.assert_called_with(
-            'os_project', '--beta', '--jailmode')
+            'os_project', '--channel=ocata/beta', '--jailmode')
 
 
 if __name__ == '__main__':