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

Proposed by James Page
Status: Merged
Merged at revision: 80
Proposed branch: lp:~james-page/charms/trusty/rabbitmq-server/network-splits
Merge into: lp:~openstack-charmers-archive/charms/trusty/rabbitmq-server/next
Diff against target: 144 lines (+39/-9)
5 files modified
Makefile (+3/-3)
config.yaml (+9/-0)
hooks/rabbitmq_server_relations.py (+19/-4)
test-requirements.txt (+2/-0)
unit_tests/test_rabbitmq_server_relations.py (+6/-2)
To merge this branch: bzr merge lp:~james-page/charms/trusty/rabbitmq-server/network-splits
Reviewer Review Type Date Requested Status
Liam Young (community) Approve
Cory Johns Pending
Tim Van Steenburgh Pending
James Page Pending
Review Queue automated testing Pending
Review via email: mp+247384@code.launchpad.net

This proposal supersedes a proposal from 2014-07-24.

Description of the change

Add support for separate 'access-network' configuration.

To post a comment you must log in.
Revision history for this message
Tim Van Steenburgh (tvansteenburgh) wrote : Posted in a previous version of this proposal

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
Revision history for this message
James Page (james-page) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Cory Johns (johnsca) wrote : Posted in a previous version of this proposal

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
Revision history for this message
James Page (james-page) wrote : Posted in a previous version of this proposal

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
Revision history for this message
James Page (james-page) wrote : Posted in a previous version of this proposal
Revision history for this message
James Page (james-page) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote : Posted in a previous version of this proposal

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote : Posted in a previous version of this proposal

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote : Posted in a previous version of this proposal

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/

Revision history for this message
Review Queue (review-queue) wrote : Posted in a previous version of this proposal

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)
Revision history for this message
Cory Johns (johnsca) wrote : Posted in a previous version of this proposal

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
73. By James Page

Optimize calls

74. By James Page

Re-instate configuration

75. By James Page

Aligment on config

76. By James Page

Run flake8 from venv as well

Revision history for this message
Liam Young (gnuoy) wrote :

