Merge lp:~bjornt/landscape-charm/package-upload-leader-change into lp:~landscape/landscape-charm/trunk

[Björn Tillenius]

Don't make haproxy break when the landscape-server leader is removed.

If you would have multiple landscape-server units and the leader would
go away, the haproxy config would break. The reason was the
package-upload config was always present in the haproxy config, but the
actual backend was only configured by the leader.

I changed it so that the leader not only adds the backend, but also adds
the package-upload config that references the backend.

I had to make use of the is-leader executable, since the leader related
APIs in charmhelpers is a bit buggy. I added an is_elected_leader
function with the intention that this will be ported to charmhelpers, so
I made it accept the same parameters as the existing is_elected_leader
function.

[lp:~bjornt/landscape-charm/package-upload-leader-change]

303. By Björn Tillenius on 2015-06-03

Merge lp:~free.ekanayaka/landscape-charm/new-data-provider-flow, resolve conflicts..

304. By Björn Tillenius on 2015-06-03

Use is_leader() from charmhelpers.

305. By Björn Tillenius on 2015-06-03

Remove custom is_leader function.

306. By Björn Tillenius on 2015-06-03

Remove ClusterStub.

307. By Björn Tillenius on 2015-06-04

Merge trunk.

308. By Björn Tillenius on 2015-06-04

Add comment.

309. By Björn Tillenius on 2015-06-04

Remove is_leader parameter.

310. By Björn Tillenius on 2015-06-04

Remove unused method.

Unmerged revisions

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2015-05-19 09:33:01 +0000
+++ charmhelpers/core/hookenv.py	2015-06-03 12:30:44 +0000
@@ -364,11 +364,16 @@
     relation_settings = relation_settings if relation_settings else {}
     relation_cmd_line = ['relation-set']
     accepts_file = "--file" in subprocess.check_output(
-        relation_cmd_line + ["--help"])
+        relation_cmd_line + ["--help"], universal_newlines=True)
     if relation_id is not None:
         relation_cmd_line.extend(('-r', relation_id))
     settings = relation_settings.copy()
     settings.update(kwargs)
+    for key, value in settings.items():
+        # Force value to be a string: it always should, but some call
+        # sites pass in things like dicts or numbers.
+        if value is not None:
+            settings[key] = "{}".format(value)
     if accepts_file:
         # --file was introduced in Juju 1.23.2. Use it by default if
         # available, since otherwise we'll break if the relation data is
@@ -390,6 +395,17 @@
     flush(local_unit())
 
 
+def relation_clear(r_id=None):
+    ''' Clears any relation data already set on relation r_id '''
+    settings = relation_get(rid=r_id,
+                            unit=local_unit())
+    for setting in settings:
+        if setting not in ['public-address', 'private-address']:
+            settings[setting] = None
+    relation_set(relation_id=r_id,
+                 **settings)
+
+
 @cached
 def relation_ids(reltype=None):
     """A list of relation_ids"""
@@ -681,3 +697,48 @@
             return 'unknown'
         else:
             raise
+
+
+def translate_exc(from_exc, to_exc):
+    def inner_translate_exc1(f):
+        def inner_translate_exc2(*args, **kwargs):
+            try:
+                return f(*args, **kwargs)
+            except from_exc:
+                raise to_exc
+
+        return inner_translate_exc2
+
+    return inner_translate_exc1
+
+
+@translate_exc(from_exc=OSError, to_exc=NotImplementedError)
+def is_leader():
+    """Does the current unit hold the juju leadership
+
+    Uses juju to determine whether the current unit is the leader of its peers
+    """
+    cmd = ['is-leader', '--format=json']
+    return json.loads(subprocess.check_output(cmd).decode('UTF-8'))
+
+
+@translate_exc(from_exc=OSError, to_exc=NotImplementedError)
+def leader_get(attribute=None):
+    """Juju leader get value(s)"""
+    cmd = ['leader-get', '--format=json'] + [attribute or '-']
+    return json.loads(subprocess.check_output(cmd).decode('UTF-8'))
+
+
+@translate_exc(from_exc=OSError, to_exc=NotImplementedError)
+def leader_set(settings=None, **kwargs):
+    """Juju leader set value(s)"""
+    log("Juju leader-set '%s'" % (settings), level=DEBUG)
+    cmd = ['leader-set']
+    settings = settings or {}
+    settings.update(kwargs)
+    for k, v in settings.iteritems():
+        if v is None:
+            cmd.append('{}='.format(k))
+        else:
+            cmd.append('{}={}'.format(k, v))
+    subprocess.check_call(cmd)

