Merge lp:~niedbalski/charms/trusty/rabbitmq-server/fix-lp-1489053 into lp:~openstack-charmers-archive/charms/trusty/rabbitmq-server/next

[Jorge Niedbalski]

- Enforce the update of /etc/hosts file on both cases (now it's just enforced
  on ipv6) for fixing cases like LP: #1489053.

- Remove the socket.gethostname() on cluster_joined relation, this is incorrect
if the intention is to determine the remote unit ip address, instead
uses the charmhelpers.contrib.network.ip.get_hostname method that
makes all the resolution logic.

[Jorge Niedbalski (niedbalski)]

This writes the /etc/hosts file and also makes the cluster relation properly.

https://gist.github.com/niedbalski/5d2ad08efd2af5938862

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #8762 rabbitmq-server-next for niedbalski mp269261
    LINT FAIL: lint-test failed

LINT Results (max last 2 lines):
make: *** [lint] Error 1
ERROR:root:Make target returned non-zero.

Full lint test output: http://paste.ubuntu.com/12202004/
Build: http://10.245.162.77:8080/job/charm_lint_check/8762/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #8093 rabbitmq-server-next for niedbalski mp269261
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/8093/

[Felipe Reyes (freyes)]

Works as expected. Thanks @niedbalski.

 juju run --service rabbit 'cat /etc/hosts'
- MachineId: "10"
  Stderr: "Warning: Permanently added '10.0.3.234' (ECDSA) to the list of known hosts.\r\n"
  Stdout: |
    127.0.0.1 localhost

    # The following lines are desirable for IPv6 capable hosts
    ::1 ip6-localhost ip6-loopback
    fe00::0 ip6-localnet
    ff00::0 ip6-mcastprefix
    ff02::1 ip6-allnodes
    ff02::2 ip6-allrouters
    ff02::3 ip6-allhosts
    10.0.3.194 freyes-lxc-machine-8
    10.0.3.204 freyes-lxc-machine-9
  UnitId: rabbit/2
- MachineId: "8"
  Stderr: "Warning: Permanently added '10.0.3.194' (ECDSA) to the list of known hosts.\r\n"
  Stdout: |
    127.0.0.1 localhost

    # The following lines are desirable for IPv6 capable hosts
    ::1 ip6-localhost ip6-loopback
    fe00::0 ip6-localnet
    ff00::0 ip6-mcastprefix
    ff02::1 ip6-allnodes
    ff02::2 ip6-allrouters
    ff02::3 ip6-allhosts
    10.0.3.204 freyes-lxc-machine-9
    10.0.3.234 freyes-lxc-machine-10
  UnitId: rabbit/0
- MachineId: "9"
  Stderr: "Warning: Permanently added '10.0.3.204' (ECDSA) to the list of known hosts.\r\n"
  Stdout: |
    127.0.0.1 localhost

    # The following lines are desirable for IPv6 capable hosts
    ::1 ip6-localhost ip6-loopback
    fe00::0 ip6-localnet
    ff00::0 ip6-mcastprefix
    ff02::1 ip6-allnodes
    ff02::2 ip6-allrouters
    ff02::3 ip6-allhosts
    10.0.3.194 freyes-lxc-machine-8
    10.0.3.234 freyes-lxc-machine-10
  UnitId: rabbit/1

review: Approve

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #6039 rabbitmq-server-next for niedbalski mp269261
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/6039/

[Billy Olsen (billy-olsen)]

I think this change makes sense, especially in the general sense (not just in the OpenStack specific use case). For the openstack deployment case, dns resolution needs to be available for other services to properly work - however it has been argued (and I tend to agree) that simply deploying a rabbitmq-server cluster should "just work".

There's a minor lint issue that needs to be resolved, but other than that I'm happy to land this in the /next branches.

review: Needs Fixing

[Jorge Niedbalski (niedbalski)]

Fixed the lint issue.

[Billy Olsen (billy-olsen)]

Thanks for fixing this Jorge.

review: Approve

[Ryan Beisner (1chb1n)]

+1 to preserving at least the existing level of user feedback.

review: Needs Resubmitting

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #9164 rabbitmq-server-next for niedbalski mp269261
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/9164/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #8469 rabbitmq-server-next for niedbalski mp269261
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/8469/

[Ryan Beisner (1chb1n)]

Removal of this conditional:
83 - if config('prefer-ipv6') and rdata.get('hostname'):

Makes this:
hostname = rdata.get('hostname', None)

Throw error:
http://paste.ubuntu.com/12242485/

2015-09-01 01:24:22 INFO leader-settings-changed hostname = rdata.get('hostname', None)
2015-09-01 01:24:22 INFO leader-settings-changed AttributeError: 'NoneType' object has no attribute 'get'

review: Needs Fixing

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #6179 rabbitmq-server-next for niedbalski mp269261
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/6179/

[Jorge Niedbalski (niedbalski)]

