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.

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 change our code to use the new leader election API, since
the is_elected_leader() function is a bit buggy.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 306
Branch: lp:~bjornt/landscape-charm/package-upload-leader-change
Jenkins: https://ci.lscape.net/job/latch-test/1171/

review: Approve (test results)

[Free Ekanayaka (free.ekanayaka)]

Looks good to me, marking as Needs information because I'd like to ask what do you think of adding some integration tests too.

It could be something like:

== tests/layers.py ==

class TwoLandscapeUnitsLayer(object):

    """Layer for tests meant to run against a deployment with two Landscape units.

    This layer will just add one unit on top the deployment from OneLandscapeUnitLayer,
    and wait for it to be fully functional. The very same unit will be removed upon
    layer tearDown.
    """

== tests/basic/test_leader.py ==

class LeaderTest(IntegrationTest):

   layer = TwoLandscapeUnitsLayer

   def test_package_upload(self):
      """Test that package upload requests are routed to the leader unit."""

review: Needs Information

[Adam Collard (adam-collard)]

Is there a bug for this? Please link and provide steps for testing this, perhaps with the integration test Free suggested it would be easy to reproduce "live"

review: Needs Information

[Björn Tillenius (bjornt)]

> Looks good to me, marking as Needs information because I'd like to ask what do
> you think of adding some integration tests too.
>
> It could be something like:
>
> == tests/layers.py ==
>
> class TwoLandscapeUnitsLayer(object):
>
> """Layer for tests meant to run against a deployment with two Landscape
> units.
>
> This layer will just add one unit on top the deployment from
> OneLandscapeUnitLayer,
> and wait for it to be fully functional. The very same unit will be removed
> upon
> layer tearDown.
> """
>
> == tests/basic/test_leader.py ==
>
> class LeaderTest(IntegrationTest):
>
> layer = TwoLandscapeUnitsLayer
>
> def test_package_upload(self):
> """Test that package upload requests are routed to the leader unit."""

Agreed that integration tests would be good, but I'd rather do that in a separate
branch. Especially since the test you're proposing is for existing functionality,
not for something I introduced in this branch.

But I also would like to add an integration test for when the leader is destroyed,
but that's not trivial, since it's destructive. So a separate branch for that
makes more sense to me.

[Björn Tillenius (bjornt)]

> Is there a bug for this? Please link and provide steps for testing this,
> perhaps with the integration test Free suggested it would be easy to reproduce
> "live"

Sorry, I forgot to link the bug. It includes an explanation of the problem. But
basically:

  make deploy
  juju add-unit landscape-server
  <wait until the new unit has joined the cluster>
  juju destroy-unit landscape-server/0

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

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

Merge trunk.

[Björn Tillenius (bjornt)]

I filed bug 1461816 for adding the integration tests.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 307
Branch: lp:~bjornt/landscape-charm/package-upload-leader-change
Jenkins: https://ci.lscape.net/job/latch-test/1180/

review: Approve (test results)

[Free Ekanayaka (free.ekanayaka)]

|--==> On Thu, 04 Jun 2015 07:07:44 -0000, Björn Tillenius <email address hidden> said:

  >>Looks good to me, marking as Needs information because I'd like to ask what do
  >>you think of adding some integration tests too.
  >>
  >>It could be something like:
  >>
  >>== tests/layers.py ==
  >>
  >>class TwoLandscapeUnitsLayer(object):
  >>
  >>"""Layer for tests meant to run against a deployment with two Landscape
  >>units.
  >>
  >>This layer will just add one unit on top the deployment from
  >>OneLandscapeUnitLayer,
  >>and wait for it to be fully functional. The very same unit will be removed
  >>upon
  >>layer tearDown.
  >>"""
  >>
  >>== tests/basic/test_leader.py ==
  >>
  >>class LeaderTest(IntegrationTest):
  >>
  >>layer = TwoLandscapeUnitsLayer
  >>
  >>def test_package_upload(self):
  >>"""Test that package upload requests are routed to the leader unit."""

  > Agreed that integration tests would be good, but I'd rather do that in a separate
  > branch. Especially since the test you're proposing is for existing functionality,
  > not for something I introduced in this branch.

Works for me, I was suggesting it since the branch is relatively small
and the integration tests would be strictly related. Not blocking tho.

  > But I also would like to add an integration test for when the leader is destroyed,
  > but that's not trivial, since it's destructive. So a separate branch for that
  > makes more sense to me.

Yes, I think this can be made as a separate layer, e.g.:

class OneLandscapeUnitLayer(object):
    """will run all tests with a fresh single unit (landscape-server/0)"""

class TwoLandscapeUnitsLayer(OneLandscapeUnitLayer):
    """will run all tests with two units (landscape-server/{0,1})"""

class LeaderUnitChangeLayer(TwoLandscapeUnitsLayer):
    """will remove landscape-server/0 and we'll be left with landscape-server/1"""

