Merge lp:~danilo/charms/trusty/haproxy/merge-services-fix into lp:charms/trusty/haproxy

[Данило Шеган]

Merge service backends properly even when new service has fewer backends than the old one

This is a problem we've noticed with Landscape deployments (trying to use multiple backends per unit), and we've started hitting this reliably (see linked bug).

The solution is simple: do not attempt to merge new_service backends which might not exist. Test is included.

Fix has been tested and works properly with landscape-charm at least.

FWIW, I've confirmed on haproxy-unit-ip:10000 (user haproxy, pass haproxy by default) that it does indeed work properly.

[Björn Tillenius (bjornt)]

review: Needs Information

[lp:~danilo/charms/trusty/haproxy/merge-services-fix]

94. By Данило Шеган on 2015-05-15

Refactor merge_service and improve test.

95. By Данило Шеган on 2015-05-15

Drop useless sorting of lists containing dicts.

96. By Данило Шеган on 2015-05-15

Add exception tests.

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

I kind of tried to stay away from refactoring the method: I've suspected the problem already, but got tricked by my tests using assertItemsEqual which always passed :/

Now I do a full refactor, stressing readability and correctness over concise code (eg. even dropping use of groupby() to weed out duplicate entries).

[lp:~danilo/charms/trusty/haproxy/merge-services-fix]

97. By Данило Шеган on 2015-05-15

Uncomment apt-get that got in by accident.

[Björn Tillenius (bjornt)]

Thanks for doing the refactoring. It looks much better now, but there are still some minor issues. +1 after you've addressed them.

review: Approve

[Björn Tillenius (bjornt)]

I've addressed the review comments here:

 lp:~bjornt/charms/trusty/haproxy/merge-services-fix/

Please merge it in before landing.

[Whit Morriss (whitmo)]

Gave each branch a lookover and tested merging them and running tests

 - tests are missing an install for flake8
 - lint returns following errors:
     F401 'groupby' imported but unused
     E111 indentation is not a multiple of four

Deployment tests pass. Merge the Bjorn's branch and fix up the flake8 install and minor lint errors and should be good to go.

review: Needs Fixing

[Björn Tillenius (bjornt)]

> Gave each branch a lookover and tested merging them and running tests
>
> - tests are missing an install for flake8
> - lint returns following errors:
> F401 'groupby' imported but unused
> E111 indentation is not a multiple of four
>
> Deployment tests pass. Merge the Bjorn's branch and fix up the flake8 install
> and minor lint errors and should be good to go.

Thanks for the review Whit. I fixed the lint added python-flake8 to be installed
in my branch, since Danilo is away.

[Whit Morriss (whitmo)]

Thanks Bjorn!

-w

On Fri, May 15, 2015 at 11:40 AM, Björn Tillenius <email address hidden>
wrote:

> > Gave each branch a lookover and tested merging them and running tests
> >
> > - tests are missing an install for flake8
> > - lint returns following errors:
> > F401 'groupby' imported but unused
> > E111 indentation is not a multiple of four
> >
> > Deployment tests pass. Merge the Bjorn's branch and fix up the flake8
> install
> > and minor lint errors and should be good to go.
>
> Thanks for the review Whit. I fixed the lint added python-flake8 to be
> installed
> in my branch, since Danilo is away.
> --
>
> https://code.launchpad.net/~danilo/charms/trusty/haproxy/merge-services-fix/+merge/259233
> You are reviewing the proposed merge of
> lp:~danilo/charms/trusty/haproxy/merge-services-fix into
> lp:charms/trusty/haproxy.
>

--
---------------
D. Whit Morriss
Developer, Juju Ecosystem
Canonical USA

[David Britton (dpb)]

review: Approve

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

I was going to merge Bjorn's branch, but I see that it's all been handled. Thanks all!

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-04-14 16:46:13 +0000
+++ hooks/hooks.py	2015-05-15 14:11:47 +0000
@@ -12,6 +12,7 @@
 import pwd
 
 from itertools import izip, tee, groupby
+from operator import itemgetter
 
 from charmhelpers.core.host import pwgen, lsb_release
 from charmhelpers.core.hookenv import (
@@ -96,6 +97,10 @@
     ]
 
 
+class InvalidRelationDataError(Exception):
+    """Invalid data has been provided in the relation."""
+
+
 # #############################################################################
 # Supporting functions
 # #############################################################################
@@ -496,30 +501,61 @@
     return services
 
 
