Merge lp:~tribaal/landscape-charm/send-haproxy-error-in-relation into lp:~landscape/landscape-charm/trunk

[Chris Glass]

This branch adds errorfiles to the haproxy relation.

This branch adds errorfiles to the haproxy relation.

Unfortunately https://bugs.launchpad.net/juju-core/+bug/1437366 prevents us from enabling all of the errorfiles for now, but once fixed it should be pretty easy to enable (just uncommenting the errorfiles in the map introduced here).

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 241
Branch: lp:~tribaal/landscape-charm/send-haproxy-error-in-relation
Jenkins: https://ci.lscape.net/job/latch-test/364/

review: Approve (test results)

[Free Ekanayaka (free.ekanayaka)]

Marking as needs information because I'd like to have your opinion on a testing approach based on dependency-injection rather than monkey-patching/mocking.

review: Needs Information

[Данило Шеган (danilo)]

I personally do not have a specific preference towards one approach or the other: neither solves the critical problem in that when the real interface changes, tests continue working because they are using stubs/mocks. However, we (well, Free, and then the rest of us) have already started using the dependency injection for the landscape-charm, and I like the consistency.

So, unless there's a particular reason not to use the same pattern, I'd prefer it if you did.

[Free Ekanayaka (free.ekanayaka)]

> I personally do not have a specific preference towards one approach or the
> other: neither solves the critical problem in that when the real interface
> changes, tests continue working because they are using stubs/mocks. However,
> we (well, Free, and then the rest of us) have already started using the
> dependency injection for the landscape-charm, and I like the consistency.

The difference with the proposed approached is that:

- You'll limit the interfaces that you mock/stub to the minimum, i.e. either to APIs that you don't control and are not easily unit-testable (either by nature of because of factoring problem) or to APIs that you do control but are not easily unit-testable by nature (e.g. they go over the network). In the code in this branch we're mocking our own APIs that we could just call directly, as it happens in the proposed approach: note that if that interface changes tests *will* break.
 So there might indeed be some parts that don't exercise the real interface, but they will be limited to the minimum, and in this particular branch there's no need for them.

- When your code grows and the call stack/layers become deeper you won't have mocking explosion.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 242
Branch: lp:~tribaal/landscape-charm/send-haproxy-error-in-relation
Jenkins: https://ci.lscape.net/job/latch-test/399/

review: Approve (test results)

[Данило Шеган (danilo)]

Other than the left-over debugging import and possibly better test setup, looks good. A few minor comments throughout (mostly me being anal about var names :), but they are up to you.

review: Approve

[Chris Glass (tribaal)]

Sorry, I should have switched to WIP before I pushed (I switched machines), that ipdb and other things could really have been spared to you :/

Anyhow, good comments, I'll fix them before switching back to needs review :)

[Данило Шеган (danilo)]

Heh, no worries, sorry for being too eager, I just didn't want you to wait too long for a review since the branch has been up for a while :)

[Chris Glass (tribaal)]

Should be good for another round of comments.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 245
Branch: lp:~tribaal/landscape-charm/send-haproxy-error-in-relation
Jenkins: https://ci.lscape.net/job/latch-test/405/

review: Approve (test results)

[Free Ekanayaka (free.ekanayaka)]

Looks good, but marking as Needs fixing because there's some untested behavior and I'm a bit strong on using composition over inheritance when it makes sense (see the Mixin vs Fixture comment).

review: Needs Fixing

[lp:~tribaal/landscape-charm/send-haproxy-error-in-relation]

246. By Chris Glass on 2015-04-07

First review comments addressed.

247. By Chris Glass on 2015-04-07

Removed leftover comments. Fixed tests.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Fail
Revno: 247
Branch: lp:~tribaal/landscape-charm/send-haproxy-error-in-relation
Jenkins: https://ci.lscape.net/job/latch-test/408/
-------------------------------------------
./dev/ubuntu-deps
Reading package lists...
Building dependency tree...
Reading state information...
software-properties-common is already the newest version.
The following package was automatically installed and is no longer required:
  os-prober
