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

[David Ames]

Helpers for install OpenStack snaps in OpenStack charms

[Alex Kavanagh (ajkavanagh)]

Please see inline comments.

Also, are there no tests for the new functions? I see patches to some of the existing tests for patching out the snaps existence test, but not for the new functions.

(I picked 'needs information' rather than 'needs fixing' because I'm not sure about the design aspect of passing in the post_snap function).

review: Needs Information

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

754. By Jorge Niedbalski on 2017-06-16

[freyes, r=niedbalski,ajkavanagh] Invoke apt-get only if there
missing packages.

755. By Jorge Niedbalski on 2017-06-16

[freyes,r=niedbalski,ajkavanagh] Fix for test_is_ipv6_disabled.

[David Ames (thedac)]

Alex,

Thanks for your suggestions.

I wrote the unit tests first which made make the changes simpler ;)

Fixed pretty much everything you requested.

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

756. By Jorge Niedbalski on 2017-06-22

[niedbalski, r=freyes,ajkavanagh] Partial fix required for LP:#1699565.

757. By Jorge Niedbalski on 2017-06-29

[billy-olsen, r=niedbalski] Ensure the config_flags_parser returns an
OrderedDict.

758. By Tim Van Steenburgh on 2017-07-05

v0.17.0

[Corey Bryant (corey.bryant)]

Hi David, I have a few comments below. I think the major thing that is messing things up now is that the current snaps aren't published to a track like they all will be in the future. I'll look into publishing them to the edge channel of the ocata track.

[David Ames (thedac)]

Corey,

I see your point about tracks. Do let me know when you have the snaps published to both the track and the channel and I'll get this MP ready to handle it.

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

759. By Stuart Bishop on 2017-07-12

[peter-sabaini,r=stub] Helper module for bcache devices

[David Ames (thedac)]

Corey,

I have responded inline on where we are at with your concerns. The next commit fixes the typos.

[Corey Bryant (corey.bryant)]

LGTM. I think we should move ahead with what David has here. Once we get tracks enabled [1] we can land track support in charm-helpers.

[1] https://forum.snapcraft.io/t/track-request-for-openstack-core-snaps/1282

[Corey Bryant (corey.bryant)]

review: Approve

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

760. By Jorge Niedbalski on 2017-07-12

[freyes, r=niedbalski] Add support to change a user's password in MySQL.

761. By Edward Hope-Morley on 2017-07-13

[zhhuabj,r=ajkavanagh]

When OpenStack clouds get bigger, more messaging transactions
will happen, which will cause more load on rabbitmq server. In
order to mitigate this, polling_interval rpc_response_timeout
and report_interval values are important to adjust accordingly
to the size of OpenStack cluster.

So this patch supports setting those values in neutron-api
centrally, then:

1. polling_interval

   neutron-openvswitch charm gets polling_interval via it's
   relations and set it in [agent] of ml2_conf.ini or
   openvswitch_agent.ini(>=Mitaka)

2. rpc_response_timeout

   neutron-gateway charm gets rpc_response_timeout via it's
   relations and set it in [default] of neutron.conf

3. report_interval

   Both neutron-openvswitch charm and neutron-gateway charm
   get report_interval via it's relations and set it in
   [agent] of neutron.conf

Thus we need to modifty charm-helpers to also set those values
in NeutronAPIContext.

Related-Bug: 1685788

[Alex Kavanagh (ajkavanagh)]

Looks good. Just a single comment for food for thought in the future.

review: Approve

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

762. By James Page on 2017-07-18

Fix release name typos in CLOUD_ARCHIVE_POCKETS

763. By David Ames on 2017-07-18

[thedac, r=coreycb,tinwood] OpenStack Charm snap helpers

=== modified file 'charmhelpers/contrib/openstack/context.py'
--- charmhelpers/contrib/openstack/context.py	2017-05-01 05:08:50 +0000
+++ charmhelpers/contrib/openstack/context.py	2017-07-12 15:52:52 +0000
@@ -97,6 +97,7 @@
     git_determine_usr_bin,
     git_determine_python_path,
     enable_memcache,
+    snap_install_requested,
 )
 from charmhelpers.core.unitdata import kv
 
@@ -244,6 +245,11 @@
                     'database_password': rdata.get(password_setting),
                     'database_type': 'mysql'
                 }
