Merge lp:~evarlast/charms/trusty/logstash-forwarder/add-logs-relation into lp:~canonical-is-sa/charms/trusty/logstash-forwarder/trunk

[Jay R. Wren]

add logs relation

[Michael Nelson (michael.nelson)]

Thanks Jay. This generally looks good. I'm not sure, but did you forget to commit the hooks/logs-relation-* symlinks, or are they no longer required?

I've a few other comments below. My main concern is the format that you're using for the relation data - in that I think it will restrict our future options (ie. if in the future we want to have two files for one type, or have extra fields for a file type). See below for more.

Let me know what you think.

[Jay R. Wren (evarlast)]

I addressed most the code issues.

Regarding the relation data: IMO to keep things simple on the other side of the relation, this makes the most sense. It turns out that logstash-forwarder is quite OK with single files configured with the same type. The simplicity of file, type pair is intentional. No need for added complexity. The restriction on an extra field is also by design. Parallel arrays were used for files and lists, an additional parallel array could be given by any charm implementing the logs interface, and this charm consuming the interface would ignore it. This is flexible and simple.

Can it land now?

[lp:~evarlast/charms/trusty/logstash-forwarder/add-logs-relation]

17. By Jay R. Wren on 2015-12-10

cleanup

18. By Jay R. Wren on 2015-12-10

add missing symlinks

[Michael Nelson (michael.nelson)]

Great - thanks Jay.

review: Approve

[Michael Nelson (michael.nelson)]

BTW, you'll need someone from IS (#webops) to land it for you, afaik.

[Greg Mason (gmason)]

LGTM

review: Approve

=== modified file 'config.yaml'
--- config.yaml	2015-11-05 04:16:47 +0000
+++ config.yaml	2015-12-10 18:17:52 +0000
@@ -26,6 +26,9 @@
         Alternatively, this can also be the full list of hashes, as per the
         definition of the files attribute of the logstash-forwarder config at
         https://github.com/elastic/logstash-forwarder
+        If this service is related to another services via the logs relation,
+        the field type and files from the logs relation are appened to this
+        value.
   package_name:
     default: "logstash-forwarder_0.4.0_amd64.deb"
     type: string

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-11-05 04:16:47 +0000
+++ hooks/hooks.py	2015-12-10 18:17:52 +0000
@@ -12,7 +12,9 @@
 import shutil
 sys.path.insert(0, os.path.join(os.environ['CHARM_DIR'], 'lib'))
 from charmhelpers.core.hookenv import Hooks, log, charm_dir, relations_of_type
+from charmhelpers.core import host
 from charmhelpers.core.host import mkdir, service_start, service_stop, service_restart
+from charmhelpers.core.services import RelationContext
 from charmhelpers.fetch import apt_install, apt_update, add_source
 from Cheetah.Template import Template
 from Config import Config
@@ -81,6 +83,7 @@
     config = {}
     servers = []
     files = []
+    logs_relation_data = logs_relation()
     if not os.path.exists(conf.configDir()):
         mkdir(conf.configDir())
     serverPort = conf.serverPort()
@@ -97,6 +100,7 @@
     if conf.timeout():
         config["network"]["timeout"] = conf.timeout()
 
+    config["files"] = []
     fileSpec = conf.files()
     if fileSpec is not None:
         if isinstance(fileSpec, list):
@@ -112,8 +116,12 @@
                 })
             config["files"] = files
 
-    with open(conf.configFile(), "w") as f:
-        f.write(json.dumps(config, sort_keys=True, indent=4))
+    if logs_relation_data is not None:
+        for typ, fil in logs_relation_data:
+            config["files"].append({"paths": [fil], "fields": {"type": typ}})
+
+    host.write_file(conf.configFile(),
+        json.dumps(config, sort_keys=True, indent=4))
 
 
 def writeEtcHosts():
@@ -224,5 +232,36 @@
     service_stop("logstash-forwarder")
 
 
[e-mail]("logs-relation-joined")
[e-mail]('logs-relation-changed')
[e-mail]('logs-relation-departed')
[e-mail]('logs-relation-broken')
+def logs_relation_hooks():
+    log(os.environ['JUJU_HOOK_NAME'])
+    writeConfig()
+    service_restart("logstash-forwarder")
+
+
+def logs_relation():
+    lsr = LogsRelation()
+    log("LogsRelation: {}".format(lsr))
+    if 'logs' not in lsr:
+        return
+    logs_relation_data = lsr['logs']
+    if (not logs_relation_data or
+        'types' not in logs_relation_data[0] or
+        'files' not in logs_relation_data[0]):
+        return None
+    types = logs_relation_data[0]['types'].split()
+    files = logs_relation_data[0]['files'].split()
+    return zip(types, files)
+
+
+class LogsRelation(RelationContext):
+    name = 'logs'
+    interface = 'logs'
+    required_keys = ['types', 'files']
+
+
 if __name__ == "__main__":
     hooks.execute(sys.argv)

