Merge lp:~free.ekanayaka/landscape-charm/handle-config-changes into lp:~landscape/landscape-charm/trunk

[Free Ekanayaka]

This branch turns the config-changed hook into a regular hook managed by the service framework, meaning that if configuration values get changed, then proper actions will be taken (such as rewriting configuration files and/or restarting services).

The code of the former config-changed hook that used to handle changes in the APT source has basically become a regular callback (under lib/callbacks/apt.py).

There also a couple of special cases:

1) if the only config value that has changed is the APT source, there's no need to restart services

2) if the SSL certificate config changes we want to notify the haproxy service and retrigger the HAProxyProvider

Finally, the standalone tests/02-sslcert test has been merged into the regular tests/01-basic test, since all we need is to set the SSL configuration and make sure it's correctly propagated to haproxy.

This branch turns the config-changed hook into a regular hook managed by the service framework, meaning that if configuration values get changed, then proper actions will be taken (such as rewriting configuration files and/or restarting services).

The code of the former config-changed hook that used to handle changes in the APT source has basically become a regular callback (under lib/callbacks/apt.py).

There also a couple of special cases:

1) if the only config value that has changed is the APT source, there's no need to restart services

2) if the SSL certificate config changes we want to notify the haproxy service and retrigger the HAProxyProvider

Finally, the standalone tests/02-sslcert test has been merged into the regular tests/01-basic test, since all we need is to set the SSL configuration and make sure it's correctly propagated to haproxy. This is enough to test the integration point between the haproxy charm and the landscape-server charm (iow there's now no need to bootstrap a separate environment in order to exercise this behavior).

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 276
Branch: lp:~free.ekanayaka/landscape-charm/handle-config-changes
Jenkins: https://ci.lscape.net/job/latch-test/932/

review: Approve (test results)

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

Looks great and works as advertised.

While I am happy that you just went ahead and made these 2 integration tests a single one (I was about to do the same, since there was no sense in testing our canonical deployment with two separate tests), it would be better if it was a separate branch.

review: Approve

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 277
Branch: lp:~free.ekanayaka/landscape-charm/handle-config-changes
Jenkins: https://ci.lscape.net/job/latch-test/935/

review: Approve (test results)

[Adam Collard (adam-collard)]

Agree with Danilo that this would have been better as two separate branches.

5 nitpicks below whilst I test the behaviour for real

[Adam Collard (adam-collard)]

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Thanks Danilo and Adam.

I was unsure whether to include the change in the SSL integration tests, however since they do exercise the somehow tricky code that this branch introduces (i.e. updating the haproxy relation on SSL config changed by manually kicking the data provider) I thought it made sense to have them here.

