Merge lp:~thedac/charm-helpers/dnsha into lp:charm-helpers

[David Ames]

DNS HA Helpers

Start the process of moving HA helpers into the openstack contrib area
Add the helper for API charms to use DNS HA
Add validation for new DNS config options in HA
Allow resolve_address to override with DNS or not

DNS HA Helpers

Start the process of moving HA helpers into the openstack contrib area
Add the helper for API charms to use DNS HA
Add validation for new DNS config options in HA
Allow resolve_address to override with DNS or not

[lp:~thedac/charm-helpers/dnsha]

582. By Ryan Beisner on 2016-06-13

[jamespage,r=1chb1n] Update amulet helpers to deploy charms from the charm store

As part of the migration to git/gerrit and with the introduction of
layered charms, the charm store really needs to be the definative
source of truth for all charms during test.

Switch the charm resolver code to pick the correct charms from the
charm store for amulet tests.

This does change behaviour in that base charms are deployed aligned
to the test series where possible; otherwise the most recent Ubuntu
series is used instead - results in mysql/trusty + keystone/xenial
for example.

583. By James Page on 2016-06-14

Enable default git repo generation if a default openstack-origin-git value is specified.

[Alex Kavanagh (ajkavanagh)]

Just some discussion on status_set() - see inline comments.

[David Ames (thedac)]

Response inline

[David Ames (thedac)]

As per request, Exceptions are now thrown immediately for Incomplete or Incorrect HA configurations to avoid confusion.

[Alex Kavanagh (ajkavanagh)]

Looks good.

review: Approve

[lp:~thedac/charm-helpers/dnsha]

584. By David Ames on 2016-06-14

[thedac, r=tinwood] DNS HA Helpers

Start the process of moving HA helpers into the openstack contrib area
Add the helper for API charms to use DNS HA
Add validation for new DNS config options in HA
Allow resolve_address to override with DNS or not

=== modified file 'charmhelpers/contrib/hahelpers/cluster.py'
--- charmhelpers/contrib/hahelpers/cluster.py	2015-06-05 19:03:02 +0000
+++ charmhelpers/contrib/hahelpers/cluster.py	2016-06-14 16:27:51 +0000
@@ -41,10 +41,11 @@
     relation_get,
     config as config_get,
     INFO,
-    ERROR,
+    DEBUG,
     WARNING,
     unit_get,