+                # Note(coreycb): We can drop mysql+pymysql if we want when the
+                # following review lands, though it seems mysql+pymysql would
+                # be preferred. https://review.openstack.org/#/c/462190/
+                if snap_install_requested():
+                    ctxt['database_type'] = 'mysql+pymysql'
                 if self.context_complete(ctxt):
                     db_ssl(rdata, ctxt, self.ssl_dir)
                     return ctxt

=== modified file 'charmhelpers/contrib/openstack/utils.py'
--- charmhelpers/contrib/openstack/utils.py	2017-06-29 22:46:34 +0000
+++ charmhelpers/contrib/openstack/utils.py	2017-07-12 15:52:52 +0000
@@ -51,6 +51,7 @@
     status_set,
     hook_name,
     application_version_set,
+    cached,
 )
 
 from charmhelpers.core.strutils import BasicStringComparator
@@ -90,6 +91,12 @@
     GPGKeyError,
     get_upstream_version
 )
+
+from charmhelpers.fetch.snap import (
+    snap_install,
+    SNAP_CHANNELS,
+)
+
 from charmhelpers.contrib.storage.linux.utils import is_block_device, zap_disk
 from charmhelpers.contrib.storage.linux.loopback import ensure_loopback_device
 from charmhelpers.contrib.openstack.exceptions import OSContextError
@@ -327,8 +334,10 @@
         return ca_rel
 
     # Best guess match based on deb string provided
-    if src.startswith('deb') or src.startswith('ppa'):
-        for k, v in six.iteritems(OPENSTACK_CODENAMES):
+    if (src.startswith('deb') or
+            src.startswith('ppa') or
+            src.startswith('snap')):
+        for v in OPENSTACK_CODENAMES.values():
             if v in src:
                 return v
 
@@ -397,6 +406,10 @@
 
 def get_os_codename_package(package, fatal=True):
     '''Derive OpenStack release codename from an installed package.'''
+
+    if snap_install_requested():
+        return None
+
     import apt_pkg as apt
 
     cache = apt_cache()
@@ -610,6 +623,10 @@
                          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)
@@ -2016,3 +2033,65 @@
     policy.update(items)
     with open(filename, "w") as fd:
         fd.write(json.dumps(policy, indent=4))
+
+
+@cached
+def snap_install_requested():
+    """ Determine if installing from snaps
+
+    If openstack-origin is of the form snap:channel-series-release
+    and channel is in SNAPS_CHANNELS return True.
+    """
+    origin = config('openstack-origin')
+    if not origin.startswith('snap:'):
+        return False
+
+    _src = origin[5:]
+    channel, series, release = _src.split('-')
+    if channel.lower() in SNAP_CHANNELS:
+        return True
+    return False
+
+
+def get_snaps_install_info_from_origin(snaps, src, mode='classic'):
+    """Generate a dictionary of snap install information from origin
+
+    @param snaps: List of snaps
+    @param src: String of openstack-origin or source of the form
+        snap:channel-series-release
+    @param mode: String classic, devmode or jailmode
+    @returns: Dictionary of snaps with channels and modes
+    """
+
+    if not src.startswith('snap:'):
+        juju_log("Snap source is not a snap origin", 'WARN')
+        return {}
+
+    _src = src[5:]
+    channel, series, release = _src.split('-')
+
+    return {snap: {'channel': channel, 'mode': mode}
+            for snap in snaps}
+
+
+def install_os_snaps(snaps):
+    """Install OpenStack snaps from channel and with mode
+
+    @param snaps: Dictionary of snaps with channels and modes of the form:
+        {'snap_name': {'channel': 'snap_channel',
+                       'mode': 'snap_mode'}}
+        Where channel a snapstore channel and mode is --classic, --devmode or
+        --jailmode.
+    @param post_snap_install: Callback function to run after snaps have been
+    installed
+    """
+
+    def _ensure_flag(flag):
+        if flag.startswith('--'):
+            return flag
+        return '--{}'.format(flag)
+
+    for snap in snaps.keys():
+        snap_install(snap,
+                     _ensure_flag(snaps[snap]['channel']),
+                     _ensure_flag(snaps[snap]['mode']))

=== modified file 'charmhelpers/fetch/snap.py'
--- charmhelpers/fetch/snap.py	2017-03-03 09:41:22 +0000
+++ charmhelpers/fetch/snap.py	2017-07-12 15:52:52 +0000
@@ -27,6 +27,12 @@
 SNAP_NO_LOCK = 1  # The return code for "couldn't acquire lock" in Snap (hopefully this will be improved).
 SNAP_NO_LOCK_RETRY_DELAY = 10  # Wait X seconds between Snap lock checks.
 SNAP_NO_LOCK_RETRY_COUNT = 30  # Retry to acquire the lock X times.