Approve

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Makefile'
--- Makefile 2015-01-19 18:02:29 +0000
+++ Makefile 2015-01-23 09:01:59 +0000
@@ -15,8 +15,8 @@
15 virtualenv .venv --system-site-packages15 virtualenv .venv --system-site-packages
16 .venv/bin/pip install -I -r test-requirements.txt16 .venv/bin/pip install -I -r test-requirements.txt
1717
18lint:18lint: .venv
19 @flake8 --exclude hooks/charmhelpers hooks unit_tests19 @.venv/bin/flake8 --exclude hooks/charmhelpers hooks unit_tests
20 @charm proof20 @charm proof
2121
22bin/charm_helpers_sync.py:22bin/charm_helpers_sync.py:
@@ -31,7 +31,7 @@
31 bzr push lp:charms/rabbitmq-server31 bzr push lp:charms/rabbitmq-server
32 bzr push lp:charms/trusty/rabbitmq-server32 bzr push lp:charms/trusty/rabbitmq-server
3333
34unit_test: clean .venv34unit_test: .venv
35 @echo Starting tests...35 @echo Starting tests...
36 env CHARM_DIR=$(CHARM_DIR) $(TEST_PREFIX) .venv/bin/nosetests unit_tests/36 env CHARM_DIR=$(CHARM_DIR) $(TEST_PREFIX) .venv/bin/nosetests unit_tests/
3737
3838
=== 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 @@
139 description: |139 description: |
140 Key ID to import to the apt keyring to support use with arbitary source140 Key ID to import to the apt keyring to support use with arbitary source
141 configuration from outside of Launchpad archives or PPA's.141 configuration from outside of Launchpad archives or PPA's.
142 # Network configuration options
143 # by default all access is over 'private-address'
144 access-network:
145 type: string
146 default:
147 description: |
148 The IP address and netmask of the 'access' network (e.g., 192.168.0.0/24)
149 .
150 This network will be used for access to RabbitMQ messaging services.
142 prefer-ipv6:151 prefer-ipv6:
143 type: boolean152 type: boolean
144 default: False153 default: False
145154
=== 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 @@
71 peer_retrieve_by_prefix,71 peer_retrieve_by_prefix,
72)72)
7373
74from charmhelpers.contrib.network.ip import get_address_in_network
75
74hooks = Hooks()76hooks = Hooks()
7577
76SERVICE_NAME = os.getenv('JUJU_UNIT_NAME').split('/')[0]78SERVICE_NAME = os.getenv('JUJU_UNIT_NAME').split('/')[0]
@@ -158,7 +160,13 @@
158 if config('prefer-ipv6'):160 if config('prefer-ipv6'):
159 relation_settings['private-address'] = get_ipv6_addr()[0]161 relation_settings['private-address'] = get_ipv6_addr()[0]
160 else:162 else:
161 relation_settings['hostname'] = unit_get('private-address')163 # NOTE(jamespage)
164 # override private-address settings if access-network is
165 # configured and an appropriate network interface is configured.
166 relation_settings['hostname'] = \
167 relation_settings['private-address'] = \
168 get_address_in_network(config('access-network'),
169 unit_get('private-address'))
162170
163 configure_client_ssl(relation_settings)171 configure_client_ssl(relation_settings)
164172
@@ -425,13 +433,13 @@
425 rbd_size = config('rbd-size')433 rbd_size = config('rbd-size')
426 sizemb = int(rbd_size.split('G')[0]) * 1024434 sizemb = int(rbd_size.split('G')[0]) * 1024
427 blk_device = '/dev/rbd/%s/%s' % (POOL_NAME, rbd_img)435 blk_device = '/dev/rbd/%s/%s' % (POOL_NAME, rbd_img)
428 # rbd_pool_rep_count = config('ceph-osd-replication-count')436 ceph.create_pool(service=SERVICE_NAME, name=POOL_NAME,
437 replicas=int(config('ceph-osd-replication-count')))
429 ceph.ensure_ceph_storage(service=SERVICE_NAME, pool=POOL_NAME,438 ceph.ensure_ceph_storage(service=SERVICE_NAME, pool=POOL_NAME,
430 rbd_img=rbd_img, sizemb=sizemb,439 rbd_img=rbd_img, sizemb=sizemb,
431 fstype='ext4', mount_point=RABBIT_DIR,440 fstype='ext4', mount_point=RABBIT_DIR,
432 blk_device=blk_device,441 blk_device=blk_device,
433 system_services=['rabbitmq-server']) # ,442 system_services=['rabbitmq-server'])
434 # rbd_pool_replicas=rbd_pool_rep_count)
435 subprocess.check_call(['chown', '-R', '%s:%s' %443 subprocess.check_call(['chown', '-R', '%s:%s' %
436 (RABBIT_USER, RABBIT_GROUP), RABBIT_DIR])444 (RABBIT_USER, RABBIT_GROUP), RABBIT_DIR])
437 else:445 else:
@@ -663,6 +671,13 @@
663 else:671 else:
664 update_nrpe_checks()672 update_nrpe_checks()
665673
674 # NOTE(jamespage)
675 # trigger amqp_changed to pickup and changes to network
676 # configuration via the access-network config option.
677 for rid in relation_ids('amqp'):
678 for unit in related_units(rid):
679 amqp_changed(relation_id=rid, remote_unit=unit)
680
666681
667def pre_install_hooks():682def pre_install_hooks():
668 for f in glob.glob('exec.d/*/charm-pre-install'):683 for f in glob.glob('exec.d/*/charm-pre-install'):
669684
=== 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 @@
1nose1nose
2testtools2testtools
3mock3mock
4coverage
5flake8
46
=== 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 @@
69 self.fake_repo = {'rabbitmq-server': {'pkg_vers': '3.0'}}69 self.fake_repo = {'rabbitmq-server': {'pkg_vers': '3.0'}}
70 rabbitmq_server_relations.amqp_changed(None, None)70 rabbitmq_server_relations.amqp_changed(None, None)
71 mock_peer_store_and_set.assert_called_with(71 mock_peer_store_and_set.assert_called_with(
72 relation_settings={'hostname': host_addr,72 relation_settings={'private-address': '10.1.2.3',
73 'hostname': host_addr,
73 'ha_queues': True},74 'ha_queues': True},
74 relation_id=None)75 relation_id=None)
7576
76 self.fake_repo = {'rabbitmq-server': {'pkg_vers': '3.0.2'}}77 self.fake_repo = {'rabbitmq-server': {'pkg_vers': '3.0.2'}}
77 rabbitmq_server_relations.amqp_changed(None, None)78 rabbitmq_server_relations.amqp_changed(None, None)
78 mock_peer_store_and_set.assert_called_with(79 mock_peer_store_and_set.assert_called_with(
79 relation_settings={'hostname': host_addr},80 relation_settings={'private-address': '10.1.2.3',
81 'hostname': host_addr},
80 relation_id=None)82 relation_id=None)
8183
82 @patch('rabbitmq_server_relations.peer_store_and_set')84 @patch('rabbitmq_server_relations.peer_store_and_set')
@@ -108,6 +110,8 @@
108 mock_config.side_effect = config110 mock_config.side_effect = config
109 ipv6_addr = "2001:db8:1:0:f816:3eff:fed6:c140"111 ipv6_addr = "2001:db8:1:0:f816:3eff:fed6:c140"
110 mock_get_ipv6_addr.return_value = [ipv6_addr]112 mock_get_ipv6_addr.return_value = [ipv6_addr]
113 host_addr = "10.1.2.3"
114 unit_get.return_value = host_addr
111 is_elected_leader.return_value = True115 is_elected_leader.return_value = True
112 relation_get.return_value = {}116 relation_get.return_value = {}
113 is_clustered.return_value = False117 is_clustered.return_value = False

Subscribers

People subscribed via source and target branches