=== added symlink 'hooks/logs-relation-broken'
=== target is u'hooks'
=== added symlink 'hooks/logs-relation-changed'
=== target is u'hooks'
=== added symlink 'hooks/logs-relation-departed'
=== target is u'hooks'
=== added symlink 'hooks/logs-relation-joined'
=== target is u'hooks'
=== added file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	1970-01-01 00:00:00 +0000
+++ hooks/test_hooks.py	2015-12-10 18:17:52 +0000
@@ -0,0 +1,94 @@
+#!/usr/bin/python
+
+import json
+import mock
+import os
+import shutil
+import subprocess
+import unittest
+
+os.environ['CHARM_DIR'] = os.path.join(os.path.dirname(__file__), '..')
+os.environ['JUJU_UNIT_NAME'] = 'logstash-forwarder/0'
+
+from charmhelpers.core import hookenv, services
+from charmhelpers.core import host
+import charmhelpers.fetch
+subprocess.check_output = mock.MagicMock()
+subprocess.check_call = mock.MagicMock()
+subprocess.call = mock.MagicMock(return_value=0)
+config = hookenv.Config({"application_name": "logstash_forwarder",
+                         "config_dir": "/etc/logstash-forwarder",
+                         "package_name": "logstash-forwarder_0.4.0_amd64.deb",
+                         })
+hookenv.config = lambda k: config.get(k)
+shutil.copyfile = mock.MagicMock()
+os.chmod = mock.MagicMock()
+
+host.mkdir = mock.MagicMock()
+host.service_start = mock.MagicMock()
+host.service_stop = mock.MagicMock()
+host.service_restart = mock.MagicMock()
+host.write_file = mock.MagicMock()
+hookenv.log = mock.MagicMock()
+
+charmhelpers.fetch.apt_install = mock.MagicMock()
+import hooks
+hooks.conf.config = config
+hooks.log = mock.MagicMock()
+
+
+class TestInstall(unittest.TestCase):
+    '''Testing that there are no exceptions in hooks.install'''
+
+    def test_install_does_not_raise(self):
+        with mock.patch('__builtin__.open'):
+            hooks.install()
+
+    def test_installs_packages(self):
+        subprocess.call.reset()
+        with mock.patch('__builtin__.open'):
+            hooks.install()
+        subprocess.call.assert_called_with(["dpkg", "-i",
+                                            os.path.join(
+                                                hookenv.charm_dir(),
+                                                "files",
+                                                hookenv.config('package_name'))
+                                            ])
+
+    def test_package_file_exist(self):
+        pkgFile = os.path.join(hookenv.charm_dir(), "files",
+                               hookenv.config('package_name'))
+        if not os.path.exists(pkgFile):
+            self.assert_fail()
+
+
+class TestLogsRelation(unittest.TestCase):
+
+    def setUp(self):
+        os.environ['JUJU_HOOK_NAME'] = 'logs-relation-joined'
+        self.phookenv = mock.patch.object(services.helpers, 'hookenv')
+        self.mhookenv = self.phookenv.start()
+        self.mhookenv.relation_ids.return_value = ['baz']
+        self.mhookenv.related_units.side_effect = lambda i: [i + '/0']
+        self.mhookenv.relation_get.side_effect = [
+            {'files': '\n'.join(['file1', 'file2']),
+             'types': '\n'.join(['type1', 'type2'])}
+        ]
+
+    def tearDown(self):
+        self.phookenv.stop()
+        del os.environ['JUJU_HOOK_NAME']
+
+    def test_logs_hooks(self):
+        conf_path = hookenv.config("config_dir") + "/logstash_forwarder.conf"
+        host.write_file.reset_mock()
+        hooks.logs_relation_hooks()
+        self.assertTrue(host.write_file.called)
+        self.assertEqual(conf_path, host.write_file.call_args[0][0])
+        conf = json.loads(host.write_file.call_args[0][1])
+        self.assertEqual("file1", conf["files"][0]["paths"][0])
+        host.service_restart.assert_called_once_with('logstash-forwarder')
+
+    def test_logs_realtion(self):
+        data = hooks.logs_relation()
+        self.assertEqual([('type1', 'file1'), ('type2', 'file2')], data)

=== modified file 'metadata.yaml'
--- metadata.yaml	2015-05-29 04:52:33 +0000
+++ metadata.yaml	2015-12-10 18:17:52 +0000
@@ -4,7 +4,7 @@
 description: |
   This charm installs and configures logstash-forwarder, a lightweight
   alternative to logstash-agent.
-categories:
+tags:
   - miscellaneous
 subordinate: true
 provides:
@@ -15,3 +15,6 @@
   juju-info:
     interface: juju-info
     scope: container
+  logs:
+    interface: logs
+    scope: container