-    is_leader as juju_is_leader
+    is_leader as juju_is_leader,
+    status_set,
 )
 from charmhelpers.core.decorators import (
     retry_on_exception,
@@ -60,6 +61,10 @@
     pass
 
 
+class HAIncorrectConfig(Exception):
+    pass
+
+
 class CRMResourceNotFound(Exception):
     pass
 
@@ -274,27 +279,71 @@
     Obtains all relevant configuration from charm configuration required
     for initiating a relation to hacluster:
 
-        ha-bindiface, ha-mcastport, vip
+        ha-bindiface, ha-mcastport, vip, os-internal-hostname,
+        os-admin-hostname, os-public-hostname
 
     param: exclude_keys: list of setting key(s) to be excluded.
     returns: dict: A dict containing settings keyed by setting name.
-    raises: HAIncompleteConfig if settings are missing.
+    raises: HAIncompleteConfig if settings are missing or incorrect.
     '''
-    settings = ['ha-bindiface', 'ha-mcastport', 'vip']
+    settings = ['ha-bindiface', 'ha-mcastport', 'vip', 'os-internal-hostname',
+                'os-admin-hostname', 'os-public-hostname']
     conf = {}
     for setting in settings:
         if exclude_keys and setting in exclude_keys:
             continue
 
         conf[setting] = config_get(setting)
-    missing = []
-    [missing.append(s) for s, v in six.iteritems(conf) if v is None]
-    if missing:
-        log('Insufficient config data to configure hacluster.', level=ERROR)
-        raise HAIncompleteConfig
+
+    if not valid_hacluster_config():
+        raise HAIncorrectConfig('Insufficient or incorrect config data to '
+                                'configure hacluster.')
     return conf
 
 
+def valid_hacluster_config():
+    '''
+    Check that either vip or dns-ha is set. If dns-ha then one of os-*-hostname
+    must be set.
+
+    Note: ha-bindiface and ha-macastport both have defaults and will always
+    be set. We only care that either vip or dns-ha is set.
+
+    :returns: boolean: valid config returns true.
+    raises: HAIncompatibileConfig if settings conflict.
+    raises: HAIncompleteConfig if settings are missing.
+    '''
+    vip = config_get('vip')
+    dns = config_get('dns-ha')
+    if not(bool(vip) ^ bool(dns)):
+        msg = ('HA: Either vip or dns-ha must be set but not both in order to '
+               'use high availability')
+        status_set('blocked', msg)
+        raise HAIncorrectConfig(msg)
+
+    # If dns-ha then one of os-*-hostname must be set
+    if dns:
+        dns_settings = ['os-internal-hostname', 'os-admin-hostname',
+                        'os-public-hostname']
+        # At this point it is unknown if one or all of the possible
+        # network spaces are in HA. Validate at least one is set which is
+        # the minimum required.
+        for setting in dns_settings:
+            if config_get(setting):
+                log('DNS HA: At least one hostname is set {}: {}'
+                    ''.format(setting, config_get(setting)),
+                    level=DEBUG)
+                return True
+
+        msg = ('DNS HA: At least one os-*-hostname(s) must be set to use '
+               'DNS HA')
+        status_set('blocked', msg)
+        raise HAIncompleteConfig(msg)
+
+    log('VIP HA: VIP is set {}'.format(vip), level=DEBUG)
+    return True
+
+
 def canonical_url(configs, vip_setting='vip'):
     '''
     Returns the correct HTTP URL to this host given the state of HTTPS

=== added directory 'charmhelpers/contrib/openstack/ha'
=== added file 'charmhelpers/contrib/openstack/ha/__init__.py'
=== added file 'charmhelpers/contrib/openstack/ha/utils.py'
--- charmhelpers/contrib/openstack/ha/utils.py	1970-01-01 00:00:00 +0000
+++ charmhelpers/contrib/openstack/ha/utils.py	2016-06-14 16:27:51 +0000
@@ -0,0 +1,111 @@
+# Copyright 2014-2016 Canonical Limited.
+#
+# This file is part of charm-helpers.
+#
+# charm-helpers is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# charm-helpers is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with charm-helpers.  If not, see <http://www.gnu.org/licenses/>.
+
+#
+# Copyright 2016 Canonical Ltd.
+#
+# Authors:
+#  Openstack Charmers <
+#
+
+"""
+Helpers for high availability.
+"""
+
+import re
+
+from charmhelpers.core.hookenv import (
+    log,
+    relation_set,
+    charm_name,
+    config,
+    status_set,
+    DEBUG,
+)
+
+from charmhelpers.contrib.openstack.ip import (
+    resolve_address,
+)
+
+
+class DNSHAException(Exception):
+    """Raised when an error occurs setting up DNS HA
+    """
+
+    pass
+
+
+def update_dns_ha_resource_params(resources, resource_params,
+                                  relation_id=None,
+                                  crm_ocf='ocf:maas:dns'):
+    """ Check for os-*-hostname settings and update resource dictionaries for
+    the HA relation.
+
+    @param resources: Pointer to dictionary of resources.
+                      Usually instantiated in ha_joined().
+    @param resource_params: Pointer to dictionary of resource parameters.
+                            Usually instantiated in ha_joined()
+    @param relation_id: Relation ID of the ha relation
+    @param crm_ocf: Corosync Open Cluster Framework resource agent to use for
+                    DNS HA
+    """
+
+    settings = ['os-admin-hostname', 'os-internal-hostname',
+                'os-public-hostname']
+
+    # Check which DNS settings are set and update dictionaries
+    hostname_group = []
+    for setting in settings:
+        hostname = config(setting)
+        if hostname is None:
+            log('DNS HA: Hostname setting {} is None. Ignoring.'
+                ''.format(setting),
+                DEBUG)
+            continue
+        m = re.search('os-(.+?)-hostname', setting)
+        if m:
+            networkspace = m.group(1)
+        else:
+            msg = ('Unexpected DNS hostname setting: {}. '
+                   'Cannot determine network space name'
+                   ''.format(setting))
+            status_set('blocked', msg)
+            raise DNSHAException(msg)
+
+        hostname_key = 'res_{}_{}_hostname'.format(charm_name(), networkspace)
+        if hostname_key in hostname_group:
+            log('DNS HA: Resource {}: {} already exists in '
+                'hostname group - skipping'.format(hostname_key, hostname),
+                DEBUG)
+            continue
+
+        hostname_group.append(hostname_key)
+        resources[hostname_key] = crm_ocf
+        resource_params[hostname_key] = (
+            'params fqdn="{}" ip_address="{}" '
+            ''.format(hostname, resolve_address(endpoint_type=networkspace,
+                                                override=False)))
+
+    if len(hostname_group) >= 1:
+        log('DNS HA: Hostname group is set with {} as members. '
+            'Informing the ha relation'.format(' '.join(hostname_group)),
+            DEBUG)
+        relation_set(relation_id=relation_id, groups={
+            'grp_{}_hostnames'.format(charm_name()): ' '.join(hostname_group)})
+    else:
+        msg = 'DNS HA: Hostname group has no members.'
+        status_set('blocked', msg)
+        raise DNSHAException(msg)

=== modified file 'charmhelpers/contrib/openstack/ip.py'
--- charmhelpers/contrib/openstack/ip.py	2016-03-21 16:34:40 +0000
+++ charmhelpers/contrib/openstack/ip.py	2016-06-14 16:27:51 +0000
@@ -109,7 +109,7 @@
         return addr_override.format(service_name=service_name())
 
 
-def resolve_address(endpoint_type=PUBLIC):
+def resolve_address(endpoint_type=PUBLIC, override=True):
     """Return unit address depending on net config.
 
     If unit is clustered with vip(s) and has net splits defined, return vip on
@@ -119,10 +119,13 @@
     split if one is configured, or a Juju 2.0 extra-binding has been used.
 
     :param endpoint_type: Network endpoing type
+    :param override: Accept hostname overrides or not
     """
-    resolved_address = _get_address_override(endpoint_type)
-    if resolved_address:
-        return resolved_address
+    resolved_address = None
+    if override:
+        resolved_address = _get_address_override(endpoint_type)
+        if resolved_address:
+            return resolved_address
 
     vips = config('vip')
     if vips:

=== modified file 'tests/contrib/hahelpers/test_cluster_utils.py'
--- tests/contrib/hahelpers/test_cluster_utils.py	2015-06-05 19:03:02 +0000
+++ tests/contrib/hahelpers/test_cluster_utils.py	2016-06-14 16:27:51 +0000
@@ -318,47 +318,62 @@
         port = cluster_utils.determine_apache_port(9696, singlenode_mode=True)
         self.assertEquals(9686, port)
 
-    def test_get_hacluster_config_complete(self):
+    @patch.object(cluster_utils, 'valid_hacluster_config')
+    def test_get_hacluster_config_complete(self, valid_hacluster_config):
         '''It fetches all hacluster charm config'''
         conf = {
             'ha-bindiface': 'eth1',
             'ha-mcastport': '3333',
             'vip': '10.0.0.1',
+            'os-admin-hostname': None,
+            'os-public-hostname': None,
+            'os-internal-hostname': None,
         }
 
+        valid_hacluster_config.return_value = True
+
         def _fake_config_get(setting):
             return conf[setting]
 
         self.config_get.side_effect = _fake_config_get
         self.assertEquals(conf, cluster_utils.get_hacluster_config())
 
-    def test_get_hacluster_config_incomplete(self):
+    @patch.object(cluster_utils, 'valid_hacluster_config')
+    def test_get_hacluster_config_incomplete(self, valid_hacluster_config):
         '''It raises exception if some hacluster charm config missing'''
         conf = {
             'ha-bindiface': 'eth1',
             'ha-mcastport': '3333',
             'vip': None,
+            'os-admin-hostname': None,
+            'os-public-hostname': None,
+            'os-internal-hostname': None,
         }
 
+        valid_hacluster_config.return_value = False
+
         def _fake_config_get(setting):
             return conf[setting]
 
         self.config_get.side_effect = _fake_config_get
-        self.assertRaises(cluster_utils.HAIncompleteConfig,
+        self.assertRaises(cluster_utils.HAIncorrectConfig,
                           cluster_utils.get_hacluster_config)
 
-    def test_get_hacluster_config_with_excludes(self):
+    @patch.object(cluster_utils, 'valid_hacluster_config')
+    def test_get_hacluster_config_with_excludes(self, valid_hacluster_config):
         '''It fetches all hacluster charm config'''
         conf = {
             'ha-bindiface': 'eth1',
             'ha-mcastport': '3333',
         }
+        valid_hacluster_config.return_value = True
 
         def _fake_config_get(setting):
             return conf[setting]
 
         self.config_get.side_effect = _fake_config_get
-        exclude_keys = ['vip']
+        exclude_keys = ['vip', 'os-admin-hostname', 'os-internal-hostname',
+                        'os-public-hostname']
         result = cluster_utils.get_hacluster_config(exclude_keys)
         self.assertEquals(conf, result)
 
@@ -406,3 +421,93 @@
         configs.complete_contexts.return_value = []
         url = cluster_utils.canonical_url(configs)
         self.assertEquals('http://10.0.0.1', url)
+
+    @patch.object(cluster_utils, 'status_set')
+    def test_valid_hacluster_config_incomplete(self, status_set):
+        '''Returns False with incomplete HA config'''
+        conf = {
+            'vip': None,
+            'os-admin-hostname': None,
+            'os-public-hostname': None,
+            'os-internal-hostname': None,
+            'dns-ha': False,
+        }
+
+        def _fake_config_get(setting):
+            return conf[setting]
+
+        self.config_get.side_effect = _fake_config_get
+        self.assertRaises(cluster_utils.HAIncorrectConfig,
+                          cluster_utils.valid_hacluster_config)
+
+    @patch.object(cluster_utils, 'status_set')
+    def test_valid_hacluster_config_both(self, status_set):
+        '''Returns False when both VIP and DNS HA are set'''
+        conf = {
+            'vip': '10.0.0.1',
+            'os-admin-hostname': None,
+            'os-public-hostname': None,
+            'os-internal-hostname': None,
+            'dns-ha': True,
+        }
+
+        def _fake_config_get(setting):
+            return conf[setting]
+
+        self.config_get.side_effect = _fake_config_get
+        self.assertRaises(cluster_utils.HAIncorrectConfig,
+                          cluster_utils.valid_hacluster_config)
+
+    @patch.object(cluster_utils, 'status_set')
+    def test_valid_hacluster_config_vip_ha(self, status_set):
+        '''Returns True with complete VIP HA config'''
+        conf = {
+            'vip': '10.0.0.1',
+            'os-admin-hostname': None,
+            'os-public-hostname': None,
+            'os-internal-hostname': None,
+            'dns-ha': False,
+        }
+
+        def _fake_config_get(setting):
+            return conf[setting]
+
+        self.config_get.side_effect = _fake_config_get
+        self.assertTrue(cluster_utils.valid_hacluster_config())
+        self.assertFalse(status_set.called)
+
+    @patch.object(cluster_utils, 'status_set')
+    def test_valid_hacluster_config_dns_incomplete(self, status_set):
+        '''Returns False with incomplete DNS HA config'''
+        conf = {
+            'vip': None,
+            'os-admin-hostname': None,
+            'os-public-hostname': None,
+            'os-internal-hostname': None,
+            'dns-ha': True,
+        }
+
+        def _fake_config_get(setting):
+            return conf[setting]
+
+        self.config_get.side_effect = _fake_config_get
+        self.assertRaises(cluster_utils.HAIncompleteConfig,
+                          cluster_utils.valid_hacluster_config)
+
+    @patch.object(cluster_utils, 'status_set')
+    def test_valid_hacluster_config_dns_ha(self, status_set):
+        '''Returns True with complete DNS HA config'''
+        conf = {
+            'vip': None,
+            'os-admin-hostname': 'somehostname',
+            'os-public-hostname': None,
+            'os-internal-hostname': None,
+            'dns-ha': True,
+        }
+
+        def _fake_config_get(setting):
+            return conf[setting]
+
+        self.config_get.side_effect = _fake_config_get
+        self.assertTrue(cluster_utils.valid_hacluster_config())
+        self.assertFalse(status_set.called)

=== added directory 'tests/contrib/openstack/ha'
=== added file 'tests/contrib/openstack/ha/__init__.py'
=== added file 'tests/contrib/openstack/ha/test_ha_utils.py'
--- tests/contrib/openstack/ha/test_ha_utils.py	1970-01-01 00:00:00 +0000
+++ tests/contrib/openstack/ha/test_ha_utils.py	2016-06-14 16:27:51 +0000
@@ -0,0 +1,98 @@
+from mock import patch
+import unittest
+
+from charmhelpers.contrib.openstack.ha import utils as ha
+
+
+class HATests(unittest.TestCase):
+    def setUp(self):
+        super(HATests, self).setUp()
+        [self._patch(m) for m in [
+            'charm_name',
+            'config',
+            'relation_set',
+            'resolve_address',
+            'status_set',
+        ]]
+        self.resources = {'res_test_haproxy': 'lsb:haproxy'}
+        self.resource_params = {'res_test_haproxy': 'op monitor interval="5s"'}
+        self.conf = {}
+        self.config.side_effect = lambda key: self.conf.get(key)
+
+    def _patch(self, method):
+        _m = patch.object(ha, method)
+        mock = _m.start()
+        self.addCleanup(_m.stop)
+        setattr(self, method, mock)
+
+    def test_update_dns_ha_resource_params_none(self):
+        self.conf = {
+            'os-admin-hostname': None,
+            'os-internal-hostname': None,
+            'os-public-hostname': None,
+        }
+
+        with self.assertRaises(ha.DNSHAException):
+            ha.update_dns_ha_resource_params(
+                relation_id='ha:1',
+                resources=self.resources,
+                resource_params=self.resource_params)
+
+    def test_update_dns_ha_resource_params_one(self):
+        EXPECTED_RESOURCES = {'res_test_public_hostname': 'ocf:maas:dns',
+                              'res_test_haproxy': 'lsb:haproxy'}
+        EXPECTED_RESOURCE_PARAMS = {
+            'res_test_public_hostname': ('params fqdn="test.maas" '
+                                         'ip_address="10.0.0.1" '),
+            'res_test_haproxy': 'op monitor interval="5s"'}
+
+        self.conf = {
+            'os-admin-hostname': None,
+            'os-internal-hostname': None,
+            'os-public-hostname': 'test.maas',
+        }
+
+        self.charm_name.return_value = 'test'
+        self.resolve_address.return_value = '10.0.0.1'
+        ha.update_dns_ha_resource_params(relation_id='ha:1',
+                                         resources=self.resources,
+                                         resource_params=self.resource_params)
+        self.assertEqual(self.resources, EXPECTED_RESOURCES)
+        self.assertEqual(self.resource_params, EXPECTED_RESOURCE_PARAMS)
+        self.relation_set.assert_called_with(
+            groups={'grp_test_hostnames': 'res_test_public_hostname'},
+            relation_id='ha:1')
+
+    def test_update_dns_ha_resource_params_all(self):
+        EXPECTED_RESOURCES = {'res_test_admin_hostname': 'ocf:maas:dns',
+                              'res_test_internal_hostname': 'ocf:maas:dns',
+                              'res_test_public_hostname': 'ocf:maas:dns',
+                              'res_test_haproxy': 'lsb:haproxy'}
+        EXPECTED_RESOURCE_PARAMS = {
+            'res_test_admin_hostname': ('params fqdn="test.admin.maas" '
+                                        'ip_address="10.0.0.1" '),
+            'res_test_internal_hostname': ('params fqdn="test.internal.maas" '
+                                           'ip_address="10.0.0.1" '),
+            'res_test_public_hostname': ('params fqdn="test.public.maas" '
+                                         'ip_address="10.0.0.1" '),
+            'res_test_haproxy': 'op monitor interval="5s"'}
+
+        self.conf = {
+            'os-admin-hostname': 'test.admin.maas',
+            'os-internal-hostname': 'test.internal.maas',
+            'os-public-hostname': 'test.public.maas',
+        }
+
+        self.charm_name.return_value = 'test'
+        self.resolve_address.return_value = '10.0.0.1'
+        ha.update_dns_ha_resource_params(relation_id='ha:1',
+                                         resources=self.resources,
+                                         resource_params=self.resource_params)
+        self.assertEqual(self.resources, EXPECTED_RESOURCES)
+        self.assertEqual(self.resource_params, EXPECTED_RESOURCE_PARAMS)
+        self.relation_set.assert_called_with(
+            groups={'grp_test_hostnames':
+                    ('res_test_admin_hostname '
+                     'res_test_internal_hostname '
+                     'res_test_public_hostname')},
+            relation_id='ha:1')

=== modified file 'tests/contrib/openstack/test_ip.py'
--- tests/contrib/openstack/test_ip.py	2016-03-30 21:04:15 +0000
+++ tests/contrib/openstack/test_ip.py	2016-06-14 16:27:51 +0000
@@ -101,6 +101,14 @@
         addr = ip.resolve_address()
         self.assertEqual('public.example.com', addr)
 
+    @patch.object(ip, '_get_address_override')
+    def test_resolve_address_no_override(self, _get_address_override):
+        self.test_config.set('os-public-hostname', 'public.example.com')
+        self.unit_get.return_value = '10.0.0.1'
+        addr = ip.resolve_address(override=False)
+        self.assertFalse(_get_address_override.called)
+        self.assertEqual('10.0.0.1', addr)
+
     def test_resolve_address_override_template(self):
         self.test_config.set('os-public-hostname',
                              '{service_name}.example.com')