Use 'apt-get autoremove' to remove it.
0 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
gpg: keyring `/tmp/tmpubm_e59u/secring.gpg' created
gpg: keyring `/tmp/tmpubm_e59u/pubring.gpg' created
gpg: requesting key C8068B11 from hkp server keyserver.ubuntu.com
gpg: /tmp/tmpubm_e59u/trustdb.gpg: trustdb created
gpg: key C8068B11: public key "Launchpad Ensemble PPA" imported
gpg: Total number processed: 1
gpg: imported: 1 (RSA: 1)
OK
Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Get:2 http://security.ubuntu.com trusty-security Release [63.5 kB]
Ign http://ppa.launchpad.net trusty InRelease
Ign http://ppa.launchpad.net trusty InRelease
Hit http://ppa.launchpad.net trusty Release.gpg
Hit http://ppa.launchpad.net trusty Release.gpg
Hit http://ppa.launchpad.net trusty Release
Hit http://ppa.launchpad.net trusty Release
Get:3 http://security.ubuntu.com trusty-security/main Sources [76.1 kB]
Hit http://ppa.launchpad.net trusty/main amd64 Packages
Ign https://private-ppa.launchpad.net trusty InRelease
Ign https://private-ppa.launchpad.net trusty InRelease
Get:4 http://security.ubuntu.com trusty-security/universe Sources [19.5 kB]
Get:5 http://security.ubuntu.com trusty-security/main amd64 Packages [251 kB]
Get:6 http://security.ubuntu.com trusty-security/universe amd64 Packages [92.0 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Ign http://archive.ubuntu.com trusty InRelease
Hit http://ppa.launchpad.net trusty/main Translation-en
Hit https://private-ppa.launchpad.net trusty Release.gpg
Hit https://private-ppa.launchpad.net trusty Release.gpg
Hit http://ppa.launchpad.net trusty/main amd64 Packages
Hit https://private-ppa.launchpad.net trusty Release
Hit http://ppa.launchpad.net trusty/main Translation-en
Hit https://private-ppa.launchpad.net trusty Release
Hit https://private-ppa.launchpad.net trusty/main amd64 Packages
Hit https://private-ppa.launchpad.net trusty/main Translation-en
Hit https://private-ppa.launchpad.net trusty/main amd64 Packages
Ign http://archive.ubuntu.com trusty-updates InRelease
Ign https://private-ppa.launchpad.net trusty/main Translation-en_US
Ign https://private-ppa.launchpad.net trusty/main Translation-en
Hit http://archive.ubuntu.com trusty Release.gpg
Hit http://archive.ubuntu.com trusty-updates Release.gpg
Hit http://archive.ubuntu.com trusty Release
Hit http://archive.ubuntu.com trusty-updates Release
Hit http://archive.ubuntu.com trusty/main Sources
Hit http://archive.ubuntu.com trusty/universe Sources
Hit http://archive.ubuntu.com trusty/main amd64 Packages
Hit http://archive...

review: Needs Fixing (test results)

[lp:~tribaal/landscape-charm/send-haproxy-error-in-relation]

248. By Chris Glass on 2015-04-07

Composition over inheritance change.

[Chris Glass (tribaal)]

All comments should be addressed now.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 248
Branch: lp:~tribaal/landscape-charm/send-haproxy-error-in-relation
Jenkins: https://ci.lscape.net/job/latch-test/410/

review: Approve (test results)

[Данило Шеган (danilo)]

FWIW, still looks good to me :)

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Thanks for addressing everything. A few more little points, +1 with those addressed.

review: Approve

[lp:~tribaal/landscape-charm/send-haproxy-error-in-relation]

249. By Chris Glass on 2015-04-07

Fixed a couple of review points.

250. By Chris Glass on 2015-04-07

All comments addressed.

251. By Chris Glass on 2015-04-07

lint

=== modified file 'README.md'
--- README.md	2015-03-23 19:27:59 +0000
+++ README.md	2015-04-07 13:50:51 +0000
@@ -90,5 +90,5 @@
 them:
 
     # Make sure your JUJU_ENV is *not* bootstrapped, and:
-    $ sudo apt-get install python-pyscopg2 python-mocker python-psutil
+    $ sudo apt-get install python-psycopg2 python-mocker python-psutil
     $ JUJU_ENV=<env> make integration-test

=== modified file 'hooks/lib/relations/haproxy.py'
--- hooks/lib/relations/haproxy.py	2015-03-04 12:16:19 +0000
+++ hooks/lib/relations/haproxy.py	2015-04-07 13:50:51 +0000
@@ -1,5 +1,9 @@
+import base64
+import os
 import yaml
 