Fixed both comments, @beisner and @dosaboy.

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #8476 rabbitmq-server-next for niedbalski mp269261
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/8476/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #9172 rabbitmq-server-next for niedbalski mp269261
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/9172/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #8478 rabbitmq-server-next for niedbalski mp269801
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/8478/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #9174 rabbitmq-server-next for niedbalski mp269801
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/9174/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #6181 rabbitmq-server-next for niedbalski mp269261
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 124
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/12248128/
Build: http://10.245.162.77:8080/job/charm_amulet_test/6181/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #6183 rabbitmq-server-next for niedbalski mp269801
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/6183/

[Ryan Beisner (1chb1n)]

Take note that the amulet results here are prone to representing false-passes, and should be ignored.

As discussed on irc, we will revisit this proposal after landing fixes for the critical bug 1486177, including refactored amulet tests.

Thanks for your patience, and for your work on this.

[lp:~niedbalski/charms/trusty/rabbitmq-server/fix-lp-1489053]

110. By Liam Young on 2015-09-03

[gnuoy,trivial] Charmhelper sync (+1'd by mojo)

111. By Liam Young on 2015-09-09

[thedac, r=gnuoy] Fix clustering race conditions for LP Bug#1486177

112. By Liam Young on 2015-09-09

[1chb1n, r=gnuoy] Refactor amulet tests, deprecate old tests

113. By Jorge Niedbalski on 2015-09-16

Rebased again

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #9295 rabbitmq-server-next for niedbalski mp269801
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/9295/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #10133 rabbitmq-server-next for niedbalski mp269801
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/10133/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #6470 rabbitmq-server-next for niedbalski mp269801
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/12428869/
Build: http://10.245.162.77:8080/job/charm_amulet_test/6470/

[Billy Olsen (billy-olsen)]

Looking through the amulet results, it seems that these tests are still somewhat racey. From what I can tell, it looks like there was a failure to cluster - but the code makes an assumption that the rabbit service is up at the time. That appears to be a separate issue outside of this particular merge proposal.

As an example, running the tests in sts-stack results in a successful pass for all tests as seen http://paste.ubuntu.com/12439865/ (note: just the latest results are shown).

Moving on to the code itself, the code itself looks good and brings the ipv4 and ipv6 paths into the same path, thus I'm happy to merge this.

review: Approve

[Jorge Niedbalski (niedbalski)]

Thanks Billy!

=== modified file 'hooks/rabbit_utils.py'
--- hooks/rabbit_utils.py	2015-09-03 17:08:03 +0000
+++ hooks/rabbit_utils.py	2015-09-16 17:58:05 +0000
@@ -2,7 +2,6 @@
 import pwd
 import grp
 import re
-import socket
 import sys
 import subprocess
 import glob
@@ -270,18 +269,13 @@
                 address = relation_get('private-address',
                                        rid=r_id, unit=unit)
             if address is not None:
-                try:
-                    node = get_hostname(address, fqdn=False)
-                except:
+                node = get_hostname(address, fqdn=False)
+                if node:
+                    available_nodes.append(node)
+                else:
                     log('Cannot resolve hostname for {} '
-                        'using DNS servers'.format(address), level='WARNING')
-                    log('Falling back to use socket.gethostname()',
+                        'using DNS servers'.format(address),
                         level='WARNING')
-                    # If the private-address is not resolvable using DNS
-                    # then use the current hostname
-                    node = socket.gethostname()
-
-                available_nodes.append(node)
 
     if len(available_nodes) == 0:
         log('No nodes available to cluster with')

=== modified file 'hooks/rabbitmq_server_relations.py'
--- hooks/rabbitmq_server_relations.py	2015-09-05 01:41:04 +0000
+++ hooks/rabbitmq_server_relations.py	2015-09-16 17:58:05 +0000
@@ -273,11 +273,18 @@
 
 @hooks.hook('cluster-relation-joined')
 def cluster_joined(relation_id=None):
+    relation_settings = {
+        'hostname': get_local_nodename(),
+    }
+
     if config('prefer-ipv6'):
-        relation_settings = {'hostname': socket.gethostname(),
-                             'private-address': get_ipv6_addr()[0]}
-        relation_set(relation_id=relation_id,
-                     relation_settings=relation_settings)
+        relation_settings['private-address'] = get_ipv6_addr()[0]
+    else:
+        relation_settings['private-address'] = get_host_ip(
+            unit_get('private-address'))
+
+    relation_set(relation_id=relation_id,
+                 relation_settings=relation_settings)
 
     if is_relation_made('ha') and \
             config('ha-vip-only') is False:
@@ -324,10 +331,12 @@
         log('cluster_joined: cookie not yet set.', level=INFO)
         return
 
-    if config('prefer-ipv6') and rdata.get('hostname'):
-        private_address = rdata.get('private-address')
-        hostname = rdata.get('hostname')
-        if hostname:
+    rdata = relation_get()
+    if rdata:
+        hostname = rdata.get('hostname', None)
+        private_address = rdata.get('private-address', None)
+
+        if hostname and private_address:
             rabbit.update_hosts_file({private_address: hostname})
 
     if not is_sufficient_peers():