+SNAP_CHANNELS = [
+    'edge',
+    'beta',
+    'candidate',
+    'stable',
+]
 
 
 class CouldNotAcquireLockException(Exception):

=== modified file 'charmhelpers/fetch/ubuntu.py'
--- charmhelpers/fetch/ubuntu.py	2017-05-25 13:46:14 +0000
+++ charmhelpers/fetch/ubuntu.py	2017-07-12 15:52:52 +0000
@@ -364,6 +364,7 @@
         (r"^cloud:(.*)-(.*)\/staging$", _add_cloud_staging),
         (r"^cloud:(.*)-(.*)$", _add_cloud_distro_check),
         (r"^cloud:(.*)$", _add_cloud_pocket),
+        (r"^snap:.*-(.*)-(.*)$", _add_cloud_distro_check),
     ])
     if source is None:
         source = ''

=== modified file 'tests/contrib/openstack/test_openstack_utils.py'
--- tests/contrib/openstack/test_openstack_utils.py	2017-04-10 17:51:42 +0000
+++ tests/contrib/openstack/test_openstack_utils.py	2017-07-12 15:52:52 +0000
@@ -8,6 +8,7 @@
 from mock import MagicMock, patch, call
 
 from charmhelpers.fetch import ubuntu as fetch
+from charmhelpers.core.hookenv import flush
 import charmhelpers.contrib.openstack.utils as openstack
 
 import six
@@ -292,8 +293,10 @@
     def test_get_swift_codename_none(self):
         self.assertEquals(openstack.get_swift_codename('1.2.3'), None)
 
-    def test_os_codename_from_package(self):
+    @patch.object(openstack, 'snap_install_requested')
+    def test_os_codename_from_package(self, mock_snap_install_requested):
         """Test deriving OpenStack codename from an installed package"""
+        mock_snap_install_requested.return_value = False
         with patch('apt_pkg.Cache') as cache:
             cache.return_value = self._apt_cache()
             for pkg, vers in six.iteritems(FAKE_REPO):
@@ -305,18 +308,24 @@
                 self.assertEquals(openstack.get_os_codename_package(pkg),
                                   vers['os_release'])
 
+    @patch.object(openstack, 'snap_install_requested')
     @patch('charmhelpers.contrib.openstack.utils.error_out')
-    def test_os_codename_from_bad_package_version(self, mocked_error):
+    def test_os_codename_from_bad_package_version(self, mocked_error,
+                                                  mock_snap_install_requested):
         """Test deriving OpenStack codename for a poorly versioned package"""
+        mock_snap_install_requested.return_value = False
         with patch('apt_pkg.Cache') as cache:
             cache.return_value = self._apt_cache()
             openstack.get_os_codename_package('bad-version')
             _e = ('Could not determine OpenStack codename for version 2200.1')
             mocked_error.assert_called_with(_e)
 
+    @patch.object(openstack, 'snap_install_requested')
     @patch('charmhelpers.contrib.openstack.utils.error_out')
-    def test_os_codename_from_bad_package(self, mocked_error):
+    def test_os_codename_from_bad_package(self, mocked_error,
+                                          mock_snap_install_requested):
         """Test deriving OpenStack codename from an unavailable package"""
+        mock_snap_install_requested.return_value = False
         with patch('apt_pkg.Cache') as cache:
             cache.return_value = self._apt_cache()
             try:
@@ -329,8 +338,11 @@
                 'candidate: foo'
             mocked_error.assert_called_with(e)
 
-    def test_os_codename_from_bad_package_nonfatal(self):
+    @patch.object(openstack, 'snap_install_requested')
+    def test_os_codename_from_bad_package_nonfatal(
+            self, mock_snap_install_requested):
         """Test OpenStack codename from an unavailable package is non-fatal"""
+        mock_snap_install_requested.return_value = False
         with patch('apt_pkg.Cache') as cache:
             cache.return_value = self._apt_cache()
             self.assertEquals(
@@ -338,9 +350,12 @@
                 openstack.get_os_codename_package('foo', fatal=False)
             )
 
+    @patch.object(openstack, 'snap_install_requested')
     @patch('charmhelpers.contrib.openstack.utils.error_out')
-    def test_os_codename_from_uninstalled_package(self, mock_error):
+    def test_os_codename_from_uninstalled_package(self, mock_error,
+                                                  mock_snap_install_requested):
         """Test OpenStack codename from an available but uninstalled pkg"""
