Merge lp:~james-page/charms/trusty/rabbitmq-server/network-splits into lp:~openstack-charmers-archive/charms/trusty/rabbitmq-server/next

[James Page]

Add support for separate 'access-network' configuration.

[Tim Van Steenburgh (tvansteenburgh)]

Hi James,

Thanks for this. Code review looks good, but I'm unsure how to do a functional test of this, can you provide instructions?

Also, there are some charm-proof warnings to clean up:

W: config.yaml: option access-network does not have the keys: default
W: config.yaml: option key does not have the keys: default
W: config.yaml: option source does not have the keys: default

Thanks!

review: Needs Fixing

[James Page (james-page)]

Hi Tim

https://code.launchpad.net/~openstack-charmers/+junk/network-splits contains some helpers for testing; however basically you need servers with more than one configured network interface.

RE the charm proof warning; not touching those - not having a default key is intentional 'unset' being a valid state. The code works on the fact that a config-get on unset keys returns None in JSON.

review: Needs Resubmitting

[Cory Johns (johnsca)]

James,

I believe that Tim's point was that the policy was that you still need to provide a default value, even if that value is an empty string. No warnings on `charm proof` is listed on the policy: https://juju.ubuntu.com/docs/authors-charm-policy.html

Also, my understanding of the "Resubmit" status was that the review was rejected and would need to be resubmitted to be approved; is that not the case?

review: Needs Fixing

[James Page (james-page)]

charm proof is wrong - sorry I've discussed this numerous times with Marco - not providing a default value is a perfectly acceptable situation and is used extensively in my charms - Marco did at least drop this to a warning rather than an error but I'm not re-writing all of my charms to comply with a policy that I believe is wrong in this case.

review: Needs Resubmitting

[James Page (james-page)]

I've raised https://bugs.launchpad.net/charm-tools/+bug/1368056 to cover this.

[James Page (james-page)]

After some discussion with Marco on IRC, he's provided me with a way to have the key, but still leave the default unset; I've updated the charm to reflect this.

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

charm_lint_check #195 trusty-rabbitmq-server for james-page mp228152
    LINT OK: passed

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

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

charm_unit_test #224 trusty-rabbitmq-server for james-page mp228152
    UNIT FAIL: unit-test missing

UNIT Results (max last 2 lines):
INFO:root:Search string not found in makefile target commands.
ERROR:root:No make target was executed.

Full unit test output: http://paste.ubuntu.com/9542548/
Build: http://10.245.162.77:8080/job/charm_unit_test/224/

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

charm_amulet_test #244 trusty-rabbitmq-server for james-page mp228152
    AMULET FAIL: amulet-test missing

AMULET Results (max last 2 lines):
INFO:root:Search string not found in makefile target commands.
ERROR:root:No make target was executed.

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

[Review Queue (review-queue)]

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-10759-results

review: Needs Fixing (automated testing)

[Cory Johns (johnsca)]

Test failure is due to a merge conflict, mainly in charmhelpers sync, but there are a couple in the charm itself, as well. I tried my hand at resolving, but there was a bit more than I felt comfortable with making decisions on without deeper knowledge of the charm.

review: Needs Fixing

[lp:~james-page/charms/trusty/rabbitmq-server/network-splits]

73. By James Page on 2015-01-23

Optimize calls

74. By James Page on 2015-01-23

Re-instate configuration

75. By James Page on 2015-01-23

Aligment on config

76. By James Page on 2015-01-23

Run flake8 from venv as well

[Liam Young (gnuoy)]

Approve

review: Approve

=== modified file 'Makefile'
--- Makefile	2015-01-19 18:02:29 +0000
+++ Makefile	2015-01-23 09:01:59 +0000
@@ -15,8 +15,8 @@
 	virtualenv .venv --system-site-packages
 	.venv/bin/pip install -I -r test-requirements.txt
 
-lint:
-	@flake8 --exclude hooks/charmhelpers hooks unit_tests
+lint: .venv
+	@.venv/bin/flake8 --exclude hooks/charmhelpers hooks unit_tests
 	@charm proof
 
 bin/charm_helpers_sync.py:
@@ -31,7 +31,7 @@
 	bzr push lp:charms/rabbitmq-server
 	bzr push lp:charms/trusty/rabbitmq-server
 
-unit_test: clean .venv
+unit_test: .venv
 	@echo Starting tests...
 	env CHARM_DIR=$(CHARM_DIR) $(TEST_PREFIX) .venv/bin/nosetests unit_tests/
 

=== modified file 'config.yaml'
--- config.yaml	2015-01-19 15:27:24 +0000
+++ config.yaml	2015-01-23 09:01:59 +0000
@@ -139,6 +139,15 @@
     description: |
       Key ID to import to the apt keyring to support use with arbitary source
       configuration from outside of Launchpad archives or PPA's.
