Merge lp:~brad-marshall/charms/trusty/ntp/add-auto-peers into lp:charms/trusty/ntp

Proposed by Brad Marshall
Status: Merged
Merged at revision: 19
Proposed branch: lp:~brad-marshall/charms/trusty/ntp/add-auto-peers
Merge into: lp:charms/trusty/ntp
Diff against target: 265 lines (+89/-19)
6 files modified
README.md (+12/-3)
config.yaml (+13/-1)
hooks/ntp_hooks.py (+33/-3)
metadata.yaml (+3/-0)
templates/ntp.conf (+5/-0)
tests/10-deploy-test.py (+23/-12)
To merge this branch: bzr merge lp:~brad-marshall/charms/trusty/ntp/add-auto-peers
Reviewer Review Type Date Requested Status
Charles Butler (community) Approve
Adam Israel (community) Approve
Whit Morriss (community) Needs Fixing
Cory Johns (community) Needs Fixing
Marco Ceppi (community) Needs Fixing
Review via email: mp+253132@code.launchpad.net

Description of the change

Add auto_peers and peers option to manage setting peers both manually and automatically between subordinates. Added a use_iburst option to speed up the initial sync period.

To post a comment you must log in.
Revision history for this message
Mick Gregg (macgreagoir) wrote :

Tested successfully in BootStack Staging environment.

Revision history for this message
Marco Ceppi (marcoceppi) wrote :

This has conflicts with a upstream as it is. Could you please correct these and move merge back in to a "Needs Review" state. Otherwise, LGTM

review: Needs Fixing
Revision history for this message
Brad Marshall (brad-marshall) wrote :

Ah, yes - it conflicts with my other ntp change I got merged. I'll fix that now.

Revision history for this message
Cory Johns (johnsca) wrote :

Brad,

This change looks mostly good, and the merge conflict has been resolved. However, I have two items that I think should be addressed before I can give my +1:

  * Missing default value for nagios_servicegroups option (seems like it was missed / removed in the conflict resolution)

  * A test case for the auto-peering should be added (maybe just a couple of lines could be added to 10-deploy-test.py?)

I'm also a little confused about the purpose of the "total_sources" option? Is it really useful to have an option whose only effect is to create a log message? Is there more behavior that will be coming to this option?

review: Needs Fixing
Revision history for this message
Brad Marshall (brad-marshall) wrote :

Ok, I've fixed the nagios_servicegroup issue, added a test for the peers and removed the total_source line - I was going to do a nagios test but ran out of time to get it sorted correctly. I'll add it back later if I get the chance.

Revision history for this message
Brad Marshall (brad-marshall) wrote :

Somehow I've managed to lose my nrpe changes in this from my other MP, I'll fix that up before resubmitting it.

Revision history for this message
Brad Marshall (brad-marshall) wrote :

This should be ready for review now - once this lands I'll look at doing similar for ntpmaster.

Revision history for this message
Whit Morriss (whitmo) wrote :

LGTM once you add this test fix. Looks like a minor typo that didn't get pushed.

https://code.launchpad.net/~whitmo/charms/trusty/ntp/test-fix/+merge/254802

Merges cleanly, all tests pass.

review: Needs Fixing
37. By Brad Marshall

[bradm] Fixed unit name on the test

Revision history for this message
Brad Marshall (brad-marshall) wrote :

Thanks for catching that, I must have missed pushing up my latest rev somehow. All merged, ready for review.

Revision history for this message
Adam Israel (aisrael) wrote :

Hi Brad,

I had the chance to review your latest revision, and everything passes cleanly for me. +1

review: Approve
Revision history for this message
Charles Butler (lazypower) wrote :

There's one fix I'd like to comment on with this branch.

It appears there's a bug in the setup routine that isn't installing amulet for the python version under test.

http://reports.vapour.ws/charm-test-details/charm-bundle-test-parent-214

I've pulled the tests and run them manually and while the dependency is present, I was unable to get a successful test run:

Traceback (most recent call last):
  File "/tmp/bundletester-s0XPw7/ntp/tests/10-deploy-test.py", line 98, in <module>
    configuration_file.index(ubuntu_0_public_address)

This returned on all tested substrates.

Once this is fixed up I'll be happy to approve and merge.

Thanks for all the hard work already @brad-marshall, I look forward to ack'ing this MP soon.

review: Needs Fixing
Revision history for this message
Charles Butler (lazypower) wrote :

I circled back and cleaned up the tests to pass.

Thanks for the contribution! Make sure that you backport the changes from tests/10-deploy-test.py into your branch before making the next revision or it will collide with whats currently included in TRUNK.

+1 LGTM. Thank you for taking the time to submit this fix for the charm store. We appreciate your work. I've merged this branch and it should be available in the charm store after the next ingestion.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'README.md'
2--- README.md 2013-08-29 13:50:54 +0000
3+++ README.md 2015-04-01 00:26:30 +0000
4@@ -27,11 +27,20 @@
5
6 juju set ntp source=myatomiclock.local.net
7
8-You can also specify multiple sources:
9+You can also specify multiple sources and peers:
10
11 juju set ntp source="mac1.local.net mac2.local.net mac3.local.net"
12-
13-Sources should be space separated.
14+ juju set ntp peers="mac1.local.net mac2.local.net mac3.local.net"
15+
16+Sources and peers should be space separated.
17+
18+When you need a set of services to keep close time to each other, it may
19+be useful to have them automatically peer with each other. This means
20+any set of services that use the same ntp subordinate will peer together.
21+
22+ juju set ntp auto_peers=true
23+
24+This will add all the hosts as peers to each other.
25
26 Mastered
27 ========
28
29=== modified file 'config.yaml'
30--- config.yaml 2015-03-23 00:39:09 +0000
31+++ config.yaml 2015-04-01 00:26:30 +0000
32@@ -3,6 +3,10 @@
33 default: ""
34 type: string
35 description: Space separated list of NTP servers to use as source for time.
36+ peers:
37+ default: ""
38+ type: string
39+ description: Space separated list of NTP servers to use as peers for time.
40 nagios_context:
41 default: "juju"
42 type: string
43@@ -13,7 +17,7 @@
44 juju-myservice-0
45 If you're running multiple environments with the same services in them
46 this allows you to differentiate between them.
47- nagios_servicegroups:
48+ nagios_servicegroups:
49 default: ""
50 type: string
51 description: |
52@@ -25,3 +29,11 @@
53 description: |
54 A space-seperated list of nagios ntpmon checks to enable.
55 If left empty, no ntpmon checks will be used.
56+ auto_peers:
57+ default: false
58+ type: boolean
59+ description: Will ntp auto peer between the environment
60+ use_iburst:
61+ default: false
62+ type: boolean
63+ description: Use iburst for all servers, not just ntpmaster
64
65=== added symlink 'hooks/ntp-peers-relation-changed'
66=== target is u'ntp_hooks.py'
67=== added symlink 'hooks/ntp-peers-relation-joined'
68=== target is u'ntp_hooks.py'
69=== modified file 'hooks/ntp_hooks.py'
70--- hooks/ntp_hooks.py 2015-03-23 00:51:20 +0000
71+++ hooks/ntp_hooks.py 2015-04-01 00:26:30 +0000
72@@ -19,6 +19,17 @@
73 hooks = hookenv.Hooks()
74
75
76+def get_peer_nodes():
77+ hosts = []
78+ hosts.append(hookenv.unit_get('private-address'))
79+ for relid in hookenv.relation_ids('ntp-peers'):
80+ for unit in hookenv.related_units(relid):
81+ hosts.append(hookenv.relation_get('private-address',
82+ unit, relid))
83+ hosts.sort()
84+ return hosts
85+
86+
87 @hooks.hook('install')
88 def install():
89 fetch.apt_update(fatal=True)
90@@ -30,27 +41,46 @@
91 @hooks.hook('config-changed')
92 @hooks.hook('master-relation-changed')
93 @hooks.hook('master-relation-departed')
94+@hooks.hook('ntp-peers-relation-joined',
95+ 'ntp-peers-relation-changed')
96 @host.restart_on_change({NTP_CONF: ['ntp']})
97 def write_config():
98+ use_iburst = hookenv.config('use_iburst')
99 source = hookenv.config('source')
100 remote_sources = []
101 if source:
102 for s in source.split(" "):
103 if len(s) > 0:
104- remote_sources.append({'name': s})
105+ if use_iburst:
106+ remote_sources.append({'name': '%s iburst' % s})
107+ else:
108+ remote_sources.append({'name': s})
109 for relid in hookenv.relation_ids('master'):
110 for unit in hookenv.related_units(relid=relid):
111 u_addr = hookenv.relation_get(attribute='private-address',
112 unit=unit, rid=relid)
113 remote_sources.append({'name': '%s iburst' % u_addr})
114
115+ auto_peers = hookenv.config('auto_peers')
116+ peers = hookenv.config('peers')
117+ remote_peers = []
118+ if peers:
119+ for p in peers.split(" "):
120+ if len(p) > 0:
121+ remote_peers.append(p)
122+ if hookenv.relation_ids('ntp-peers'):
123+ if auto_peers:
124+ for rp in get_peer_nodes():
125+ remote_peers.append(rp)
126+
127 if len(remote_sources) == 0:
128 shutil.copy(NTP_CONF_ORIG, NTP_CONF)
129 else:
130 with open(NTP_CONF, "w") as ntpconf:
131 ntpconf.write(render(os.path.basename(NTP_CONF),
132- {'servers': remote_sources}))
133-
134+ {'servers': remote_sources,
135+ 'peers': remote_peers,
136+ 'use_iburst': use_iburst}))
137 update_nrpe_config()
138
139
140
141=== modified file 'metadata.yaml'
142--- metadata.yaml 2015-03-17 04:10:30 +0000
143+++ metadata.yaml 2015-04-01 00:26:30 +0000
144@@ -22,3 +22,6 @@
145 scope: container
146 master:
147 interface: ntp
148+peers:
149+ ntp-peers:
150+ interface: ntp
151
152=== modified file 'templates/ntp.conf'
153--- templates/ntp.conf 2013-07-15 14:26:04 +0000
154+++ templates/ntp.conf 2015-04-01 00:26:30 +0000
155@@ -8,6 +8,11 @@
156 restrict -6 default kod notrap nomodify nopeer noquery
157 restrict 127.0.0.1
158 restrict ::1
159+{% for peer in peers %}restrict {{ peer }} kod notrap nomodify
160+{% endfor %}
161 # SERVERS
162 {% for server in servers %}server {{ server.name }}
163 {% endfor %}
164+# PEERS
165+{% for peer in peers %}peer {{ peer }} {% if use_iburst %}iburst{% endif %}
166+{% endfor %}
167
168=== modified file 'tests/10-deploy-test.py'
169--- tests/10-deploy-test.py 2014-06-18 18:11:00 +0000
170+++ tests/10-deploy-test.py 2015-04-01 00:26:30 +0000
171@@ -5,35 +5,36 @@
172 import amulet
173
174 # The number of seconds to wait for Juju to set up the environment.
175-seconds = 900
176+seconds = 9600
177 ntp_server_0 = 'ntp.your.org'
178 ntp_server_1 = 'us.pool.ntp.org'
179 ntp_server_2 = 'tock.mtnlion.com'
180
181 # The ntp configuration to test.
182 ntp_configuration = {
183- 'source': ntp_server_0 + ' ' + ntp_server_1 + ' ' + ntp_server_2
184+ 'source': ntp_server_0 + ' ' + ntp_server_1 + ' ' + ntp_server_2,
185+ 'auto_peers': True
186 }
187
188-d = amulet.Deployment()
189+d = amulet.Deployment(series='trusty', juju_deployer='juju-deployer -d -v')
190 # Add the ntp charm to the deployment.
191 d.add('ntp')
192 # Add the ntpmaster charm to the deployment.
193 d.add('ntpmaster')
194 # Add the ubuntu charm to the deployment.
195-d.add('ubuntu')
196+d.add('ubuntu', units=2)
197 # Configure the ntp charm.
198 d.configure('ntp', ntp_configuration)
199
200+# Relate the ntp and the ubuntu charm.
201+d.relate('ntp:juju-info', 'ubuntu:juju-info')
202 # Relate the ntp and ntpmaster charms.
203 d.relate('ntp:master', 'ntpmaster:master')
204-# Relate the ntp and the ubuntu charm.
205-d.relate('ntp:juju-info', 'ubuntu:juju-info')
206
207 # Deploy the environment and wait for it to setup.
208 try:
209 d.setup(timeout=seconds)
210- d.sentry.wait(seconds)
211+ d.sentry.wait(timeout=seconds)
212 except amulet.helpers.TimeoutError:
213 message = 'The environment did not setup in %d seconds.' % seconds
214 # The SKIP status enables skip or fail the test based on configuration.
215@@ -48,7 +49,8 @@
216 ntpmaster_unit = d.sentry.unit['ntpmaster/0']
217
218 # Get the sentry unit for ubuntu.
219-ubuntu_unit = d.sentry.unit['ubuntu/0']
220+ubuntu_unit_0 = d.sentry.unit['ubuntu/0']
221+ubuntu_unit_1 = d.sentry.unit['ubuntu/1']
222
223 # Get the public address for the system running the ntmpmaster charm.
224 master_public_address = ntpmaster_unit.info['public-address']
225@@ -59,22 +61,27 @@
226 master_private_address = master_relation['private-address']
227 print('ntpmaster private address ' + master_private_address)
228
229+ubuntu_0_public_address = ubuntu_unit_0.info['public-address']
230+print('ubuntu/0 public address ' + ubuntu_0_public_address)
231+ubuntu_1_public_address = ubuntu_unit_1.info['public-address']
232+print('ubuntu/1 public address ' + ubuntu_1_public_address)
233+
234 # Create a command to check the ntp service.
235 command = 'sudo service ntp status'
236 print(command)
237-# Run the command to see if apache2 is running.
238-output, code = ubuntu_unit.run(command)
239+# Run the command to see if ntp is running.
240+output, code = ubuntu_unit_0.run(command)
241 print(output)
242 if code != 0 or output.find('NTP server is running') == -1:
243 message = 'The NTP service is not running on the ubuntu unit.'
244 print(message)
245 amulet.raise_status(amulet.FAIL, msg=message)
246
247-# The ubuntu cloud image does not have ntp installed by default,
248+# The ubuntu cloud image does not have ntp installed by default,
249 # and therefore does not have the /etc/ntp.conf file.
250
251 # Read in the ntp configuration file from the ubuntu unit.
252-configuration_file = ubuntu_unit.file_contents('/etc/ntp.conf')
253+configuration_file = ubuntu_unit_0.file_contents('/etc/ntp.conf')
254 # This call will fail with an IO exception if the file does not exist.
255
256 # Search for ntp server 0 in the config file, raise an exception if not found.
257@@ -87,4 +94,8 @@
258 # Search for the ntpmaster IP address in the config file, added by relation.
259 configuration_file.index(master_private_address)
260
261+# Search for ubuntu/[01] in the config file, raise an exception if not found.
262+configuration_file.index(ubuntu_0_public_address)
263+configuration_file.index(ubuntu_1_public_address)
264+
265 print('The ntp deploy test completed successfully.')

Subscribers

People subscribed via source and target branches

to all changes: