Merge lp:~1chb1n/charm-helpers/heat-amulet into lp:charm-helpers

[Ryan Beisner]

* Prep helpers for Wily/Liberty; resolve additional Vivid issues.

* Add heat client method.

* Add generic openstack resource status check and delete methods.

* Add create_or_get_keypair method.

* Add file_to_url method.

* Add get_ubuntu_releases - uses distro_info to create a list property containing Ubuntu release codenames, in order of release.

* Add get_ubuntu_release_from_sentry - return lsb_release codename from unit.

* Flag validate_services with deprecation warning. This method should be eventually be deprecated in favor of the following, after charm tests are updated to use validate_services_by_name.

* Add validate_services_by_name - uses a dictionary of units:service-names to check service status, automatically generating command based on init system type. Tests should now be updated to use this.

* Add debug logging.

Tracking bug: https://bugs.launchpad.net/charm-helpers/+bug/1461535

This should be landed in conjunction with the new heat amulet tests @ https://code.launchpad.net/~1chb1n/charms/trusty/heat/next-amulet-init/+merge/258105.

## resource status and resource delete rationale:
New and existing amulet tests can be made with less code effort or duplication.

Rather than having numerous delete_XYZ methods for each of the numerous openstack types/objects, use delete_resource and pass a pointer. Similarly, instead of having wait/check/timeout/confirm loops built into numerous methods, use resource_reaches_status + a pointer.

[Corey Bryant (corey.bryant)]

The changes make sense to me. How about 'resource' instead of 'thing'? Something like delete_openstack_resource() seems like it would be understandable if someone was reading through code.

[Corey Bryant (corey.bryant)]

Also there's one comment inline.

[Corey Bryant (corey.bryant)]

Looks like there are also some lint errors.

[Ryan Beisner (1chb1n)]

See that's why I ask you! Resource makes too much sense. Ack on the lint, already resolved locally. Also fully agree on the inline comment about comments. Thanks Corey!

[Ryan Beisner (1chb1n)]

FYI - all amulet tests pass except Vivid, due to bug 1461535. Addressing @ charmhelpers as well as the heat branch.

[Corey Bryant (corey.bryant)]

Looking good. I'd just remove the old validate_services() now and rename validate_services_by_name() to validate_services(), and land all the charm test updates at the same time.

[Ryan Beisner (1chb1n)]

I'd prefer to not do that actually. I'd like to take it smaller chunks.

This is what I'm aiming for:

1. Land heat amulet tests, at the same time unblocking vivid for all of the other amulet tests.

2. Enable and land vivid coverage in all of the charms with existing amulet tests, while also updating each of them.

3. Deprecate the old validate_services after it is confirmed to be not-used.

4. Add amulet tests for charms which don't yet have them.

Whaddaya think?

[Ryan Beisner (1chb1n)]

Also, rmq reply inline.

[Ryan Beisner (1chb1n)]

Oh, add to (3): Deprecate delete_image after the relevant tests have been updated to use delete_resource.

[Corey Bryant (corey.bryant)]

> I'd prefer to not do that actually. I'd like to take it smaller chunks.
>
> This is what I'm aiming for:
>
> 1. Land heat amulet tests, at the same time unblocking vivid for all of the
> other amulet tests.
>
> 2. Enable and land vivid coverage in all of the charms with existing amulet
> tests, while also updating each of them.
>
> 3. Deprecate the old validate_services after it is confirmed to be not-used.
>
> 4. Add amulet tests for charms which don't yet have them.
>
> Whaddaya think?

I just prefer not to land code that's going to be pulled back out in a week. But I can't land to c-h anyway so it's not up to me.

Now that I think of it though, and more importantly, I don't know what the process is for deprecating functions that aren't openstack specific. Someone else might already be using them and some of these changes would break them. I wonder what the precedence is.

[Ryan Beisner (1chb1n)]

I can definitely see your point, which raises more caution:

The original validate_services method isn't actually broken in any way. Misnamed, maybe, but not broken. It just runs commands and reports back pass/fail. If the test writer passes the proper service-checking (or other arbitrary) commands, it will happily run them. A more appropriate name for the method may have been something like run_these_commands_on_these_units(), as it's really not service-aware at all.

Maybe we shouldn't deprecate or change the behavior of validate_services at all, as there is no way to know who that will break.

I'd suggest that we:

leave validate_services untouched, not "fixed"

add validate_services_by_name, as proposed, and roll tests to it as we update each charm.

discuss and handle the topic of deprecation separately, if at all.

[Corey Bryant (corey.bryant)]

Good points. I think that sounds like a good plan, and keep your deprecation comments to steer folks to the new functions.

[Ryan Beisner (1chb1n)]

Ok, thanks - let's proceed as such.

Just a reminder, the "create or get keypair" method will be movedto this MP once that's reworked from the heat MP.

[lp:~1chb1n/charm-helpers/heat-amulet]

385. By Ryan Beisner on 2015-06-11

Return validate_services to original functionality
Add create_or_get_keypair
Update comments and docstrings

386. By Ryan Beisner on 2015-06-11

Add Wily/Liberty awareness.

387. By Ryan Beisner on 2015-06-11

add file_to_url

[Ryan Beisner (1chb1n)]

Ready for review.

[Corey Bryant (corey.bryant)]

Looks good. 2 really minor inline comments.

review: Approve

[lp:~1chb1n/charm-helpers/heat-amulet]

388. By Ryan Beisner on 2015-06-11

light cleanup and comment correction

=== modified file 'charmhelpers/contrib/amulet/utils.py'
--- charmhelpers/contrib/amulet/utils.py	2015-04-21 15:40:51 +0000
+++ charmhelpers/contrib/amulet/utils.py	2015-06-11 19:47:33 +0000
@@ -15,13 +15,15 @@
 # along with charm-helpers.  If not, see <http://www.gnu.org/licenses/>.
 
 import ConfigParser
+import distro_info
 import io
 import logging
+import os
 import re
+import six
 import sys
 import time
-
-import six
+import urlparse
 
 
 class AmuletUtils(object):
@@ -33,6 +35,7 @@
 
     def __init__(self, log_level=logging.ERROR):
         self.log = self.get_logger(level=log_level)
+        self.ubuntu_releases = self.get_ubuntu_releases()
 
     def get_logger(self, name="amulet-logger", level=logging.DEBUG):
         """Get a logger object that will log to stdout."""
@@ -70,12 +73,44 @@
         else:
             return False
 
+    def get_ubuntu_release_from_sentry(self, sentry_unit):
+        """Get Ubuntu release codename from sentry unit.
+
+        :param sentry_unit: amulet sentry/service unit pointer
+        :returns: list of strings - release codename, failure message
+        """
+        msg = None
+        cmd = 'lsb_release -cs'
+        release, code = sentry_unit.run(cmd)
+        if code == 0:
+            self.log.debug('{} lsb_release: {}'.format(
+                sentry_unit.info['unit_name'], release))
+        else:
+            msg = ('{} `{}` returned {} '
+                   '{}'.format(sentry_unit.info['unit_name'],
+                               cmd, release, code))
+        if release not in self.ubuntu_releases:
+            msg = ("Release ({}) not found in Ubuntu releases "
+                   "({})".format(release, self.ubuntu_releases))
+        return release, msg
+
     def validate_services(self, commands):
-        """Validate services.
-
-           Verify the specified services are running on the corresponding
+        """Validate that lists of commands succeed on service units.  Can be
+           used to verify system services are running on the corresponding
            service units.
-           """
+
+        :param commands: dict with sentry keys and arbitrary command list values
+        :returns: None if successful, Failure string message otherwise
+        """
+        self.log.debug('Checking status of system services...')
+
+        # /!\ DEPRECATION WARNING (beisner):
+        # New and existing tests should be rewritten to use
+        # validate_services_by_name() as it is aware of init systems.
+        self.log.warn('/!\\ DEPRECATION WARNING:  use '
+                      'validate_services_by_name instead of validate_services '
+                      'due to init system differences.')
+
         for k, v in six.iteritems(commands):
             for cmd in v:
                 output, code = k.run(cmd)
@@ -86,6 +121,41 @@
                     return "command `{}` returned {}".format(cmd, str(code))
         return None
 
+    def validate_services_by_name(self, sentry_services):
+        """Validate system service status by service name, automatically
+           detecting init system based on Ubuntu release codename.
+
+        :param sentry_services: dict with sentry keys and svc list values
+        :returns: None if successful, Failure string message otherwise
+        """
+        self.log.debug('Checking status of system services...')
+
+        # Point at which systemd became a thing
+        systemd_switch = self.ubuntu_releases.index('vivid')
+
+        for sentry_unit, services_list in six.iteritems(sentry_services):
+            # Get lsb_release codename from unit
+            release, ret = self.get_ubuntu_release_from_sentry(sentry_unit)
+            if ret:
+                return ret
+
+            for service_name in services_list:
+                if (self.ubuntu_releases.index(release) >= systemd_switch or
+                        service_name == "rabbitmq-server"):
+                    # init is systemd
+                    cmd = 'sudo service {} status'.format(service_name)
+                elif self.ubuntu_releases.index(release) < systemd_switch:
+                    # init is upstart
+                    cmd = 'sudo status {}'.format(service_name)
+
+                output, code = sentry_unit.run(cmd)
+                self.log.debug('{} `{}` returned '
+                               '{}'.format(sentry_unit.info['unit_name'],
+                                           cmd, code))
+                if code != 0:
+                    return "command `{}` returned {}".format(cmd, str(code))
+        return None
+
     def _get_config(self, unit, filename):
         """Get a ConfigParser object for parsing a unit's config file."""
         file_contents = unit.file_contents(filename)
@@ -104,6 +174,9 @@
            Verify that the specified section of the config file contains
            the expected option key:value pairs.
            """
+        self.log.debug('Validating config file data ({} in {} on {})'
+                       '...'.format(section, config_file,
+                                    sentry_unit.info['unit_name']))
         config = self._get_config(sentry_unit, config_file)
 
         if section != 'DEFAULT' and not config.has_section(section):
@@ -321,3 +394,15 @@
 
     def endpoint_error(self, name, data):
         return 'unexpected endpoint data in {} - {}'.format(name, data)
+
+    def get_ubuntu_releases(self):
+        """Return a list of all Ubuntu releases in order of release."""
+        _d = distro_info.UbuntuDistroInfo()
+        _release_list = _d.all
+        self.log.debug('Ubuntu release list: {}'.format(_release_list))
+        return _release_list
+
+    def file_to_url(self, file_rel_path):
+        """Convert a relative file path to a file URL."""
+        _abs_path = os.path.abspath(file_rel_path)
+        return urlparse.urlparse(_abs_path, scheme='file').geturl()
 
=== modified file 'charmhelpers/contrib/openstack/amulet/deployment.py'
--- charmhelpers/contrib/openstack/amulet/deployment.py	2015-04-21 15:40:51 +0000
+++ charmhelpers/contrib/openstack/amulet/deployment.py	2015-06-11 19:47:33 +0000
@@ -110,7 +110,8 @@
         (self.precise_essex, self.precise_folsom, self.precise_grizzly,
          self.precise_havana, self.precise_icehouse,
          self.trusty_icehouse, self.trusty_juno, self.utopic_juno,
-         self.trusty_kilo, self.vivid_kilo) = range(10)
+         self.trusty_kilo, self.vivid_kilo, self.trusty_liberty,
+         self.wily_liberty) = range(12)
 
         releases = {
             ('precise', None): self.precise_essex,
@@ -121,8 +122,10 @@
             ('trusty', None): self.trusty_icehouse,
             ('trusty', 'cloud:trusty-juno'): self.trusty_juno,
             ('trusty', 'cloud:trusty-kilo'): self.trusty_kilo,
+            ('trusty', 'cloud:trusty-liberty'): self.trusty_liberty,
             ('utopic', None): self.utopic_juno,
-            ('vivid', None): self.vivid_kilo}
+            ('vivid', None): self.vivid_kilo,
+            ('wily', None): self.wily_liberty}
         return releases[(self.series, self.openstack)]
 
     def _get_openstack_release_string(self):
@@ -138,6 +141,7 @@
             ('trusty', 'icehouse'),
             ('utopic', 'juno'),
             ('vivid', 'kilo'),
+            ('wily', 'liberty'),
         ])
         if self.openstack:
             os_origin = self.openstack.split(':')[1]
 
=== modified file 'charmhelpers/contrib/openstack/amulet/utils.py'
--- charmhelpers/contrib/openstack/amulet/utils.py	2015-01-22 06:06:03 +0000
+++ charmhelpers/contrib/openstack/amulet/utils.py	2015-06-11 19:47:33 +0000
@@ -16,15 +16,15 @@
 
 import logging
 import os
+import six
 import time
 import urllib
 
 import glanceclient.v1.client as glance_client
+import heatclient.v1.client as heat_client
 import keystoneclient.v2_0 as keystone_client
 import novaclient.v1_1.client as nova_client
 
-import six
-
 from charmhelpers.contrib.amulet.utils import (
     AmuletUtils
 )
@@ -37,7 +37,7 @@
     """OpenStack amulet utilities.
 
        This class inherits from AmuletUtils and has additional support
-       that is specifically for use by OpenStack charms.
+       that is specifically for use by OpenStack charm tests.
        """
 
     def __init__(self, log_level=ERROR):
@@ -51,6 +51,8 @@
            Validate actual endpoint data vs expected endpoint data. The ports
            are used to find the matching endpoint.
            """
+        self.log.debug('Validating endpoint data...')
+        self.log.debug('actual: {}'.format(repr(endpoints)))
         found = False
         for ep in endpoints:
             self.log.debug('endpoint: {}'.format(repr(ep)))
@@ -77,6 +79,7 @@
            Validate a list of actual service catalog endpoints vs a list of
            expected service catalog endpoints.
            """
+        self.log.debug('Validating service catalog endpoint data...')
         self.log.debug('actual: {}'.format(repr(actual)))
         for k, v in six.iteritems(expected):
             if k in actual:
@@ -93,6 +96,7 @@
            Validate a list of actual tenant data vs list of expected tenant
            data.
            """
+        self.log.debug('Validating tenant data...')
         self.log.debug('actual: {}'.format(repr(actual)))
         for e in expected:
             found = False
@@ -114,6 +118,7 @@
            Validate a list of actual role data vs a list of expected role
            data.
            """
+        self.log.debug('Validating role data...')
         self.log.debug('actual: {}'.format(repr(actual)))
         for e in expected:
             found = False
@@ -134,6 +139,7 @@
            Validate a list of actual user data vs a list of expected user
            data.
            """
+        self.log.debug('Validating user data...')
         self.log.debug('actual: {}'.format(repr(actual)))
         for e in expected:
             found = False
@@ -155,17 +161,20 @@
 
            Validate a list of actual flavors vs a list of expected flavors.
            """
+        self.log.debug('Validating flavor data...')
         self.log.debug('actual: {}'.format(repr(actual)))
         act = [a.name for a in actual]
         return self._validate_list_data(expected, act)
 
     def tenant_exists(self, keystone, tenant):
         """Return True if tenant exists."""
+        self.log.debug('Checking if tenant exists ({})...'.format(tenant))
         return tenant in [t.name for t in keystone.tenants.list()]
 
     def authenticate_keystone_admin(self, keystone_sentry, user, password,
                                     tenant):
         """Authenticates admin user with the keystone admin endpoint."""
+        self.log.debug('Authenticating keystone admin...')
         unit = keystone_sentry
         service_ip = unit.relation('shared-db',
                                    'mysql:shared-db')['private-address']
@@ -175,6 +184,7 @@
 
     def authenticate_keystone_user(self, keystone, user, password, tenant):
         """Authenticates a regular user with the keystone public endpoint."""
+        self.log.debug('Authenticating keystone user ({})...'.format(user))
         ep = keystone.service_catalog.url_for(service_type='identity',
                                               endpoint_type='publicURL')
         return keystone_client.Client(username=user, password=password,
@@ -182,12 +192,21 @@
 
     def authenticate_glance_admin(self, keystone):
         """Authenticates admin user with glance."""
+        self.log.debug('Authenticating glance admin...')
         ep = keystone.service_catalog.url_for(service_type='image',
                                               endpoint_type='adminURL')
         return glance_client.Client(ep, token=keystone.auth_token)
 
+    def authenticate_heat_admin(self, keystone):
+        """Authenticates the admin user with heat."""
+        self.log.debug('Authenticating heat admin...')
+        ep = keystone.service_catalog.url_for(service_type='orchestration',
+                                              endpoint_type='publicURL')
+        return heat_client.Client(endpoint=ep, token=keystone.auth_token)
+
     def authenticate_nova_user(self, keystone, user, password, tenant):
         """Authenticates a regular user with nova-api."""
+        self.log.debug('Authenticating nova user ({})...'.format(user))
         ep = keystone.service_catalog.url_for(service_type='identity',
                                               endpoint_type='publicURL')
         return nova_client.Client(username=user, api_key=password,
@@ -195,6 +214,7 @@
 
     def create_cirros_image(self, glance, image_name):
         """Download the latest cirros image and upload it to glance."""
+        self.log.debug('Creating glance image ({})...'.format(image_name))
         http_proxy = os.getenv('AMULET_HTTP_PROXY')
         self.log.debug('AMULET_HTTP_PROXY: {}'.format(http_proxy))
         if http_proxy:
@@ -235,6 +255,11 @@
 
     def delete_image(self, glance, image):
         """Delete the specified image."""
+
+        # /!\ DEPRECATION WARNING
+        self.log.warn('/!\\ DEPRECATION WARNING:  use '
+                      'delete_resource instead of delete_image.')
+        self.log.debug('Deleting glance image ({})...'.format(image))
         num_before = len(list(glance.images.list()))
         glance.images.delete(image)
 
@@ -254,6 +279,8 @@
 
     def create_instance(self, nova, image_name, instance_name, flavor):
         """Create the specified instance."""
+        self.log.debug('Creating instance '
+                       '({}|{}|{})'.format(instance_name, image_name, flavor))
         image = nova.images.find(name=image_name)
         flavor = nova.flavors.find(name=flavor)
         instance = nova.servers.create(name=instance_name, image=image,
@@ -276,6 +303,11 @@
 
     def delete_instance(self, nova, instance):
         """Delete the specified instance."""
+
+        # /!\ DEPRECATION WARNING
+        self.log.warn('/!\\ DEPRECATION WARNING:  use '
+                      'delete_resource instead of delete_instance.')
+        self.log.debug('Deleting instance ({})...'.format(instance))
         num_before = len(list(nova.servers.list()))
         nova.servers.delete(instance)
 
@@ -292,3 +324,90 @@
             return False
 
         return True
+
+    def create_or_get_keypair(self, nova, keypair_name="testkey"):
+        """Create a new keypair, or return pointer if it already exists."""
+        try:
+            _keypair = nova.keypairs.get(keypair_name)
+            self.log.debug('Keypair ({}) already exists, '
+                           'using it.'.format(keypair_name))
+            return _keypair
+        except:
+            self.log.debug('Keypair ({}) does not exist, '
+                           'creating it.'.format(keypair_name))
+
+        _keypair = nova.keypairs.create(name=keypair_name)
+        return _keypair
+
+    def delete_resource(self, resource, resource_id,
+                        msg="resource", max_wait=120):
+        """Delete one openstack resource, such as one instance, keypair,
+        image, volume, stack, etc., and confirm deletion within max wait time.
+
+        :param resource: pointer to os resource type, ex:glance_client.images
+        :param resource_id: unique name or id for the openstack resource
+        :param msg: text to identify purpose in logging
+        :param max_wait: maximum wait time in seconds
+        :returns: True if successful, otherwise False
+        """
+        num_before = len(list(resource.list()))
+        resource.delete(resource_id)
+
+        tries = 0
+        num_after = len(list(resource.list()))
+        while num_after != (num_before - 1) and tries < (max_wait / 4):
+            self.log.debug('{} delete check: '
+                           '{} [{}:{}] {}'.format(msg, tries,
+                                                  num_before,
+                                                  num_after,
+                                                  resource_id))
+            time.sleep(4)
+            num_after = len(list(resource.list()))
+            tries += 1
+
+        self.log.debug('{}:  expected, actual count = {}, '
+                       '{}'.format(msg, num_before - 1, num_after))
+
+        if num_after == (num_before - 1):
+            return True
+        else:
+            self.log.error('{} delete timed out'.format(msg))
+            return False
+
+    def resource_reaches_status(self, resource, resource_id,
+                                expected_stat='available',
+                                msg='resource', max_wait=120):
+        """Wait for an openstack resources status to reach an
+           expected status within a specified time.  Useful to confirm that
+           nova instances, cinder vols, snapshots, glance images, heat stacks
+           and other resources eventually reach the expected status.
+
+        :param resource: pointer to os resource type, ex: heat_client.stacks
+        :param resource_id: unique id for the openstack resource
+        :param expected_stat: status to expect resource to reach
+        :param msg: text to identify purpose in logging
+        :param max_wait: maximum wait time in seconds
+        :returns: True if successful, False if status is not reached
+        """
+
+        tries = 0
+        resource_stat = resource.get(resource_id).status
+        while resource_stat != expected_stat and tries < (max_wait / 4):
+            self.log.debug('{} status check: '
+                           '{} [{}:{}] {}'.format(msg, tries,
+                                                  resource_stat,
+                                                  expected_stat,
+                                                  resource_id))
+            time.sleep(4)
+            resource_stat = resource.get(resource_id).status
+            tries += 1
+
+        self.log.debug('{}:  expected, actual status = {}, '
+                       '{}'.format(msg, resource_stat, expected_stat))
+
+        if resource_stat == expected_stat:
+            return True
+        else:
+            self.log.debug('{} never reached expected status: '
+                           '{}'.format(resource_id, expected_stat))
+            return False
 
=== modified file 'charmhelpers/contrib/openstack/utils.py'
--- charmhelpers/contrib/openstack/utils.py	2015-05-11 18:53:44 +0000
+++ charmhelpers/contrib/openstack/utils.py	2015-06-11 19:47:33 +0000
@@ -79,6 +79,7 @@
     ('trusty', 'icehouse'),
     ('utopic', 'juno'),
     ('vivid', 'kilo'),
+    ('wily', 'liberty'),
 ])
 
 
@@ -91,6 +92,7 @@
     ('2014.1', 'icehouse'),
     ('2014.2', 'juno'),
     ('2015.1', 'kilo'),
+    ('2015.2', 'liberty'),
 ])
 
 # The ugly duckling
@@ -113,6 +115,7 @@
     ('2.2.0', 'juno'),
     ('2.2.1', 'kilo'),
     ('2.2.2', 'kilo'),
+    ('2.3.0', 'liberty'),
 ])
 
 DEFAULT_LOOPBACK_SIZE = '5G'
@@ -321,6 +324,9 @@
             'kilo': 'trusty-updates/kilo',
             'kilo/updates': 'trusty-updates/kilo',
             'kilo/proposed': 'trusty-proposed/kilo',
+            'liberty': 'trusty-updates/liberty',
+            'liberty/updates': 'trusty-updates/liberty',
+            'liberty/proposed': 'trusty-proposed/liberty',
         }
 
         try:
@@ -641,7 +647,8 @@
         subprocess.check_call(cmd)
     except subprocess.CalledProcessError:
         package = os.path.basename(package_dir)
-        error_out("Error updating {} from global-requirements.txt".format(package))
+        error_out("Error updating {} from "
+                  "global-requirements.txt".format(package))
     os.chdir(orig_dir)