[🤖 Landscape Builder (landscape-builder)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 278
Branch: lp:~free.ekanayaka/landscape-charm/handle-config-changes
Jenkins: https://ci.lscape.net/job/latch-test/939/

review: Approve (test results)

=== modified file 'Makefile'
--- Makefile	2015-05-12 19:57:04 +0000
+++ Makefile	2015-05-14 14:52:22 +0000
@@ -43,11 +43,11 @@
 integration-test: test-depends
 	juju test --set-e -p SKIP_SLOW_TESTS,LS_CHARM_SOURCE,JUJU_HOME,JUJU_ENV,PG_MANUAL_TUNING -v --timeout 3000s
 
-integration-test-dense-maas: test-depends
+integration-test-dense-maas:
 	DEPLOYER_TARGET=landscape-dense-maas make integration-test
 
 # Run integration tests using the LDS package from the lds-trunk PPA
-integration-test-trunk: test-depends secrets
+integration-test-trunk: secrets
 	LS_CHARM_SOURCE=lds-trunk-ppa make integration-test
 
 deploy-dense-maas: bundles

=== modified file 'hooks/config-changed'
--- hooks/config-changed	2015-02-03 10:00:55 +0000
+++ hooks/config-changed	2015-05-14 14:52:22 +0000
@@ -1,9 +1,9 @@
 #!/usr/bin/python
 import sys
 
-from lib.config import ConfigHook
+from lib.services import ServicesHook
 
 
 if __name__ == "__main__":
-    hook = ConfigHook()
+    hook = ServicesHook()
     sys.exit(hook())

=== renamed file 'lib/config.py' => 'lib/callbacks/apt.py'
--- lib/config.py	2015-05-12 16:02:54 +0000
+++ lib/callbacks/apt.py	2015-05-14 14:52:22 +0000
@@ -2,25 +2,22 @@
 
 from charmhelpers import fetch
 from charmhelpers.core import hookenv
+from charmhelpers.core.services.base import ManagerCallback
 
-from lib.hook import Hook
 from lib.apt import Apt
 
 
-class ConfigHook(Hook):
-    """Execute config-changed hook logic."""
+class SetAPTSources(ManagerCallback):
+    """Set APT sources and refresh them if needed."""
 
     def __init__(self, hookenv=hookenv, fetch=fetch, subprocess=subprocess):
-        super(ConfigHook, self).__init__(hookenv=hookenv)
+        self._hookenv = hookenv
         self._fetch = fetch
         self._subprocess = subprocess
 
-    def _run(self):
-        # Re-set APT sources, if the have changed.
+    def __call__(self, manager, service_name, event_name):
+        # Re-set APT sources, if they have changed.
         apt = Apt(
             hookenv=self._hookenv, fetch=self._fetch,
             subprocess=self._subprocess)
         apt.set_sources()
-
-        config = self._hookenv.config()
-        config.save()

=== modified file 'lib/callbacks/scripts.py'
--- lib/callbacks/scripts.py	2015-05-07 10:26:45 +0000
+++ lib/callbacks/scripts.py	2015-05-14 14:52:22 +0000
@@ -1,5 +1,6 @@
 import subprocess
 
+from charmhelpers.core import hookenv
 from charmhelpers.core.services.base import ManagerCallback
 
 from lib.paths import LSCTL
@@ -32,6 +33,10 @@
 class LSCtl(ScriptCallback):
     """Call the lsctl script to start or stop services."""
 
+    def __init__(self, subprocess=subprocess, hookenv=hookenv):
+        super(LSCtl, self).__init__(subprocess=subprocess)
+        self._hookenv = hookenv
+
     def __call__(self, manager, service_name, event_name):
         action = event_name
         if event_name == "start":
@@ -39,4 +44,16 @@
             #     config changes have been applied and its semantics actually
             #     maps to a 'restart' action.
             action = "restart"
+
+        # In case we're reacting to config changes, we don't always want to
+        # restart the processes (for example if only the APT source changed).
+        if self._hookenv.hook_name() == "config-changed":
+            config = self._hookenv.config()
+            changed = set()
+            for key in config.keys():
+                if config.changed(key):
+                    changed.add(key)
+            if changed.issubset({"source", "key"}):
+                return
+
         self._run(LSCTL, (action,))

=== renamed file 'lib/tests/test_config.py' => 'lib/callbacks/tests/test_apt.py'
--- lib/tests/test_config.py	2015-05-13 09:22:49 +0000
+++ lib/callbacks/tests/test_apt.py	2015-05-14 14:52:22 +0000
@@ -1,41 +1,25 @@
 from lib.tests.helpers import HookenvTest
 from lib.tests.stubs import FetchStub, SubprocessStub
-from lib.config import ConfigHook
-
-
-class ConfigHookTest(HookenvTest):
+from lib.callbacks.apt import SetAPTSources
+
+
+class SetAPTSourcesTest(HookenvTest):
 
     def setUp(self):
-        super(ConfigHookTest, self).setUp()
+        super(SetAPTSourcesTest, self).setUp()
         self.fetch = FetchStub()
         self.subprocess = SubprocessStub()
-        self.hook = ConfigHook(
+        self.callback = SetAPTSources(
             hookenv=self.hookenv, fetch=self.fetch, subprocess=self.subprocess)
 
     def test_run(self):
         """
-        The L{ConfigHook} re-configures APT sources if they have changed.
+        The SetAPTSources callback re-configures APT sources if they have
+        changed.
         """
         config = self.hookenv.config()
         config["source"] = "ppa:landscape/14.10"
         config.save()
-        config.load_previous()
-        config["source"] = "ppa:landscape/15.01"
-        self.assertEqual(0, self.hook())
-        self.assertTrue(len(self.fetch.sources) == 1)
-
-    def test_save_after_change(self):
-        """
-        The L{ConfigHook} saves previous config values when called.
-        """
-        config = self.hookenv.config()
-        config["source"] = "ppa:landscape/14.10"
-        self.assertIsNone(config.previous("source"))
-        self.hook()
-        config.load_previous()
-        self.assertEqual("ppa:landscape/14.10", config.previous("source"))
-
-        config["source"] = "ppa:landscape/15.01"
-        self.hook()
-        config.load_previous()
-        self.assertEqual("ppa:landscape/15.01", config.previous("source"))
+        config["source"] = "ppa:landscape/15.01"
+        self.callback(None, None, None)
+        self.assertEqual(1, len(self.fetch.sources), repr(self.fetch.sources))

=== modified file 'lib/callbacks/tests/test_scripts.py'
--- lib/callbacks/tests/test_scripts.py	2015-04-07 13:56:21 +0000
+++ lib/callbacks/tests/test_scripts.py	2015-05-14 14:52:22 +0000
@@ -1,6 +1,7 @@
 from fixtures import TestWithFixtures
 
 from lib.callbacks.scripts import SchemaBootstrap, LSCtl
+from lib.tests.helpers import HookenvTest
 from lib.tests.stubs import SubprocessStub
 
 
@@ -21,12 +22,12 @@
             self.subprocess.calls[0][0])
 
 
-class LSCtlTest(TestWithFixtures):
+class LSCtlTest(HookenvTest):
 
     def setUp(self):
         super(LSCtlTest, self).setUp()
         self.subprocess = SubprocessStub()
-        self.callback = LSCtl(subprocess=self.subprocess)
+        self.callback = LSCtl(subprocess=self.subprocess, hookenv=self.hookenv)
 
     def test_start(self):
         """
@@ -45,3 +46,30 @@
         self.callback(None, None, "stop")
         self.assertEqual(
             ["/usr/bin/lsctl", "stop"], self.subprocess.calls[0][0])
+
+    def test_config_changed_only_apt(self):
+        """
+        The 'lsctl' script is not invoked if only the APT source has changed.
+        """
+        self.hookenv.hook = "config-changed"
+        config = self.hookenv.config()
+        config["source"] = "ppa:landscape/14.10"
+        config.save()
+        config["source"] = "ppa:landscape/15.01"
+        self.callback(None, None, "start")
+        self.assertEqual([], self.subprocess.calls)
+
+    def test_config_changed_not_only_apt(self):
+        """
+        The 'lsctl' script is invoked if not only the APT source has changed.
+        """
+        self.hookenv.hook = "config-changed"
+        config = self.hookenv.config()
+        config["source"] = "ppa:landscape/14.10"
+        config["license-file"] = "<old data>"
+        config.save()
+        config["source"] = "ppa:landscape/15.01"
+        config["license-file"] = "<new data>"
+        self.callback(None, None, "start")
+        self.assertEqual(
+            ["/usr/bin/lsctl", "restart"], self.subprocess.calls[0][0])

=== modified file 'lib/services.py'
--- lib/services.py	2015-05-13 07:05:26 +0000
+++ lib/services.py	2015-05-14 14:52:22 +0000
@@ -1,5 +1,6 @@
 import subprocess
 
+from charmhelpers import fetch
 from charmhelpers.core import hookenv
 from charmhelpers.core import host
 from charmhelpers.core.services.base import ServiceManager
@@ -18,6 +19,7 @@
 from lib.callbacks.scripts import SchemaBootstrap, LSCtl
 from lib.callbacks.filesystem import (
     EnsureConfigDir, WriteCustomSSLCertificate, WriteLicenseFile)
+from lib.callbacks.apt import SetAPTSources
 
 
 SERVICE_COUNTS = {
@@ -34,13 +36,14 @@
     proceed with the configuration if ready.
     """
     def __init__(self, hookenv=hookenv, cluster=cluster, host=host,
-                 subprocess=subprocess, paths=default_paths):
+                 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
+        self._fetch = fetch
 
     def _run(self):
         leader_context = None
@@ -49,13 +52,15 @@
             leader_context = LandscapeLeaderContext(
                 host=self._host, hookenv=self._hookenv)
 
-        manager = ServiceManager([{
+        haproxy_provider = HAProxyProvider(
+            SERVICE_COUNTS, paths=self._paths, is_leader=is_leader)
+
+        manager = ServiceManager(services=[{
             "service": "landscape",
             "ports": [],
             "provided_data": [
                 LandscapeProvider(leader_context),
-                HAProxyProvider(SERVICE_COUNTS, paths=self._paths,
-                                is_leader=is_leader),
+                haproxy_provider,
                 RabbitMQProvider(),
             ],
             # Required data is available to the render_template calls below.
@@ -77,11 +82,27 @@
                     owner="landscape", group="root", perms=0o640,
                     source="landscape-server",
                     target=self._paths.default_file()),
+                SetAPTSources(
+                    hookenv=self._hookenv, fetch=self._fetch,
+                    subprocess=self._subprocess),
                 EnsureConfigDir(paths=self._paths),
                 WriteCustomSSLCertificate(paths=self._paths),
                 SchemaBootstrap(subprocess=self._subprocess),
                 WriteLicenseFile(host=self._host, paths=self._paths),
             ],
-            "start": LSCtl(subprocess=self._subprocess),
+            "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()

=== modified file 'lib/tests/sample.py'
--- lib/tests/sample.py	2015-05-06 09:57:45 +0000
+++ lib/tests/sample.py	2015-05-14 14:52:22 +0000
@@ -27,10 +27,15 @@
     "password": "guessme",
 }
 
-SAMPLE_CONFIG_OPENID_DATA = {
+SAMPLE_CONFIG = {
+    "source": "ppa:landscape/14.10"
+}
+
+SAMPLE_CONFIG_OPENID_DATA = SAMPLE_CONFIG.copy()
+SAMPLE_CONFIG_OPENID_DATA.update({
     "openid-provider-url": "http://openid-host/",
     "openid-logout-url": "http://openid-host/logout",
-}
+})
 
 SAMPLE_CONFIG_LICENSE_DATA = {
     "license-file": base64.b64encode("license data"),

=== modified file 'lib/tests/test_services.py'
--- lib/tests/test_services.py	2015-05-06 10:47:36 +0000
+++ lib/tests/test_services.py	2015-05-14 14:52:22 +0000
@@ -1,13 +1,15 @@
 import os
+import base64
+import yaml
 
 from charmhelpers.core import templating
 
 from lib.tests.helpers import HookenvTest
-from lib.tests.stubs import ClusterStub, HostStub, SubprocessStub
+from lib.tests.stubs import ClusterStub, 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,
-    SAMPLE_SERVICE_COUNT_DATA, SAMPLE_WEBSITE_UNIT_DATA)
+    SAMPLE_SERVICE_COUNT_DATA, SAMPLE_WEBSITE_UNIT_DATA, SAMPLE_CONFIG)
 from lib.services import ServicesHook
 from lib.tests.rootdir import RootDir
 
@@ -24,9 +26,10 @@
         self.root_dir = self.useFixture(RootDir())
         self.paths = self.root_dir.paths
         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)
+            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.
@@ -34,7 +37,7 @@
         self.addCleanup(setattr, templating, "render", templating.render)
         templating.render = lambda *args: self.renders.append(args)
 
-        # Setup sample relation data for the "common" happy case (an LDS
+        # Setup sample data for the "common" happy case (an LDS
         # deployment with postgresql, haproxy and rabbitmq-server).
         self.hookenv.relations = {
             "db": {
@@ -53,6 +56,7 @@
                 },
             },
         }
+        self.hookenv.config().update(SAMPLE_CONFIG)
 
     def test_db_relation_not_ready(self):
         """
@@ -100,7 +104,7 @@
             "amqp": [SAMPLE_AMQP_UNIT_DATA],
             "website": [SAMPLE_WEBSITE_UNIT_DATA],
             "hosted": [SAMPLE_HOSTED_DATA],
-            "config": {},
+            "config": SAMPLE_CONFIG,
             "is_leader": True,
             "service_counts": SAMPLE_SERVICE_COUNT_DATA,
         }
@@ -205,3 +209,27 @@
             [("write_file", (self.paths.license_file(), "license data"),
               {"owner": "landscape", "group": "root", "perms": 0o640})],
             self.host.calls)
+
+    def test_apt_source(self):
+        """
+        If the source config changes, APT sources are refreshed.
+        """
+        config = self.hookenv.config()
+        config["source"] = "ppa:landscape/14.10"
+        config.save()
+        config["source"] = "ppa:landscape/15.01"
+        self.hook()
+        self.assertTrue(len(self.fetch.sources) == 1)
+
+    def test_ssl_cert_changed(self):
+        """
+        If the SSL certificate changes, the relation with haproxy gets updated.
+        """
+        self.hookenv.hook = "config-changed"
+        config = self.hookenv.config()
+        config.save()
+        config["ssl-cert"] = base64.b64encode(b"<cert>")
+        config["ssl-key"] = base64.b64encode(b"<key>")
+        self.hook()
+        data = yaml.load(self.hookenv.relations["website:1"]["services"])
+        self.assertIsNotNone(data)

=== removed file 'tests/02-sslcert'
--- tests/02-sslcert	2015-05-12 14:29:51 +0000
+++ tests/02-sslcert	1970-01-01 00:00:00 +0000
@@ -1,16 +0,0 @@
-#!/usr/bin/python3
-import base64
-
-from helpers import main
-from sslcert.assets import CERT_FILE, KEY_FILE
-
-if __name__ == "__main__":
-    with open(CERT_FILE, "rb") as fd:
-        ssl_cert = fd.read()
-    with open(KEY_FILE, "rb") as fd:
-        ssl_key = fd.read()
-    config = {
-        "landscape": {
-            "ssl-cert": base64.b64encode(ssl_cert).decode("utf-8"),
-            "ssl-key": base64.b64encode(ssl_key).decode("utf-8")}}
-    main(config=config)

=== renamed file 'tests/sslcert/assets.py' => 'tests/assets.py'
--- tests/sslcert/assets.py	2015-05-12 14:29:51 +0000
+++ tests/assets.py	2015-05-14 14:52:22 +0000
@@ -1,5 +1,18 @@
 import os
+import base64
 
-TEST_DIR = os.path.dirname(os.path.dirname(__file__))
+TEST_DIR = os.path.dirname(__file__)
 CERT_FILE = os.path.join(TEST_DIR, "sslcert", "server.crt")
 KEY_FILE = os.path.join(TEST_DIR, "sslcert", "server.key")
+
+
+def b64_ssl_cert():
+    with open(CERT_FILE, "rb") as fd:
+        ssl_cert = fd.read()
+    return base64.b64encode(ssl_cert).decode("utf-8")
+
+
+def b64_ssl_key():
+    with open(KEY_FILE, "rb") as fd:
+        ssl_key = fd.read()
+    return base64.b64encode(ssl_key).decode("utf-8")

=== renamed file 'tests/sslcert/test_config.py' => 'tests/basic/test_sslcert.py'
--- tests/sslcert/test_config.py	2015-05-13 10:18:14 +0000
+++ tests/basic/test_sslcert.py	2015-05-14 14:52:22 +0000
@@ -5,14 +5,14 @@
 a charm config option).
 """
 
-from sslcert.assets import CERT_FILE
+from assets import CERT_FILE
 from helpers import IntegrationTest, get_ssl_certificate_over_wire
-from layers import OneLandscapeUnitLayer
+from layers import OneLandscapeUnitCustomSSLCertificateLayer
 
 
 class SSLConfigurationTest(IntegrationTest):
 
-    layer = OneLandscapeUnitLayer
+    layer = OneLandscapeUnitCustomSSLCertificateLayer
 
     def test_certificate_is_what_we_expect(self):
         """

=== modified file 'tests/helpers.py'
--- tests/helpers.py	2015-05-13 10:21:31 +0000
+++ tests/helpers.py	2015-05-14 14:52:22 +0000
@@ -152,6 +152,17 @@
         """Start the given Landscape service on the given unit."""
         self._control_landscape_service("start", service, unit)
 
+    def configure_ssl(self, cert, key):
+        """Start the given Landscape service on the given unit."""
+        self._deployment.configure(
+            "landscape-server", {"ssl-cert": cert, "ssl-key": key})
+        # Wait for initial landscape-server hooks to fire
+        self._deployment.sentry.wait()
+        # Wait for haproxy hooks to fire
+        self._deployment.sentry.wait()
+        # Wait for landscape-server hooks triggered by the haproxy ones to fire
+        self._deployment.sentry.wait()
+
     def _get_charm_dir(self):
         """Get the path to the root of the charm directory."""
         return os.path.join(os.path.dirname(__file__), "..")
@@ -257,6 +268,11 @@
     global _config
     _config = config
 
+    # XXX This will force zope.testrunner to write to stderr, since stdout is
+    # not being printed synchronously by "juju test", see also the call to
+    # subprocess in charmtools.test.Orchestra.perform().
+    sys.stdout = sys.stderr
+
     # Figure out the package holding the test files to use and run them.
     path = os.path.join(os.getcwd(), "tests")
     module = os.path.basename(sys.argv[0]).split("-")[1]

=== modified file 'tests/layers.py'
--- tests/layers.py	2015-05-13 10:18:14 +0000
+++ tests/layers.py	2015-05-14 14:52:22 +0000
@@ -1,4 +1,5 @@
 from helpers import EnvironmentFixture, get_config
+from assets import b64_ssl_cert, b64_ssl_key
 
 
 class OneLandscapeUnitLayer(object):
@@ -16,3 +17,20 @@
     @classmethod
     def tearDown(cls):
         cls.environment.cleanUp()
+
+
+class OneLandscapeUnitCustomSSLCertificateLayer(OneLandscapeUnitLayer):
+    """Layer for all tests needing a deployment with a custom SSL certificate.
+
+    The deployment has the same structure as OneLandscapeUnitLayer, but a
+    custom SSL certificate will be set on the landscape-service service, using
+    the ssl-cert and ssl-key configuration options.
+    """
+
+    @classmethod
+    def setUp(cls):
+        cls.environment.configure_ssl(b64_ssl_cert(), b64_ssl_key())
+
+    @classmethod
+    def tearDown(cls):
+        cls.environment.configure_ssl("", "")

=== removed file 'tests/sslcert/__init__.py'