And probably drop the OneLandscapeUnitNoCronLayer and
OneLandscapeUnitCustomSSLCertificateLayer layers, moving that
logic to just relevant setUpClass/tearDownClass (since those layers
are used just by one test class).

[Adam Collard (adam-collard)]

I share Free's concern about the (seemingly) left-over code, but you can deal with that on his review. Tested as described and all looks good, confirmed with a GET to /upload after killing the leader that it was routed through properly.

review: Approve

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

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.

[Björn Tillenius (bjornt)]

Replied to the inline comments.

[Free Ekanayaka (free.ekanayaka)]

+1

review: Approve

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 310
Branch: lp:~bjornt/landscape-charm/package-upload-leader-change
Jenkins: https://ci.lscape.net/job/latch-test/1187/

review: Approve (test results)

=== added file 'hooks/leader-elected'
--- hooks/leader-elected	1970-01-01 00:00:00 +0000
+++ hooks/leader-elected	2015-06-04 09:40:39 +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-04 09:40:39 +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 = {
@@ -73,12 +71,10 @@
     interface = "http"
     required_keys = ["services"]
 
-    def __init__(self, service_counts, hookenv=hookenv, paths=default_paths,
-                 is_leader=False):
+    def __init__(self, service_counts, hookenv=hookenv, paths=default_paths):
         self._hookenv = hookenv
         self._service_counts = service_counts
         self._paths = paths
-        self._is_leader = is_leader
         super(HAProxyProvider, self).__init__()
 
     def provide_data(self):
@@ -100,14 +96,18 @@
     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")),
         ]
-        if self._is_leader:
+        if self._hookenv.is_leader():
             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 +116,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 +135,9 @@
             "service_name": "landscape-%s" % name,
             "service_host": "0.0.0.0",
             "service_port": SERVICE_PORTS[name],
-            "service_options": SERVICE_OPTIONS[name],
+            # Copy the service options, since it will be modified if
+            # we're the leader.
+            "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-04 09:40:39 +0000
@@ -38,8 +38,9 @@
         The HAProxyProvider class feeds haproxy with the services that this
         Landscape unit runs. By default all services are run.
         """
+        self.hookenv.leader = False
         relation = HAProxyProvider(
-            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths)
+            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths, hookenv=self.hookenv)
 
         # Provide some fake ssl-cert and ssl-key config entries
         config = self.hookenv.config()
@@ -96,10 +97,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": [
@@ -137,7 +136,7 @@
                 fd.write("{} error page".format(name))
 
         relation = HAProxyProvider(
-            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths)
+            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths, hookenv=self.hookenv)
 
         data = relation.provide_data()
         services = yaml.safe_load(data["services"])
@@ -158,17 +157,80 @@
         # Create an empty root tree
         temp_dir = self.useFixture(TempDir())
         provider = HAProxyProvider(
-            SAMPLE_SERVICE_COUNT_DATA, paths=Paths(temp_dir.path))
+            SAMPLE_SERVICE_COUNT_DATA, paths=Paths(temp_dir.path),
+            hookenv=self.hookenv)
 
         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.
+        """
+        self.hookenv.leader = True
+        relation = HAProxyProvider(
+            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths, hookenv=self.hookenv)
+
+        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.
+        """
+        self.hookenv.leader = False
+        relation = HAProxyProvider(
+            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths, hookenv=self.hookenv)
+
+        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
         ["DEFAULT"] for the HAproxy SSL cert.
         """
         provider = HAProxyProvider(
-            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths)
+            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths, hookenv=self.hookenv)
         data = provider.provide_data()
         services = yaml.safe_load(data["services"])
 
@@ -258,9 +320,9 @@
         The landscape service leader writes a server entry in the
         landscape-package-upload backend.
         """
+        self.hookenv.leader = True
         provider = HAProxyProvider(
-            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths, hookenv=self.hookenv,
-            is_leader=True)
+            SAMPLE_SERVICE_COUNT_DATA, paths=self.paths, hookenv=self.hookenv)
 
         data = provider.provide_data()
         services = yaml.safe_load(data["services"])

=== modified file 'lib/services.py'
--- lib/services.py	2015-06-03 12:23:06 +0000
+++ lib/services.py	2015-06-04 09:40:39 +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,7 +46,7 @@
 
     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)
@@ -58,8 +56,7 @@
             "ports": [],
             "provided_data": [
                 LandscapeProvider(leader_context),
-                HAProxyProvider(
-                    SERVICE_COUNTS, paths=self._paths, is_leader=is_leader),
+                HAProxyProvider(SERVICE_COUNTS, paths=self._paths),
                 RabbitMQProvider(),
             ],
             # Required data is available to the render_template calls below.

=== modified file 'lib/tests/stubs.py'
--- lib/tests/stubs.py	2015-06-02 07:06:30 +0000
+++ lib/tests/stubs.py	2015-06-04 09:40:39 +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-06-03 12:23:06 +0000
+++ lib/tests/test_services.py	2015-06-04 09:40:39 +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.
@@ -175,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"),
@@ -186,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": {
@@ -232,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)