Merge ~vlgrevtsev/ntp-charm:lp1835766 into ntp-charm:master

[Vladimir Grevtsev]

Added option to ensure all source NTP servers provided via config are being tested - and error raised, if some of them are not reachable.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Stuart Bishop (stub)]

Looks good to me. 'blocked' is probably the right status to notify users with, but I could also argue 'waiting' since in many cases the issue will resolve itself rather than needing user intervention.

The subprocess call needs to be tweaked before landing to avoid the need for shell=True, which avoids potential problems if the server address needs shell character escaping (probably trusted and fine, but if shell=False nobody has to trace things back to confirm that).

Still needs a peer review by someone more familiar with NTP before landing, but I suspect it will be no problem since the new behavior is optional.

review: Approve

[Vladimir Grevtsev (vlgrevtsev)]

@Stuart, thanks for comment re "shell=True" - I fixed that and updated a review.

[Haw Loeung (hloeung)]

There's nothing to install the 'ntpdate' package, from universe.

Also, there's code in the ntpmon layer which this charm pulls in and does all the reach-ability checks. Maybe you could import and use that rather than ntpdate?

[Tom Haddon (mthaddon)]

> There's nothing to install the 'ntpdate' package, from universe.

I've tested this on an LXC on trusty:

$ apt-cache policy ntpdate
ntpdate:
  Installed: 1:4.2.6.p5+dfsg-3ubuntu2.14.04.13
  Candidate: 1:4.2.6.p5+dfsg-3ubuntu2.14.04.13
  Version table:
 *** 1:4.2.6.p5+dfsg-3ubuntu2.14.04.13 0
        500 http://archive.ubuntu.com/ubuntu/ trusty-updates/main amd64 Packages
        500 http://security.ubuntu.com/ubuntu/ trusty-security/main amd64 Packages
        100 /var/lib/dpkg/status
     1:4.2.6.p5+dfsg-3ubuntu2 0
        500 http://archive.ubuntu.com/ubuntu/ trusty/main amd64 Packages

And on an LXC on xenial:

$ apt-cache policy ntpdate
ntpdate:
  Installed: (none)
  Candidate: 1:4.2.8p4+dfsg-3ubuntu5.9
  Version table:
     1:4.2.8p4+dfsg-3ubuntu5.9 500
        500 http://archive.ubuntu.com/ubuntu xenial-updates/main amd64 Packages
        500 http://security.ubuntu.com/ubuntu xenial-security/main amd64 Packages
     1:4.2.8p4+dfsg-3ubuntu5 500
        500 http://archive.ubuntu.com/ubuntu xenial/main amd64 Packages

And on LXC in bionic:

$ apt-cache policy ntpdate
ntpdate:
  Installed: (none)
  Candidate: 1:4.2.8p10+dfsg-5ubuntu7.1
  Version table:
     1:4.2.8p10+dfsg-5ubuntu7.1 500
        500 http://archive.ubuntu.com/ubuntu bionic-updates/universe amd64 Packages
        500 http://security.ubuntu.com/ubuntu bionic-security/universe amd64 Packages
     1:4.2.8p10+dfsg-5ubuntu7 500
        500 http://archive.ubuntu.com/ubuntu bionic/universe amd64 Packages

So it seems like configuring universe would be needed if we're on bionic, just to clarify.

[Vladimir Grevtsev (vlgrevtsev)]

Well, the charm itself is installing ntpdate tool:

https://git.launchpad.net/ntp-charm/tree/lib/ntp_scoring.py#n27
https://git.launchpad.net/ntp-charm/tree/reactive/ntp.py#n108

I did a test deployment with Juju and MAAS provider and charm has installed an ntpdate package itself without any manual intervention (however, it was not present before charm installation began): https://pastebin.canonical.com/p/2mJ9J7fwGq/

So is there any reason we still can't use ntpdate, as proposed?

[Haw Loeung (hloeung)]

Ah, I didn't realise that we're already pulling in 'ntpdate' and using it - although it is to be deprecated because the code hasn't really been updated[1]. We certainly want to fix that, but that can happen in future changes.

[1]http://support.ntp.org/bin/view/Dev/DeprecatingNtpdate

[Haw Loeung (hloeung)]

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision b120456b0b3370d44877993c06fbbc200b3f6e37

diff --git a/config.yaml b/config.yaml
index f7574a6..bb07365 100644
--- a/config.yaml
+++ b/config.yaml
@@ -1,4 +1,10 @@
 options:
+  verify_ntp_servers:
+    default: false
+    type: boolean
+    description: >
+      If configured to True, charm will automatically try to verify that
+      given NTP servers are accessible and raise an error, if they aren't.
   source:
     default: ""
     type: string
diff --git a/reactive/ntp.py b/reactive/ntp.py
index 9476fd2..6b8f082 100755
--- a/reactive/ntp.py
+++ b/reactive/ntp.py
@@ -296,6 +296,26 @@ def assess_status():
         hookenv.application_version_set(version)
 
     status = []
+    if hookenv.config('verify_ntp_servers'):
+        failed_servers = []
+        if hookenv.config('source'):
+            for server in hookenv.config('source').split():
+                try:
+                    subprocess.check_call(
+                        ["ntpdate", "-qd", server],
+                        stderr=subprocess.DEVNULL,
+                    )
+                except subprocess.CalledProcessError:
+                    failed_servers.append(server)
+                    continue
+
+            if failed_servers:
+                _servers = '; '.join(failed_servers)
+                hookenv.status_set(
+                    'blocked',
+                    'NTP servers are not reachable: %s' % _servers
+                )
+                return
 
     # service status
     if host.service_running(implementation.service_name()):