=== modified file 'charmhelpers/core/services/base.py'
--- charmhelpers/core/services/base.py	2015-05-12 14:31:50 +0000
+++ charmhelpers/core/services/base.py	2015-06-03 12:30:44 +0000
@@ -15,8 +15,8 @@
 # along with charm-helpers.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import re
 import json
+from inspect import getargspec
 from collections import Iterable, OrderedDict
 
 from charmhelpers.core import host
@@ -132,8 +132,8 @@
         if hook_name == 'stop':
             self.stop_services()
         else:
+            self.reconfigure_services()
             self.provide_data()
-            self.reconfigure_services()
         cfg = hookenv.config()
         if cfg.implicit_save:
             cfg.save()
@@ -145,15 +145,36 @@
         A provider must have a `name` attribute, which indicates which relation
         to set data on, and a `provide_data()` method, which returns a dict of
         data to set.
+
+        The `provide_data()` method can optionally accept two parameters:
+
+          * ``remote_service`` The name of the remote service that the data will
+            be provided to.  The `provide_data()` method will be called once
+            for each connected service (not unit).  This allows the method to
+            tailor its data to the given service.
+          * ``service_ready`` Whether or not the service definition had all of
+            its requirements met, and thus the ``data_ready`` callbacks run.
+
+        Note that the ``provided_data`` methods are now called **after** the
+        ``data_ready`` callbacks are run.  This gives the ``data_ready`` callbacks
+        a chance to generate any data necessary for the providing to the remote
+        services.
         """
-        hook_name = hookenv.hook_name()
-        for service in self.services.values():
+        for service_name, service in self.services.items():
+            service_ready = self.is_ready(service_name)
             for provider in service.get('provided_data', []):
-                if re.match(r'{}-relation-(joined|changed)'.format(provider.name), hook_name):
-                    data = provider.provide_data()
-                    _ready = provider._is_ready(data) if hasattr(provider, '_is_ready') else data
-                    if _ready:
-                        hookenv.relation_set(None, data)
+                for relid in hookenv.relation_ids(provider.name):
+                    units = hookenv.related_units(relid)
+                    if not units:
+                        continue
+                    remote_service = units[0].split('/')[0]
+                    argspec = getargspec(provider.provide_data)
+                    if len(argspec.args) > 1:
+                        data = provider.provide_data(remote_service, service_ready)
+                    else:
+                        data = provider.provide_data()
+                    if data:
+                        hookenv.relation_set(relid, data)
 
     def reconfigure_services(self, *service_names):
         """

=== modified file 'charmhelpers/fetch/giturl.py'
--- charmhelpers/fetch/giturl.py	2015-03-12 11:42:26 +0000
+++ charmhelpers/fetch/giturl.py	2015-06-03 12:30:44 +0000
@@ -45,14 +45,16 @@
         else:
             return True
 
-    def clone(self, source, dest, branch):
+    def clone(self, source, dest, branch, depth=None):
         if not self.can_handle(source):
             raise UnhandledSource("Cannot handle {}".format(source))
 
-        repo = Repo.clone_from(source, dest)
-        repo.git.checkout(branch)
+        if depth:
+            Repo.clone_from(source, dest, branch=branch, depth=depth)
+        else:
+            Repo.clone_from(source, dest, branch=branch)
 
-    def install(self, source, branch="master", dest=None):
+    def install(self, source, branch="master", dest=None, depth=None):
         url_parts = self.parse_url(source)
         branch_name = url_parts.path.strip("/").split("/")[-1]
         if dest:
@@ -63,7 +65,7 @@
         if not os.path.exists(dest_dir):
             mkdir(dest_dir, perms=0o755)
         try:
-            self.clone(source, dest_dir, branch)
+            self.clone(source, dest_dir, branch, depth)
         except GitCommandError as e:
             raise UnhandledSource(e.message)
         except OSError as e:

