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

[Brad Marshall]

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.

[Mick Gregg (macgreagoir)]

Tested successfully in BootStack Staging environment.

[Marco Ceppi (marcoceppi)]

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

[Brad Marshall (brad-marshall)]

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

[Cory Johns (johnsca)]

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

[Brad Marshall (brad-marshall)]

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.

[Brad Marshall (brad-marshall)]

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

[Brad Marshall (brad-marshall)]

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

[Whit Morriss (whitmo)]

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

[lp:~brad-marshall/charms/trusty/ntp/add-auto-peers]

37. By Brad Marshall on 2015-04-01

[bradm] Fixed unit name on the test

[Brad Marshall (brad-marshall)]

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

[Adam Israel (aisrael)]

Hi Brad,

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

review: Approve

[Charles Butler (lazypower)]

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

[Charles Butler (lazypower)]

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

=== modified file 'README.md'
--- README.md	2013-08-29 13:50:54 +0000
+++ README.md	2015-04-01 00:26:30 +0000
@@ -27,11 +27,20 @@
 
     juju set ntp source=myatomiclock.local.net
 
-You can also specify multiple sources:
+You can also specify multiple sources and peers:
 
     juju set ntp source="mac1.local.net mac2.local.net mac3.local.net"
-
-Sources should be space separated.
+    juju set ntp peers="mac1.local.net mac2.local.net mac3.local.net"
+
+Sources and peers should be space separated.
+
+When you need a set of services to keep close time to each other, it may
+be useful to have them automatically peer with each other.  This means
+any set of services that use the same ntp subordinate will peer together.
+
+    juju set ntp auto_peers=true
+
+This will add all the hosts as peers to each other.
 
 Mastered
 ========
 
=== modified file 'config.yaml'
--- config.yaml	2015-03-23 00:39:09 +0000
+++ config.yaml	2015-04-01 00:26:30 +0000
@@ -3,6 +3,10 @@
     default: ""
     type: string
     description: Space separated list of NTP servers to use as source for time.
+  peers:
+    default: ""
+    type: string
+    description: Space separated list of NTP servers to use as peers for time.
   nagios_context:
     default: "juju"
     type: string
@@ -13,7 +17,7 @@
           juju-myservice-0
       If you're running multiple environments with the same services in them
       this allows you to differentiate between them.
-  nagios_servicegroups:
+  nagios_servicegroups: 
     default: ""
     type: string
     description: |
@@ -25,3 +29,11 @@
     description: |
         A space-seperated list of nagios ntpmon checks to enable.
         If left empty, no ntpmon checks will be used.
+  auto_peers:
+    default: false
+    type: boolean
+    description: Will ntp auto peer between the environment
+  use_iburst:
+    default: false
+    type: boolean
+    description: Use iburst for all servers, not just ntpmaster
 
=== added symlink 'hooks/ntp-peers-relation-changed'
=== target is u'ntp_hooks.py'
=== added symlink 'hooks/ntp-peers-relation-joined'
=== target is u'ntp_hooks.py'
=== modified file 'hooks/ntp_hooks.py'
--- hooks/ntp_hooks.py	2015-03-23 00:51:20 +0000
+++ hooks/ntp_hooks.py	2015-04-01 00:26:30 +0000
@@ -19,6 +19,17 @@
 hooks = hookenv.Hooks()
 
 
+def get_peer_nodes():
+    hosts = []
+    hosts.append(hookenv.unit_get('private-address'))
+    for relid in hookenv.relation_ids('ntp-peers'):
+        for unit in hookenv.related_units(relid):
+            hosts.append(hookenv.relation_get('private-address',
+                         unit, relid))
+    hosts.sort()
+    return hosts
+
+
 @hooks.hook('install')
 def install():
     fetch.apt_update(fatal=True)
@@ -30,27 +41,46 @@
 @hooks.hook('config-changed')
 @hooks.hook('master-relation-changed')
 @hooks.hook('master-relation-departed')
[e-mail]('ntp-peers-relation-joined',
+            'ntp-peers-relation-changed')
 @host.restart_on_change({NTP_CONF: ['ntp']})
 def write_config():
+    use_iburst = hookenv.config('use_iburst')
     source = hookenv.config('source')
     remote_sources = []
     if source:
         for s in source.split(" "):
             if len(s) > 0:
-                remote_sources.append({'name': s})
+                if use_iburst:
+                    remote_sources.append({'name': '%s iburst' % s})
+                else:
+                    remote_sources.append({'name': s})
     for relid in hookenv.relation_ids('master'):
         for unit in hookenv.related_units(relid=relid):
             u_addr = hookenv.relation_get(attribute='private-address',
                                           unit=unit, rid=relid)
             remote_sources.append({'name': '%s iburst' % u_addr})
 
+    auto_peers = hookenv.config('auto_peers')
+    peers = hookenv.config('peers')
+    remote_peers = []
+    if peers:
+        for p in peers.split(" "):
+            if len(p) > 0:
+                remote_peers.append(p)
+    if hookenv.relation_ids('ntp-peers'):
+        if auto_peers:
+            for rp in get_peer_nodes():
+                remote_peers.append(rp)
+
     if len(remote_sources) == 0:
         shutil.copy(NTP_CONF_ORIG, NTP_CONF)
     else:
         with open(NTP_CONF, "w") as ntpconf:
             ntpconf.write(render(os.path.basename(NTP_CONF),
-                                 {'servers': remote_sources}))
-
+                                 {'servers': remote_sources,
+                                  'peers': remote_peers,
+                                  'use_iburst': use_iburst}))
     update_nrpe_config()
 
 
 