+from lib.hook import HookError
+
 from charmhelpers.core import hookenv
 from charmhelpers.core.services.helpers import RelationContext
 
@@ -40,6 +44,19 @@
     "fall 5",
     "maxconn 50",
 ]
+ERRORFILES_MAP = {
+    # Add 503 only for now since that's what the integration tests
+    # check.
+    "503": "unplanned-offline-haproxy.html",
+    # TODO: Due to bug #1437366 the command line call to "relation-set"
+    # will fail by reaching MAX_ARGS if too many errorfiles are set.
+    # Until fixed let's set only one errorfile to assert it works.
+    #"403": "unauthorized-haproxy.html",
+    #"500": "exception-haproxy.html",
+    #"502": "unplanned-offline-haproxy.html",
+    #"504": "timeout-haproxy.html",
+}
+OFFLINE_FOLDER = "/opt/canonical/landscape/canonical/landscape/offline"
 
 
 class HAProxyProvider(RelationContext):
@@ -47,11 +64,11 @@
 
     name = "website"
     interface = "http"
-    required_keys = [
-        "services"]
+    required_keys = ["services"]
 
-    def __init__(self, hookenv=hookenv):
+    def __init__(self, hookenv=hookenv, offline_dir=OFFLINE_FOLDER):
         self._hookenv = hookenv
+        self._offline_dir = offline_dir
         super(HAProxyProvider, self).__init__()
 
     def provide_data(self):
@@ -93,6 +110,7 @@
             "service_host": "0.0.0.0",
             "service_port": SERVICE_PORTS[name],
             "service_options": SERVICE_OPTIONS[name],
+            "errorfiles": self._get_error_files()
         }
 
     def _backend(self, name, servers):
@@ -118,3 +136,24 @@
         server_name = "landscape-%s-%s" % (name, unit_name.replace("/", "-"))
         server_port = SERVER_PORTS[name]
         return (server_name, server_ip, server_port, SERVER_OPTIONS)
+
+    def _get_error_files(self):
+        """Return the errorfiles configuration."""
+        result = []
+
+        for error_code, file_name in sorted(ERRORFILES_MAP.items()):
+            content = None
+            path = os.path.join(self._offline_dir, file_name)
+
+            try:
+                with open(path, "r") as error_file:
+                    content = error_file.read()
+            except IOError as error:
+                raise HookError(
+                    "Could not read '%s' (%s)!" % (path, str(error)))
+
+            entry = {"http_status": error_code,
+                     "content": base64.b64encode(content)}
+            result.append(entry)
+
+        return result

=== modified file 'hooks/lib/relations/tests/test_haproxy.py'
--- hooks/lib/relations/tests/test_haproxy.py	2015-03-04 07:47:03 +0000
+++ hooks/lib/relations/tests/test_haproxy.py	2015-04-07 13:50:51 +0000
@@ -1,7 +1,13 @@
 import yaml
 
+from base64 import b64encode
+from fixtures import TempDir
+
+from lib.relations.haproxy import (
+    HAProxyProvider, SERVER_OPTIONS, ERRORFILES_MAP)
+from lib.hook import HookError
 from lib.tests.helpers import HookenvTest
-from lib.relations.haproxy import HAProxyProvider, SERVER_OPTIONS
+from lib.tests.offline_fixture import OfflineDir
 
 
 class HAProxyProviderTest(HookenvTest):