=== added file 'hooks/leader-elected'
--- hooks/leader-elected	1970-01-01 00:00:00 +0000
+++ hooks/leader-elected	2015-06-03 12:30:44 +0000
@@ -0,0 +1,9 @@
+#!/usr/bin/python
+import sys
+
+from lib.services import ServicesHook
+
+
+if __name__ == "__main__":
+    hook = ServicesHook()
+    sys.exit(hook())

=== modified file 'lib/relations/haproxy.py'
--- lib/relations/haproxy.py	2015-05-25 09:11:59 +0000
+++ lib/relations/haproxy.py	2015-06-03 12:30:44 +0000
@@ -37,10 +37,8 @@
         "http-request set-header X-Forwarded-Proto https",
         "acl message path_beg -i /message-system",
         "acl api path_beg -i /api",
-        "acl package-upload path_beg -i /upload",
         "use_backend landscape-message if message",
         "use_backend landscape-api if api",
-        "use_backend landscape-package-upload if package-upload",
     ],
 }
 SERVER_BASE_PORTS = {
@@ -100,6 +98,7 @@
     def _get_https(self):
         """Return the service configuration for the HTTPS frontend."""
 
+        service = self._get_service("https")
         backends = [
             self._get_backend("message", self._get_servers("message-server")),
             self._get_backend("api", self._get_servers("api")),
@@ -108,6 +107,9 @@
             self._hookenv.log(
                 "This unit is the juju leader: Writing package-upload backends"
                 " entry.")
+            service["service_options"].extend([
+                "acl package-upload path_beg -i /upload",
+                "use_backend landscape-package-upload if package-upload"])
             backends.append(
                 self._get_backend(
                     "package-upload", self._get_servers("package-upload")))
@@ -116,7 +118,6 @@
                 "This unit is not the juju leader: not writing package-upload"
                 " backends entry.")
 
-        service = self._get_service("https")
         service.update({
             "crts": self._get_ssl_certificate(),
             "servers": self._get_servers("appserver"),
@@ -136,7 +137,7 @@
             "service_name": "landscape-%s" % name,
             "service_host": "0.0.0.0",
             "service_port": SERVICE_PORTS[name],
-            "service_options": SERVICE_OPTIONS[name],
+            "service_options": SERVICE_OPTIONS[name][:],
             "errorfiles": self._get_error_files()
         }
 

=== modified file 'lib/relations/tests/test_haproxy.py'
--- lib/relations/tests/test_haproxy.py	2015-06-02 09:21:44 +0000
+++ lib/relations/tests/test_haproxy.py	2015-06-03 12:30:44 +0000
@@ -96,10 +96,8 @@
                  "http-request set-header X-Forwarded-Proto https",
                  "acl message path_beg -i /message-system",
                  "acl api path_beg -i /api",
-                 "acl package-upload path_beg -i /upload",
                  "use_backend landscape-message if message",
-                 "use_backend landscape-api if api",
-                 "use_backend landscape-package-upload if package-upload"],
+                 "use_backend landscape-api if api"],
              "errorfiles": expected_errorfiles,
              "crts": expected_certs,
              "servers": [
@@ -162,6 +160,66 @@
 
         self.assertRaises(HookError, provider.provide_data)
 
+    def test_provide_data_package_upload_leader(self):
+        """
+        If the unit is a leader, package-upload config is provided for
+        the https service, but not for http.
+        """
+        relation = HAProxyProvider(
+            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths, is_leader=True)
+
+        data = relation.provide_data()
+
+        [http, https] = yaml.safe_load(data["services"])
+        self.assertNotIn(
+            "acl package-upload path_beg -i /upload",
+            http["service_options"])
+        self.assertNotIn(
+            "use_backend landscape-package-upload if package-upload",
+            http["service_options"])
+        self.assertNotIn(
+            "landscape-package-upload",
+            [backend["backend_name"] for backend in http["backends"]])
+        self.assertIn(
+            "acl package-upload path_beg -i /upload",
+            https["service_options"])
+        self.assertIn(
+            "use_backend landscape-package-upload if package-upload",
+            https["service_options"])
+        self.assertIn(
+            "landscape-package-upload",
+            [backend["backend_name"] for backend in https["backends"]])
+
+    def test_provide_data_package_upload_no_leader(self):
+        """
+        If the unit is not a leader, package-upload config isn't
+        provided for neither the https nor http services.
+        """
+        relation = HAProxyProvider(
+            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths, is_leader=False)
+
+        data = relation.provide_data()
+
+        [http, https] = yaml.safe_load(data["services"])
+        self.assertNotIn(
+            "acl package-upload path_beg -i /upload",
+            http["service_options"])
+        self.assertNotIn(
+            "use_backend landscape-package-upload if package-upload",
+            http["service_options"])
+        self.assertNotIn(
+            "landscape-package-upload",
+            [backend["backend_name"] for backend in http["backends"]])
+        self.assertNotIn(
+            "acl package-upload path_beg -i /upload",
+            https["service_options"])
+        self.assertNotIn(
+            "use_backend landscape-package-upload if package-upload",
+            https["service_options"])
+        self.assertNotIn(
+            "landscape-package-upload",
+            [backend["backend_name"] for backend in https["backends"]])
+
     def test_default_ssl_cert_is_used_without_config_keys(self):
         """
         If no "ssl-cert" is specified, the provide_data method returns

=== modified file 'lib/services.py'
--- lib/services.py	2015-06-02 09:21:44 +0000
+++ lib/services.py	2015-06-03 12:30:44 +0000
@@ -5,7 +5,6 @@
 from charmhelpers.core import host
 from charmhelpers.core.services.base import ServiceManager
 from charmhelpers.core.services.helpers import render_template
-from charmhelpers.contrib.hahelpers import cluster
 
 from lib.hook import Hook
 from lib.paths import default_paths
@@ -36,11 +35,10 @@
     all relation data we need in order to configure this Landscape unit, and
     proceed with the configuration if ready.
     """
-    def __init__(self, hookenv=hookenv, cluster=cluster, host=host,
+    def __init__(self, hookenv=hookenv, host=host,
                  subprocess=subprocess, paths=default_paths, fetch=fetch):
         super(ServicesHook, self).__init__(hookenv=hookenv)
         self._hookenv = hookenv
-        self._cluster = cluster
         self._host = host
         self._paths = paths
         self._subprocess = subprocess
@@ -48,20 +46,18 @@
 
     def _run(self):
         leader_context = None
-        is_leader = self._cluster.is_elected_leader(None)
+        is_leader = self._hookenv.is_leader()
         if is_leader:
             leader_context = LandscapeLeaderContext(
                 host=self._host, hookenv=self._hookenv)
 
-        haproxy_provider = HAProxyProvider(
-            SERVICE_COUNTS, paths=self._paths, is_leader=is_leader)
-
         manager = ServiceManager(services=[{
             "service": "landscape",
             "ports": [],
             "provided_data": [
                 LandscapeProvider(leader_context),
-                haproxy_provider,
+                HAProxyProvider(
+                    SERVICE_COUNTS, paths=self._paths, is_leader=is_leader),
                 RabbitMQProvider(),
             ],
             # Required data is available to the render_template calls below.
@@ -94,16 +90,11 @@
             "start": LSCtl(subprocess=self._subprocess, hookenv=self._hookenv),
         }])
 
-        # XXX The services framework only triggers data providers within the
-        #     context of relation joined/changed hooks, however we also
-        #     want to trigger the haproxy provider if the SSL certificate
-        #     has changed.
-        if self._hookenv.hook_name() == "config-changed":
-            config = self._hookenv.config()
-            if config.changed("ssl-cert") or config.changed("ssl-key"):
-                relation_ids = self._hookenv.relation_ids(HAProxyProvider.name)
-                data = haproxy_provider.provide_data()
-                for relation_id in relation_ids:
-                    self._hookenv.relation_set(relation_id, data)
-
         manager.manage()
+
+    def _set_haproxy_data(self, haproxy_provider):
+        """Provide and set the data in the haproxy relation."""
+        relation_ids = self._hookenv.relation_ids(HAProxyProvider.name)
+        data = haproxy_provider.provide_data()
+        for relation_id in relation_ids:
+            self._hookenv.relation_set(relation_id, data)

=== modified file 'lib/tests/stubs.py'
--- lib/tests/stubs.py	2015-06-02 07:06:30 +0000
+++ lib/tests/stubs.py	2015-06-03 12:30:44 +0000
@@ -10,6 +10,7 @@
     hook = "some-hook"
     unit = "landscape-server/0"
     relid = None
+    leader = True
 
     def __init__(self, charm_dir):
         self.messages = []
@@ -68,6 +69,9 @@
     def action_fail(self, message):
         self._action_fails.append(message)
 
+    def is_leader(self):
+        return self.leader
+
 
 class FetchStub(object):
     """Provide a testable stub for C{charmhelpers.fetch}."""
@@ -92,15 +96,6 @@
         self.installed.append((packages, options, fatal))
 
 
-class ClusterStub(object):
-    """Testable stub for C{charmhelpers.contrib.hahelpers.cluster}."""
-
-    leader = True
-
-    def is_elected_leader(self, resource):
-        return self.leader
-
-
 class HostStub(object):
     """Testable stub for C{charmhelpers.core.host}."""
 

=== modified file 'lib/tests/test_services.py'
--- lib/tests/test_services.py	2015-05-26 12:05:02 +0000
+++ lib/tests/test_services.py	2015-06-03 12:30:44 +0000
@@ -5,7 +5,7 @@
 from charmhelpers.core import templating
 
 from lib.tests.helpers import HookenvTest
-from lib.tests.stubs import ClusterStub, HostStub, SubprocessStub, FetchStub
+from lib.tests.stubs import HostStub, SubprocessStub, FetchStub
 from lib.tests.sample import (
     SAMPLE_DB_UNIT_DATA, SAMPLE_LEADER_CONTEXT_DATA, SAMPLE_AMQP_UNIT_DATA,
     SAMPLE_CONFIG_LICENSE_DATA, SAMPLE_CONFIG_OPENID_DATA, SAMPLE_HOSTED_DATA,
@@ -21,7 +21,6 @@
 
     def setUp(self):
         super(ServicesHookTest, self).setUp()
-        self.cluster = ClusterStub()
         self.host = HostStub()
         self.subprocess = SubprocessStub()
         self.subprocess.add_fake_executable(SCHEMA_SCRIPT)
@@ -31,8 +30,8 @@
         self.root_dir = self.useFixture(RootDir())
         self.fetch = FetchStub()
         self.hook = ServicesHook(
-            hookenv=self.hookenv, cluster=self.cluster, host=self.host,
-            subprocess=self.subprocess, paths=self.paths, fetch=self.fetch)
+            hookenv=self.hookenv, host=self.host, subprocess=self.subprocess,
+            paths=self.paths, fetch=self.fetch)
 
         # XXX Monkey patch the templating API, charmhelpers doesn't sport
         #     any dependency injection here as well.
@@ -77,12 +76,8 @@
         self.hook()
         # Assert that the HAProxyProvider has run by checking that it set the
         # relation with the dict returned by HAProxyProvider.provide_data (the
-        # only key of that dict is 'services'). The ID of relation being set
-        # is None because we're running in the website-relation-joined hook
-        # and are using the default relation ID (which in a real-world
-        # relation-set run will resolve to the relation for the http
-        # interface).
-        self.assertIn("services", self.hookenv.relations[None])
+        # only key of that dict is 'services').
+        self.assertIn("services", self.hookenv.relations["website:1"])
 
     def test_amqp_relation_not_ready(self):
         """
@@ -179,7 +174,7 @@
         If we're not the leader unit and we didn't yet get relation data from
         the leader, we are not ready.
         """
-        self.cluster.leader = False
+        self.hookenv.leader = False
         self.hook()
         self.assertIn(
             ("Incomplete relation: LandscapeRequirer", "DEBUG"),
@@ -190,7 +185,7 @@
         If we're not the leader unit and we got relation data from the leader,
         along with the rest of required relations, then we're good.
         """
-        self.cluster.leader = False
+        self.hookenv.leader = False
         self.hookenv.relations.update({
             "cluster": {
                 "cluster:1": {
@@ -236,3 +231,13 @@
         self.hook()
         data = yaml.load(self.hookenv.relations["website:1"]["services"])
         self.assertIsNotNone(data)
+
+    def test_leader_elected(self):
+        """
+        When a leader is elected, the ServicesHook sets the haproxy
+        relation data.
+        """
+        self.hookenv.hook = "leader-elected"
+        self.hook()
+        data = yaml.load(self.hookenv.relations["website:1"]["services"])
+        self.assertIsNotNone(data)