=== modified file 'metadata.yaml'
--- metadata.yaml	2015-03-17 04:10:30 +0000
+++ metadata.yaml	2015-04-01 00:26:30 +0000
@@ -22,3 +22,6 @@
     scope: container
   master:
     interface: ntp
+peers:
+  ntp-peers:
+    interface: ntp
 
=== modified file 'templates/ntp.conf'
--- templates/ntp.conf	2013-07-15 14:26:04 +0000
+++ templates/ntp.conf	2015-04-01 00:26:30 +0000
@@ -8,6 +8,11 @@
 restrict -6 default kod notrap nomodify nopeer noquery
 restrict 127.0.0.1
 restrict ::1
+{% for peer in peers %}restrict {{ peer }} kod notrap nomodify
+{% endfor %}
 # SERVERS
 {% for server in servers %}server {{ server.name }}
 {% endfor %}
+# PEERS
+{% for peer in peers %}peer {{ peer }} {% if use_iburst %}iburst{% endif %}
+{% endfor %}
 
=== modified file 'tests/10-deploy-test.py'
--- tests/10-deploy-test.py	2014-06-18 18:11:00 +0000
+++ tests/10-deploy-test.py	2015-04-01 00:26:30 +0000
@@ -5,35 +5,36 @@
 import amulet
 
 # The number of seconds to wait for Juju to set up the environment.
-seconds = 900
+seconds = 9600
 ntp_server_0 = 'ntp.your.org'
 ntp_server_1 = 'us.pool.ntp.org'
 ntp_server_2 = 'tock.mtnlion.com'
 
 # The ntp configuration to test.
 ntp_configuration = {
-    'source': ntp_server_0 + ' ' + ntp_server_1 + ' ' + ntp_server_2
+    'source': ntp_server_0 + ' ' + ntp_server_1 + ' ' + ntp_server_2,
+    'auto_peers': True
 }
 
-d = amulet.Deployment()
+d = amulet.Deployment(series='trusty', juju_deployer='juju-deployer -d -v')
 # Add the ntp charm to the deployment.
 d.add('ntp')
 # Add the ntpmaster charm to the deployment.
 d.add('ntpmaster')
 # Add the ubuntu charm to the deployment.
-d.add('ubuntu')
+d.add('ubuntu', units=2)
 # Configure the ntp charm.
 d.configure('ntp', ntp_configuration)
 
+# Relate the ntp and the ubuntu charm.
+d.relate('ntp:juju-info', 'ubuntu:juju-info')
 # Relate the ntp and ntpmaster charms.
 d.relate('ntp:master', 'ntpmaster:master')
-# Relate the ntp and the ubuntu charm.
-d.relate('ntp:juju-info', 'ubuntu:juju-info')
 
 # Deploy the environment and wait for it to setup.
 try:
     d.setup(timeout=seconds)
-    d.sentry.wait(seconds)
+    d.sentry.wait(timeout=seconds)
 except amulet.helpers.TimeoutError:
     message = 'The environment did not setup in %d seconds.' % seconds
     # The SKIP status enables skip or fail the test based on configuration.
@@ -48,7 +49,8 @@
 ntpmaster_unit = d.sentry.unit['ntpmaster/0']
 
 # Get the sentry unit for ubuntu.
-ubuntu_unit = d.sentry.unit['ubuntu/0']
+ubuntu_unit_0 = d.sentry.unit['ubuntu/0']
+ubuntu_unit_1 = d.sentry.unit['ubuntu/1']
 
 # Get the public address for the system running the ntmpmaster charm.
 master_public_address = ntpmaster_unit.info['public-address']
@@ -59,22 +61,27 @@
 master_private_address = master_relation['private-address']
 print('ntpmaster private address ' + master_private_address)
 
+ubuntu_0_public_address = ubuntu_unit_0.info['public-address']
+print('ubuntu/0 public address ' + ubuntu_0_public_address)
+ubuntu_1_public_address = ubuntu_unit_1.info['public-address']
+print('ubuntu/1 public address ' + ubuntu_1_public_address)
+
 # Create a command to check the ntp service.
 command = 'sudo service ntp status'
 print(command)
-# Run the command to see if apache2 is running.
-output, code = ubuntu_unit.run(command)
+# Run the command to see if ntp is running.
+output, code = ubuntu_unit_0.run(command)
 print(output)
 if code != 0 or output.find('NTP server is running') == -1:
     message = 'The NTP service is not running on the ubuntu unit.'
     print(message)
     amulet.raise_status(amulet.FAIL, msg=message)
 
-# The ubuntu cloud image does not have ntp installed by default, 
+# The ubuntu cloud image does not have ntp installed by default,
 # and therefore does not have the /etc/ntp.conf file.
 
 # Read in the ntp configuration file from the ubuntu unit.
-configuration_file = ubuntu_unit.file_contents('/etc/ntp.conf')
+configuration_file = ubuntu_unit_0.file_contents('/etc/ntp.conf')
 # This call will fail with an IO exception if the file does not exist.
 
 # Search for ntp server 0 in the config file, raise an exception if not found.
@@ -87,4 +94,8 @@
 # Search for the ntpmaster IP address in the config file, added by relation.
 configuration_file.index(master_private_address)
 
+# Search for ubuntu/[01] in the config file, raise an exception if not found.
+configuration_file.index(ubuntu_0_public_address)
+configuration_file.index(ubuntu_1_public_address)
+
 print('The ntp deploy test completed successfully.')