Merge lp:~verterok/charms/trusty/haproxy/restart-rsyslog-after-install into lp:charms/trusty/haproxy

[Guillermo Gonzalez]

Update default global_log and add a restart of rsyslog at the end of install_hook to make logging work by default.

Due to bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=790871, after installing the haproxy package we end up without getting any logs to rsyslog.
This branch restarts rsyslog service at the end of install_hook.
Also, for logging to work out of the box, I had to change the default global_log config to use /dev/log instead of 127.0.0.1

[Andrew McLeod (admcleod)]

Hi Guillermo,

I manually deployed as per the amulet test and: haproxy restart worked, socketfile defined in /etc/rsyslog.d/haproxy.conf exists, /var/log/haproxy.log exists,

However, I ran into problems with running bundletester against this update:

Checking for Python syntax...
/tmp/bundletester-DsZsMg/haproxy/hooks/tests/test_reverseproxy_hooks.py:17:80: E501 line too long (101 > 79 characters)
make: *** [lint] Error 1

I fixed this manually and then ran into this:

http://pastebin.ubuntu.com/12207030/

I managed to get past these issues once (randomly), and although 49-haproxy.conf exists and the restart occurs according to debug logs, there is no socketfile and no /var/log/haproxy.log.

http://pastebin.ubuntu.com/12207109/ (clock differences in container and local machine)

Looks like its probably just a test problem which needs to be resolved

review: Needs Fixing

[Guillermo Gonzalez (verterok)]

Hi,
Thanks for the prompt review.
Will work on fixing those issues, marking this as work in progress.

Cheers,

[Guillermo Gonzalez (verterok)]

Fixed some lint issues and install test to patch hooks.lsb_release (to make it work on != trusty).
Also had to change the Makefile to just install dependencies from the archive, as the pypi ones were not working.

Installed bundletester and everything seems to work, didn't changed anything related to this MP :/

The logs of bundleteser run: http://paste.ubuntu.com/12245509/

And the log file is there, also the socket: http://paste.ubuntu.com/12245510/

Let me know if I missed something.

Cheers,

[Kevin W Monroe (kwmonroe)]

Hey Guillermo, thanks for the work to make haproxy better! I tried to review your changes, but could not get the tests to pass. I tried 'make tests', '.venv/bin/bundletester -l DEBUG', and 'bundletester -l DEBUG' (to use my system bundletester vs the .venv version). In all three cases, I get the following when make hits the 'test' target:

http://paste.ubuntu.com/12263768/

Does this look familiar to you? I'm curious how you were able to get a successful run when mine is failing so miserably. I'm reviewing as 'Needs Fixing' for now, but hope there's a simple environment difference that I just don't have.

review: Needs Fixing

[Guillermo Gonzalez (verterok)]

Hi Kevin,

> Hey Guillermo, thanks for the work to make haproxy better! I tried to review
> your changes, but could not get the tests to pass. I tried 'make tests',
> '.venv/bin/bundletester -l DEBUG', and 'bundletester -l DEBUG' (to use my
> system bundletester vs the .venv version). In all three cases, I get the
> following when make hits the 'test' target:
>
> http://paste.ubuntu.com/12263768/
>
> Does this look familiar to you? I'm curious how you were able to get a
> successful run when mine is failing so miserably. I'm reviewing as 'Needs
> Fixing' for now, but hope there's a simple environment difference that I just
> don't have.

I'm running make test in vivid (host) and trusty (lxc), the trusty lxc is a vanialla (just created for this)

The bundletester run is done using the version from pypi (via the virtualenv), in the vivid host using the local provider.

Hope this helps, if not please let me know in what environment you'r running make test and I'll try to replicate.

One thing that might help, rm .venv; and then make test.

Cheers

[lp:~verterok/charms/trusty/haproxy/restart-rsyslog-after-install]

104. By Guillermo Gonzalez on 2015-09-04

- fix tests to work latest mock version
- add python-yaml dep

[Guillermo Gonzalez (verterok)]

Just pushed some fixes to the tests to work with both mock versions trusty/vivid and pypi

[lp:~verterok/charms/trusty/haproxy/restart-rsyslog-after-install]

105. By Guillermo Gonzalez on 2015-09-23

Rework rsyslog restart when installing/updating haproxy, required when changing the source config and the package is down|upgraded.

[Cory Johns (johnsca)]

Guillermo,

There was an outstanding lint error, and when running the 10_deploy_test.py the haproxy unit went into an error state on config-changed, even though the test passed.

The error was due to your change using the wrong indentation level and changing the logic of the surrounding if / else block. The test didn't pick it up because it was missing calls to d.sentry.wait().

I've submitted an MP with all of these issues fixed, and once that is merged, I'd give this change my +1: https://code.launchpad.net/~johnsca/charms/trusty/haproxy/verterok-restart-rsyslog-after-install/+merge/273867

review: Needs Fixing

[lp:~verterok/charms/trusty/haproxy/restart-rsyslog-after-install]

106. By Cory Johns on 2015-11-04

Fixed config-changed hook failure, test not catching aforementioned error, and minor lint issue.