+def _add_items_if_missing(target, additions):
+    """
+    Append items from `additions` to `target` if they are not present already.
+
+    Returns a new list.
+    """
+    result = target[:]
+    for addition in additions:
+        if addition not in result:
+           result.append(addition)
+    return result
+
+
 def merge_service(old_service, new_service):
     """
-    Helper function to merge two serivce entries correctly.
+    Helper function to merge two service entries correctly.
     Everything will get trampled (preferring old_service), except "servers"
     which will be unioned acrosss both entries, stripping strict dups.
     """
-    service = {}
     service = new_service.copy()
     service.update(old_service)
-    if "servers" in service:
-        # Merge all 'servers' entries of the default backend
-        servers = service["servers"]
-        if "servers" in new_service:
-            servers.extend(new_service["servers"])
-            servers.sort()
-            service["servers"] = list(x for x, _ in groupby(servers))
-    if "backends" in service and "backends" in new_service:
-        # Merge all 'servers' entries of the additional backends
-        for i, backend in enumerate(service["backends"]):
-            servers = backend["servers"]
-            servers.extend(new_service["backends"][i]["servers"])
-            servers.sort()
-            service["backends"][i]["servers"] = list(
-                x for x, _ in groupby(servers))
+
+    # Merge all 'servers' entries of the default backend.
+    if "servers" in old_service and "servers" in new_service:
+        service["servers"] = _add_items_if_missing(
+            old_service["servers"], new_service["servers"])
+
+    # Merge all 'backends' and their contained "servers".
+    if "backends" in old_service and "backends" in new_service:
+        backends_by_name = {}
+        # Populate backends_by_name with backends from (old_)service.
+        for backend in service["backends"]:
+            backend_name = backend.get("backend_name")
+            if backend_name is None:
+                raise InvalidRelationDataError(
+                    "Each backend must have backend_name.")
+            backends_by_name.setdefault(backend_name, backend)
+
+        # Go through backends in new_service and add them to backends_by_name,
+        # merging 'servers' while at it.
+        for backend in new_service["backends"]:
+            backend_name = backend.get("backend_name")
+            if backend_name is None:
+                raise InvalidRelationDataError(
+                    "Each backend must have backend_name.")
+            if backend_name in backends_by_name:
+                # Merge servers.
+                target_backend = backends_by_name[backend_name]
+                target_backend["servers"] = _add_items_if_missing(
+                    target_backend["servers"], backend["servers"])
+            else:
+                backends_by_name[backend_name] = backend
+
+        service["backends"] = sorted(
+            backends_by_name.values(), key=itemgetter('backend_name'))
     return service
 
 

=== modified file 'hooks/tests/test_reverseproxy_hooks.py'
--- hooks/tests/test_reverseproxy_hooks.py	2015-04-14 14:21:45 +0000
+++ hooks/tests/test_reverseproxy_hooks.py	2015-05-15 14:11:47 +0000
@@ -489,6 +489,46 @@
         expected = {'service_name': 'left', 'foo': 'bar', 'bar': 'baz'}
         self.assertEqual(expected, hooks.merge_service(s1, s2))
 
+    def test_merge_service_old_backend_without_name(self):
+        """Backends in old_service without name raise an exception."""
+
+        s1 = {'backends': [{'servers': []}]}
+        s2 = {'backends': []}
+        self.assertRaises(
+            hooks.InvalidRelationDataError, hooks.merge_service, s1, s2)
+
+    def test_merge_service_new_backend_without_name(self):
+        """Backends in new_service without name raise an exception."""
+
+        s1 = {'backends': []}
+        s2 = {'backends': [{'servers': []}]}
+        self.assertRaises(
+            hooks.InvalidRelationDataError, hooks.merge_service, s1, s2)
+
+    def test_merge_service_backend_name_matching(self):
+        """Backends are merged by backend_name."""
+
+        s1 = {'backends': [
+            {'backend_name': 'api',
+             'servers': [['api-0', '10.0.0.1', 9080, []]]},
+            {'backend_name': 'webapp',
+             'servers': [['webapp-0', '10.0.0.1', 8090, []]]},
+        ]}
+        s2 = {'backends': [
+            {'backend_name': 'webapp',
+             'servers': [['webapp-1', '10.0.0.2', 8090, []]]},
+        ]}
+        expected = {
+            'backends': [
+                {'backend_name': 'api',
+                 'servers': [['api-0', '10.0.0.1', 9080, []]]},
+                {'backend_name': 'webapp',
+                 'servers': [['webapp-0', '10.0.0.1', 8090, []],
+                             ['webapp-1', '10.0.0.2', 8090, []]]},
+            ]
+        }
+        self.assertEqual(expected, hooks.merge_service(s1, s2))
+
     def test_join_reverseproxy_relation(self):
         """
         When haproxy joins a reverseproxy relation it advertises its public