@@ -22,8 +28,18 @@
         The HAProxyProvider class feeds haproxy with the services that this
         Landscape unit runs. By default all services are run.
         """
-        relation = HAProxyProvider()
+        offline_dir = self.useFixture(OfflineDir()).path
+        relation = HAProxyProvider(offline_dir=offline_dir)
+
+        expected_errorfiles = []
+
+        for error_code, filename in sorted(ERRORFILES_MAP.items()):
+            expected_content = b64encode("Fake %s" % filename)
+            expected_errorfiles.append(
+                {"http_status": error_code, "content": expected_content})
+
         data = relation.provide_data()
+
         services = yaml.safe_load(data["services"])
         self.assertEqual([
             {"service_name": "landscape-http",
@@ -36,6 +52,7 @@
                  "acl ping path_beg -i /ping",
                  "redirect scheme https unless ping",
                  "use_backend landscape-ping if ping"],
+             "errorfiles": expected_errorfiles,
              "servers": [
                  ["landscape-appserver-landscape-server-0",
                   "1.2.3.4", 8080, SERVER_OPTIONS]],
@@ -56,6 +73,7 @@
                  "acl api path_beg -i /api",
                  "use_backend landscape-message if message",
                  "use_backend landscape-api if api"],
+             "errorfiles": expected_errorfiles,
              "crts": ["DEFAULT"],
              "servers": [
                  ["landscape-appserver-landscape-server-0",
@@ -70,3 +88,13 @@
                       ["landscape-api-landscape-server-0",
                        "1.2.3.4", 9080, SERVER_OPTIONS]]}]}],
             services)
+
+    def test_files_cannot_be_read(self):
+        """
+        In case a file specified in the errorfiles map cannot be read, the
+        provide_data method raises a HookError.
+        """
+        offline_dir = self.useFixture(TempDir()).path
+        provider = HAProxyProvider(offline_dir=offline_dir)
+
+        self.assertRaises(HookError, provider.provide_data)

=== modified file 'hooks/lib/services.py'
--- hooks/lib/services.py	2015-04-02 16:37:14 +0000
+++ hooks/lib/services.py	2015-04-07 13:50:51 +0000
@@ -9,7 +9,7 @@
 from lib.hook import Hook
 from lib.relations.postgresql import PostgreSQLRequirer
 from lib.relations.rabbitmq import RabbitMQRequirer, RabbitMQProvider
-from lib.relations.haproxy import HAProxyProvider
+from lib.relations.haproxy import HAProxyProvider, OFFLINE_FOLDER
 from lib.relations.landscape import (
     LandscapeLeaderContext, LandscapeRequirer, LandscapeProvider)
 from lib.callbacks.scripts import SchemaBootstrap, LSCtl
@@ -26,11 +26,12 @@
     proceed with the configuration if ready.
     """
     def __init__(self, hookenv=hookenv, cluster=cluster, host=host,
-                 subprocess=subprocess):
+                 subprocess=subprocess, offline_dir=OFFLINE_FOLDER):
         super(ServicesHook, self).__init__(hookenv=hookenv)
         self._cluster = cluster
         self._host = host
         self._subprocess = subprocess
+        self._offline_dir = offline_dir
 
     def _run(self):
         leader_context = None
@@ -43,7 +44,7 @@
             "ports": [],
             "provided_data": [
                 LandscapeProvider(leader_context),
-                HAProxyProvider(),
+                HAProxyProvider(offline_dir=self._offline_dir),
                 RabbitMQProvider(),
             ],
             "required_data": [

=== added file 'hooks/lib/tests/offline_fixture.py'
--- hooks/lib/tests/offline_fixture.py	1970-01-01 00:00:00 +0000
+++ hooks/lib/tests/offline_fixture.py	2015-04-07 13:50:51 +0000
@@ -0,0 +1,21 @@
+import os
+from fixtures import TempDir
+
+from lib.relations.haproxy import ERRORFILES_MAP
+
+
+class OfflineDir(TempDir):
+    """Temporary offline dir populated with sample data."""
+
+    rootdir = ""  # This is expected to be set
+
+    def __init__(self, errorfiles_map=ERRORFILES_MAP):
+        super(OfflineDir, self)
+        self._errorfiles_map = errorfiles_map
+
+    def setUp(self):
+        super(OfflineDir, self).setUp()
+        for filename in self._errorfiles_map.itervalues():
+            fake_content = "Fake %s" % filename
+            with open(os.path.join(self.path, filename), "w") as fd:
+                fd.write(fake_content)

=== modified file 'hooks/lib/tests/test_services.py'
--- hooks/lib/tests/test_services.py	2015-04-02 17:00:37 +0000
+++ hooks/lib/tests/test_services.py	2015-04-07 13:50:51 +0000
@@ -6,6 +6,7 @@
     SAMPLE_DB_UNIT_DATA, SAMPLE_LEADER_CONTEXT_DATA, SAMPLE_AMQP_UNIT_DATA,
     SAMPLE_CONFIG_OPENID_DATA)
 from lib.services import ServicesHook, SERVICE_CONF, DEFAULT_FILE