107. By Guillermo Gonzalez on 2015-11-04

move rsyslog restart condition outside of the check if/else block

108. By Guillermo Gonzalez on 2015-11-04

fix test to avoid restarting host rsyslog

[Guillermo Gonzalez (verterok)]

Pushed a fix for the upgrade-charm case, moved the restart block outside the last if/else.
Also fixed a test that was trying to restart the host rsyslog

P.S: tested on vivid using: make buid; source .venv/bin/activate; bundletester

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1408/

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

The results (PASS) are in and available here: http://juju-ci.vapour.ws:8080/job/charm-bundle-test-hp/1119/

review: Approve (automated testing)

[Review Queue (review-queue)]

The results (PASS) are in and available here: http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1419/

review: Approve (automated testing)

[Andrew McLeod (admcleod)]

Hi Guillermo,

I've manually bundletested this, everything passed first time - I was then able to generate live http traffic and watch it being logged into an rsyslog handled file, so the restart of rsyslog was successful.

Approve

review: Approve

[Kevin W Monroe (kwmonroe)]

Hi Guillermo,

In juju-1.25, the first unit number for a service may not always be 0. If you deploy haproxy for the first time, you'll get haproxy/0. If you then destroy haproxy and re-deploy it, you'll get haproxy/1. Because of this, I tweaked your 10_deploy_test.py like this:

-haproxy_unit = d.sentry.unit['haproxy/0']
+haproxy_unit = d.sentry['haproxy'][0]

-apache_unit = d.sentry.unit['apache2/0']
+apache_unit = d.sentry['apache2'][0]

-apache_unit2 = d.sentry.unit['apache2/1']
+apache_unit2 = d.sentry['apache2'][1]

The [0] bracket notation ensures you always get the 1st unit; similarly, [1] gives you the 2nd unit, no matter what the actual unit number may be.

I also added python-flake8 to your list of apt packages to make sure lint would be successful on systems without flake8.

Since these were such trivial changes, I made them while merging into the promulgated haproxy charm. The charm store should have an updated haproxy shortly that contains your changes + these tweaks.

Everything LGTM, so a big thanks to you for this proposal and for your patience during the review process!

review: Approve

=== modified file 'Makefile'
--- Makefile	2015-05-15 16:54:14 +0000
+++ Makefile	2015-11-04 14:12:30 +0000
@@ -14,13 +14,13 @@
 	@charm proof
 
 .venv:
-	sudo apt-get install -y python-apt python-virtualenv python-jinja2
+	sudo apt-get install -y python-apt python-virtualenv python-jinja2 python-mock python-yaml python-testtools python-nose python-yaml
 	virtualenv .venv --system-site-packages
-	.venv/bin/pip install -I nose testtools mock pyyaml
+	.venv/bin/pip install bundletester
 
 test: .venv
 	@echo Starting tests...
-	@CHARM_DIR=$(CHARM_DIR) $(TEST_PREFIX) .venv/bin/nosetests -s $(TEST_DIR)
+	@CHARM_DIR=$(CHARM_DIR) $(TEST_PREFIX) nosetests -s $(TEST_DIR)
 
 lint:
 	@echo Checking for Python syntax...

=== modified file 'config.yaml'
--- config.yaml	2015-08-17 12:58:35 +0000
+++ config.yaml	2015-11-04 14:12:30 +0000
@@ -1,6 +1,6 @@
 options:
   global_log:
-    default: "127.0.0.1 local0, 127.0.0.1 local1 notice"
+    default: "/dev/log local0, /dev/log local1 notice"
     type: string
     description: Global log line ( multiples ... comma separated list )
   global_maxconn:

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-10-16 09:39:42 +0000
+++ hooks/hooks.py	2015-11-04 14:12:30 +0000
@@ -14,7 +14,7 @@
 from itertools import izip, tee
 from operator import itemgetter
 