+        mock_snap_install_requested.return_value = False
         with patch('apt_pkg.Cache') as cache:
             cache.return_value = self._apt_cache()
             try:
@@ -351,8 +366,11 @@
                  'cinder-common')
             mock_error.assert_called_with(e)
 
-    def test_os_codename_from_uninstalled_package_nonfatal(self):
+    @patch.object(openstack, 'snap_install_requested')
+    def test_os_codename_from_uninstalled_package_nonfatal(
+            self, mock_snap_install_requested):
         """Test OpenStack codename from avail uninstalled pkg is non fatal"""
+        mock_snap_install_requested.return_value = False
         with patch('apt_pkg.Cache') as cache:
             cache.return_value = self._apt_cache()
             self.assertEquals(
@@ -360,9 +378,12 @@
                 openstack.get_os_codename_package('cinder-common', fatal=False)
             )
 
+    @patch.object(openstack, 'snap_install_requested')
     @patch('charmhelpers.contrib.openstack.utils.error_out')
-    def test_os_version_from_package(self, mocked_error):
+    def test_os_version_from_package(self, mocked_error,
+                                     mock_snap_install_requested):
         """Test deriving OpenStack version from an installed package"""
+        mock_snap_install_requested.return_value = False
         with patch('apt_pkg.Cache') as cache:
             cache.return_value = self._apt_cache()
             for pkg, vers in six.iteritems(FAKE_REPO):
@@ -373,9 +394,12 @@
                 self.assertEquals(openstack.get_os_version_package(pkg),
                                   vers['os_version'])
 
+    @patch.object(openstack, 'snap_install_requested')
     @patch('charmhelpers.contrib.openstack.utils.error_out')
-    def test_os_version_from_bad_package(self, mocked_error):
+    def test_os_version_from_bad_package(self, mocked_error,
+                                         mock_snap_install_requested):
         """Test deriving OpenStack version from an uninstalled package"""
+        mock_snap_install_requested.return_value = False
         with patch('apt_pkg.Cache') as cache:
             cache.return_value = self._apt_cache()
             try:
@@ -388,8 +412,11 @@
                 'candidate: foo'
             mocked_error.assert_called_with(e)
 
-    def test_os_version_from_bad_package_nonfatal(self):
+    @patch.object(openstack, 'snap_install_requested')
+    def test_os_version_from_bad_package_nonfatal(
+            self, mock_snap_install_requested):
         """Test OpenStack version from an uninstalled package is non-fatal"""
+        mock_snap_install_requested.return_value = False
         with patch('apt_pkg.Cache') as cache:
             cache.return_value = self._apt_cache()
             self.assertEquals(
@@ -1886,6 +1913,47 @@
             openstack.os_application_version_set('cinder-common')
             mock_application_version_set.assert_called_with('mitaka')
 
+    @patch('charmhelpers.contrib.openstack.utils.config')
+    def test_snap_install_requested(self, config):
+        # Expect True
+        flush('snap_install_requested')
+        config.return_value = 'snap:edge-xenial-ocata'
+        self.assertTrue(openstack.snap_install_requested())
+        flush('snap_install_requested')
+        config.return_value = 'snap:BETA-xenial-ocata'
+        self.assertTrue(openstack.snap_install_requested())
+        # Expect False
+        flush('snap_install_requested')
+        config.return_value = 'snap:None-xenial-ocata'
+        self.assertFalse(openstack.snap_install_requested())
+        flush('snap_install_requested')
+        config.return_value = 'cloud:xenial-ocata'
+        self.assertFalse(openstack.snap_install_requested())
+
+    def test_get_snaps_install_info_from_origin(self):
+        snaps = ['os_project']
+        mode = 'jailmode'
+        src = 'snap:beta-xenial-ocata'
+        expected = {snaps[0]: {'mode': mode,
+                               'channel': 'beta'}}
+        self.assertEqual(
+            expected,
+            openstack.get_snaps_install_info_from_origin(snaps, src,
+                                                         mode=mode))
+
+    @patch.object(openstack, 'snap_install')
+    def test_install_os_snaps(self, mock_snap_install):
+        snaps = ['os_project']
+        mode = 'jailmode'
+        src = 'snap:beta-xenial-ocata'
+        post_install = MagicMock()
+
+        openstack.install_os_snaps(
+            openstack.get_snaps_install_info_from_origin(
+                    snaps, src, mode=mode))
+        mock_snap_install.assert_called_with(
+            'os_project', '--beta', '--jailmode')
+
 
 if __name__ == '__main__':
     unittest.main()