+from lib.tests.offline_fixture import OfflineDir
 
 
 class ServicesHookTest(HookenvTest):
@@ -17,9 +18,10 @@
         self.cluster = ClusterStub()
         self.host = HostStub()
         self.subprocess = SubprocessStub()
+        self.offline_dir = self.useFixture(OfflineDir()).path
         self.hook = ServicesHook(
             hookenv=self.hookenv, cluster=self.cluster, host=self.host,
-            subprocess=self.subprocess)
+            subprocess=self.subprocess, offline_dir=self.offline_dir)
 
         # XXX Monkey patch the templating API, charmhelpers doesn't sport
         #     any dependency injection here as well.

=== modified file 'tests/01-begin.py'
--- tests/01-begin.py	2015-03-26 09:03:58 +0000
+++ tests/01-begin.py	2015-04-07 13:50:51 +0000
@@ -378,7 +378,6 @@
             self.assertNotEqual(len(schema["store_password"]), 0)
 
 
[e-mail]("no unavailable pages yet")
 class LandscapeErrorPagesTests(BaseLandscapeTests):
 
     @classmethod
@@ -386,14 +385,8 @@
         """Prepares juju_status and other attributes that many tests use."""
         cls.juju_status = juju_status()
         cls.frontend = find_address(cls.juju_status, "haproxy")
-        cls.app_unit = find_landscape_unit_with_service(
-            cls.juju_status, "appserver")
-        cls.msg_unit = find_landscape_unit_with_service(
-            cls.juju_status, "msgserver")
-        cls.ping_unit = find_landscape_unit_with_service(
-            cls.juju_status, "pingserver")
-        cls.async_unit = find_landscape_unit_with_service(
-            cls.juju_status, "async-frontend")
+        cls.landscape_units = get_landscape_units(cls.juju_status)
+        cls.first_unit = cls.landscape_units[0]
 
     def run_command_on_unit(self, cmd, unit):
         output = check_output(["juju", "ssh", unit, cmd], stderr=PIPE)
@@ -409,46 +402,49 @@
 
     def test_app_unavailable_page(self):
         """
-        Verify that the frontend shows the styled unavailable page for app.
+        Verify that the frontend shows the styled unavailable page.
         """
         self.addCleanup(self.start_server, "landscape-appserver",
-                        self.app_unit)
-        self.stop_server("landscape-appserver", self.app_unit)
+                        self.first_unit)
+        self.stop_server("landscape-appserver", self.first_unit)
         good_content = "please phone us"
         url = "https://{}/".format(self.frontend)
         check_url(url, good_content)
 
+    @unittest.skip
     def test_msg_unavailable_page(self):
         """
         Verify that the frontend shows the unstyled unavailable page for msg.
         """
         self.addCleanup(self.start_server, "landscape-msgserver",
-                        self.msg_unit)
-        self.stop_server("landscape-msgserver", self.msg_unit)
+                        self.first_unit)
+        self.stop_server("landscape-msgserver", self.first_unit)
         good_content = ["503 Service Unavailable",
                         "No server is available to handle this request."]
         url = "https://{}/message-system".format(self.frontend)
         check_url(url, good_content)
 
+    @unittest.skip
     def test_ping_unavailable_page(self):
         """
         Verify that the frontend shows the unstyled unavailable page for ping.
         """
         self.addCleanup(self.start_server, "landscape-pingserver",
-                        self.ping_unit)
-        self.stop_server("landscape-pingserver", self.ping_unit)
+                        self.first_unit)
+        self.stop_server("landscape-pingserver", self.first_unit)
         good_content = ["503 Service Unavailable",
                         "No server is available to handle this request."]
         url = "http://{}/ping".format(self.frontend)
         check_url(url, good_content)
 
+    @unittest.skip
     def test_async_unavailable_page(self):
         """
         Verify that the frontend shows the unstyled unavailable page for async.
         """
         self.addCleanup(self.start_server, "landscape-async-frontend",
-                        self.async_unit)
-        self.stop_server("landscape-async-frontend", self.async_unit)
+                        self.first_unit)
+        self.stop_server("landscape-async-frontend", self.first_unit)
         good_content = ["503 Service Unavailable",
                         "No server is available to handle this request."]
         url = "https://{}/ajax".format(self.frontend)