-from charmhelpers.core.host import pwgen, lsb_release
+from charmhelpers.core.host import pwgen, lsb_release, service_restart
 from charmhelpers.core.hookenv import (
     log,
     config as config_get,
@@ -963,6 +963,12 @@
         # to be broken.
         log("HAProxy configuration check failed, exiting.")
         sys.exit(1)
+    if config_data.changed("global_log") or config_data.changed("source"):
+        # restart rsyslog to pickup haproxy rsyslog config
+        # This could be removed once the following bug is fixed in the haproxy
+        # package:
+        #   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=790871
+        service_restart("rsyslog")
 
 
 def start_hook():

=== modified file 'hooks/tests/test_config_changed_hooks.py'
--- hooks/tests/test_config_changed_hooks.py	2015-04-14 14:21:45 +0000
+++ hooks/tests/test_config_changed_hooks.py	2015-11-04 14:12:30 +0000
@@ -3,7 +3,7 @@
 import os
 
 from testtools import TestCase
-from mock import patch
+from mock import patch, call
 
 import hooks
 from utils_for_tests import patch_open
@@ -92,10 +92,36 @@
         config_data = self.config_get()
         config_data.changed.return_value = True
         _notify_reverseproxy = self.patch_hook("_notify_reverseproxy")
-
-        hooks.config_changed()
-        config_data.changed.assert_called_once_with("ssl_cert")
-        _notify_reverseproxy.assert_called_once()
+        service_restart = self.patch_hook('service_restart')
+
+        hooks.config_changed()
+        self.assertIn(call('ssl_cert'), config_data.changed.mock_calls)
+        _notify_reverseproxy.assert_called_once_with()
+        service_restart.assert_called_once_with('rsyslog')
+
+    def test_config_changed_restart_rsyslog(self):
+        """
+        If the gloabl_log or source config value changes, rsyslog is
+        restarted
+        """
+        config_data = self.config_get()
+        called = []
+
+        def changed(a):
+            if a in called or a == 'ssl_cert':
+                return False
+            called.append(a)
+            return True
+
+        config_data.changed.side_effect = changed
+        service_restart = self.patch_hook('service_restart')
+
+        hooks.config_changed()
+        self.assertIn(call('global_log'), config_data.changed.mock_calls)
+        service_restart.assert_called_once_with('rsyslog')
+        hooks.config_changed()
+        self.assertIn(call('source'), config_data.changed.mock_calls)
+        service_restart.assert_called_with('rsyslog')
 
 
 class HelpersTest(TestCase):

=== modified file 'hooks/tests/test_install.py'
--- hooks/tests/test_install.py	2015-02-09 12:53:57 +0000
+++ hooks/tests/test_install.py	2015-11-04 14:12:30 +0000
@@ -53,7 +53,7 @@
 
     def test_add_source(self):
         hooks.install_hook()
-        self.config_get.assert_called_once()
+        self.config_get.assert_called_once_with()
         self.add_source.assert_called_once_with(
             self.config_get.return_value.get("source"),
             self.config_get.return_value.get("key"))
@@ -65,12 +65,12 @@
     def test_add_source_with_backports(self):
         self.config_get.return_value = {
             'source': 'backports', 'package_status': 'install'}
-        with patch("charmhelpers.core.host.lsb_release") as lsb_release:
+        with patch("hooks.lsb_release") as lsb_release:
             lsb_release.return_value = {'DISTRIB_CODENAME': 'trusty'}
             with patch("hooks.add_backports_preferences") as add_apt_prefs:
-                add_apt_prefs.assert_called_once()
                 hooks.install_hook()
-        self.config_get.assert_called_once()
+                add_apt_prefs.assert_called_once_with('trusty')
+        self.config_get.assert_called_once_with()
         source = ("deb http://archive.ubuntu.com/ubuntu trusty-backports "
                   "main restricted universe multiverse")
         self.add_source.assert_called_once_with(
@@ -79,7 +79,7 @@
 
     def test_ensures_package_status(self):
         hooks.install_hook()
-        self.config_get.assert_called_once()
+        self.config_get.assert_called_once_with()
         self.ensure_package_status.assert_called_once_with(
             hooks.service_affecting_packages,
             self.config_get.return_value["package_status"])

=== modified file 'hooks/tests/test_reverseproxy_hooks.py'
--- hooks/tests/test_reverseproxy_hooks.py	2015-08-17 12:58:35 +0000
+++ hooks/tests/test_reverseproxy_hooks.py	2015-11-04 14:12:30 +0000
@@ -14,7 +14,8 @@
         super(ReverseProxyRelationTest, self).setUp()
 
         self.config_get = self.patch_hook("config_get")
-        self.config_get.return_value = {"monitoring_port": "10000", "peering_mode": "active-passive"}
+        self.config_get.return_value = {"monitoring_port": "10000",
+                                        "peering_mode": "active-passive"}
         self.relations_of_type = self.patch_hook("relations_of_type")
         self.get_config_services = self.patch_hook("get_config_services")
         self.log = self.patch_hook("log")

=== modified file 'hooks/tests/test_website_hooks.py'
--- hooks/tests/test_website_hooks.py	2013-11-04 13:09:04 +0000
+++ hooks/tests/test_website_hooks.py	2015-11-04 14:12:30 +0000
@@ -146,4 +146,4 @@
                 relation_id="website:1", port="4242", hostname="bar.local",
                 all_services=""),
             ])
-        self.log.assert_not_called()
+        self.log.assert_has_calls([call("No services configured, exiting.")])

=== modified file 'tests/10_deploy_test.py'
--- tests/10_deploy_test.py	2015-03-18 20:28:29 +0000
+++ tests/10_deploy_test.py	2015-11-04 14:12:30 +0000
@@ -105,6 +105,8 @@
          'crts': ['DEFAULT'],
          'servers': [['apache', apache_private, 80, 'maxconn 50']]}])
 })
+time.sleep(10)
+d.sentry.wait(seconds)
 
 # We need a retry loop here, since there's no way to tell when the new
 # configuration is in place.
@@ -151,6 +153,8 @@
                   ['apache2', apache_private2, 80, 'maxconn 50']]}
          ]}])
 })
+time.sleep(10)
+d.sentry.wait(seconds)
 
 # Let's exercise our URL-based routing by trying to fetch a URL that will
 # only work for the second apache unit (which is configured as server