Merge lp://staging/~dpb/charms/precise/haproxy/fix-service-entries into lp://staging/charms/haproxy

Hi David, thanks for the branch. A few comments below.

43 + service = {}
44 + # First come, first serve, make sure all options other than
45 + # "servers" are represented in the combined dict
46 + for key in old_service.keys():
47 + service[key] = old_service[key]

This can be rewritten as:

  service = old_service.copy()

56 + service["servers"] = old_servers + new_servers

This doesn't account for duplicate server entries. I would deduplicate on (ip, port). The trick is if options differ for two definitions of the same (ip, port) combination, what to do so it's deterministic. Maybe sort on the whole tuple contents and pick first?

FWIW, using this branch I get the second unit added correctly to haproxy:
--- /etc/haproxy/haproxy.cfg.one-unit 2014-01-21 12:14:08.228486964 +0000
+++ /etc/haproxy/haproxy.cfg 2014-01-21 12:16:45.186927000 +0000
@@ -28,6 +28,8 @@
     option httpchk GET / HTTP/1.0
     server message 10.55.32.209:80 check inter 5000 rise 2 fall 5 maxconn 4
     server message 10.55.32.209:81 check inter 5000 rise 2 fall 5 maxconn 4
+ server message 10.55.32.215:80 check inter 5000 rise 2 fall 5 maxconn 4
+ server message 10.55.32.215:81 check inter 5000 rise 2 fall 5 maxconn 4

 frontend haproxy-0-80
     bind 0.0.0.0:80

It also supported removing a unit correctly:
--- /etc/haproxy/haproxy.cfg.two-units 2014-01-21 12:18:58.645623297 +0000
+++ /etc/haproxy/haproxy.cfg 2014-01-21 12:19:05.136867001 +0000
@@ -28,8 +28,6 @@
     option httpchk GET / HTTP/1.0
     server message 10.55.32.209:80 check inter 5000 rise 2 fall 5 maxconn 4
     server message 10.55.32.209:81 check inter 5000 rise 2 fall 5 maxconn 4
- server message 10.55.32.215:80 check inter 5000 rise 2 fall 5 maxconn 4
- server message 10.55.32.215:81 check inter 5000 rise 2 fall 5 maxconn 4

 frontend haproxy-0-80
     bind 0.0.0.0:80

It also supported the adding of two units at once:
--- /etc/haproxy/haproxy.cfg.one-unit 2014-01-21 12:14:08.228486964 +0000
+++ /etc/haproxy/haproxy.cfg 2014-01-21 12:27:24.239249653 +0000
@@ -28,6 +28,10 @@
     option httpchk GET / HTTP/1.0
     server message 10.55.32.209:80 check inter 5000 rise 2 fall 5 maxconn 4
     server message 10.55.32.209:81 check inter 5000 rise 2 fall 5 maxconn 4
+ server message 10.55.32.106:80 check inter 5000 rise 2 fall 5 maxconn 4
+ server message 10.55.32.106:81 check inter 5000 rise 2 fall 5 maxconn 4
+ server message 10.55.32.203:80 check inter 5000 rise 2 fall 5 maxconn 4
+ server message 10.55.32.203:81 check inter 5000 rise 2 fall 5 maxconn 4

 frontend haproxy-0-80
     bind 0.0.0.0:80

Thanks a lot for the review, Sidnei. Here is my response:

On Tue, Jan 21, 2014 at 5:10 AM, Sidnei da Silva <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> Hi David, thanks for the branch. A few comments below.
>
> 43 + service = {}
> 44 + # First come, first serve, make sure all options other than
> 45 + # "servers" are represented in the combined dict
> 46 + for key in old_service.keys():
> 47 + service[key] = old_service[key]
>
> This can be rewritten as:
>
> service = old_service.copy()
>
>
Done, and further optimized with a copy() and update() in the right order.
 I think it is much more readable now.

> 56 + service["servers"] = old_servers + new_servers
>
> This doesn't account for duplicate server entries. I would deduplicate on
> (ip, port). The trick is if options differ for two definitions of the same
> (ip, port) combination, what to do so it's deterministic. Maybe sort on the
> whole tuple contents and pick first?
>

After our discussion in IRC, we decided to removed strict duplicates only
(since ip/port duplication is supported), but strict duplicates are likely
a programming error or a configuration error by the admin/charm. I have
updated this code as well.

There are test cases for everything I added, please re-review when you get
a chance.

Thanks!

--
David Britton <email address hidden>

LGTM +1