+  # Network configuration options
+  # by default all access is over 'private-address'
+  access-network:
+    type: string
+    default:
+    description: |
+      The IP address and netmask of the 'access' network (e.g., 192.168.0.0/24)
+       .
+      This network will be used for access to RabbitMQ messaging services.
   prefer-ipv6:
     type: boolean
     default: False

=== modified file 'hooks/rabbitmq_server_relations.py'
--- hooks/rabbitmq_server_relations.py	2015-01-22 15:37:28 +0000
+++ hooks/rabbitmq_server_relations.py	2015-01-23 09:01:59 +0000
@@ -71,6 +71,8 @@
     peer_retrieve_by_prefix,
 )
 
+from charmhelpers.contrib.network.ip import get_address_in_network
+
 hooks = Hooks()
 
 SERVICE_NAME = os.getenv('JUJU_UNIT_NAME').split('/')[0]
@@ -158,7 +160,13 @@
     if config('prefer-ipv6'):
         relation_settings['private-address'] = get_ipv6_addr()[0]
     else:
-        relation_settings['hostname'] = unit_get('private-address')
+        # NOTE(jamespage)
+        # override private-address settings if access-network is
+        # configured and an appropriate network interface is configured.
+        relation_settings['hostname'] = \
+            relation_settings['private-address'] = \
+            get_address_in_network(config('access-network'),
+                                   unit_get('private-address'))
 
     configure_client_ssl(relation_settings)
 
@@ -425,13 +433,13 @@
         rbd_size = config('rbd-size')
         sizemb = int(rbd_size.split('G')[0]) * 1024
         blk_device = '/dev/rbd/%s/%s' % (POOL_NAME, rbd_img)
-        # rbd_pool_rep_count = config('ceph-osd-replication-count')
+        ceph.create_pool(service=SERVICE_NAME, name=POOL_NAME,
+                         replicas=int(config('ceph-osd-replication-count')))
         ceph.ensure_ceph_storage(service=SERVICE_NAME, pool=POOL_NAME,
                                  rbd_img=rbd_img, sizemb=sizemb,
                                  fstype='ext4', mount_point=RABBIT_DIR,
                                  blk_device=blk_device,
-                                 system_services=['rabbitmq-server'])  # ,
-        # rbd_pool_replicas=rbd_pool_rep_count)
+                                 system_services=['rabbitmq-server'])
         subprocess.check_call(['chown', '-R', '%s:%s' %
                                (RABBIT_USER, RABBIT_GROUP), RABBIT_DIR])
     else:
@@ -663,6 +671,13 @@
     else:
         update_nrpe_checks()
 
+    # NOTE(jamespage)
+    # trigger amqp_changed to pickup and changes to network
+    # configuration via the access-network config option.
+    for rid in relation_ids('amqp'):
+        for unit in related_units(rid):
+            amqp_changed(relation_id=rid, remote_unit=unit)
+
 
 def pre_install_hooks():
     for f in glob.glob('exec.d/*/charm-pre-install'):

=== modified file 'test-requirements.txt'
--- test-requirements.txt	2015-01-19 18:32:29 +0000
+++ test-requirements.txt	2015-01-23 09:01:59 +0000
@@ -1,3 +1,5 @@
 nose
 testtools
 mock
+coverage
+flake8

=== modified file 'unit_tests/test_rabbitmq_server_relations.py'
--- unit_tests/test_rabbitmq_server_relations.py	2014-11-27 10:52:50 +0000
+++ unit_tests/test_rabbitmq_server_relations.py	2015-01-23 09:01:59 +0000
@@ -69,14 +69,16 @@
         self.fake_repo = {'rabbitmq-server': {'pkg_vers': '3.0'}}
         rabbitmq_server_relations.amqp_changed(None, None)
         mock_peer_store_and_set.assert_called_with(
-            relation_settings={'hostname': host_addr,
+            relation_settings={'private-address': '10.1.2.3',
+                               'hostname': host_addr,
                                'ha_queues': True},
             relation_id=None)
 
         self.fake_repo = {'rabbitmq-server': {'pkg_vers': '3.0.2'}}
         rabbitmq_server_relations.amqp_changed(None, None)
         mock_peer_store_and_set.assert_called_with(
-            relation_settings={'hostname': host_addr},
+            relation_settings={'private-address': '10.1.2.3',
+                               'hostname': host_addr},
             relation_id=None)
 
     @patch('rabbitmq_server_relations.peer_store_and_set')
@@ -108,6 +110,8 @@
         mock_config.side_effect = config
         ipv6_addr = "2001:db8:1:0:f816:3eff:fed6:c140"
         mock_get_ipv6_addr.return_value = [ipv6_addr]
+        host_addr = "10.1.2.3"
+        unit_get.return_value = host_addr
         is_elected_leader.return_value = True
         relation_get.return_value = {}
         is_clustered.return_value = False