[Nagiosplug-devel] [Nagiosplug-help] check_ntp

[Thomas Guyot-Sionnest]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/11/07 12:33 PM, Andreas Ericsson wrote:
> Thomas Guyot-Sionnest wrote:
>> Isn't that as bad? A plugin doesn't do what it is advertised to do, so
>> change the description and get out with it? What about people that
>> actually relied on this description? Shouldn't they get what they think
>> they're getting?
>>
> 
> Well, the plugin *does* check the selected ntp server. The description is
> just ambiguous, not actually wrong.

If you add jitter or stratum check. The base check alert only on clock
diff between the local and remote host using NTP, which doesn't have
much to do about the remote NTP server apart that it's responding.

>>
>> I didn't reviewed my code yet; I also planed some more thoughtful tests
>> and even putting check_ntpd in a full production system before release.
>>
>> If you found any bug I'll be glad to fix them.
>>
> 
> Here's one.
> 
> diff --git a/plugins/check_ntpd.c b/plugins/check_ntpd.c
> [...}

I don't get this. It isn't a patch since the b/ code it the current one.
So I guess it's one of my commit. I moved the status variable from an
argument-passed *int to a local int, so there's no bug here. I also
planed reviewing variables in this function anyways.

>> I totally disagree there. First check_ntp IS broken. "This plugin checks
>> the selected ntp server". Ok, I run ntpd on all my server, so I add a
>> service check for all my servers that do "check_ntp -H $HOSTADDRESS$
>> - --some-thresholds-here", right? WRONG.
>>
> 
> That depends. If all your servers are supposed to be in sync you'll get
> a warning when one of them goes out of sync with that. Granted, if all
> servers *but* the nagios server stay in sync you'll get a bucket-load
> of notifications, but that's beside the point.

Let me add that in either case of this example check_ntpd will catch
this error just as well as check_ntp (without the bucket-load of
notifications in the 2nd case).

>> So I tell them if you want to monitor your ntp servers
> 
> But there you go re-introducing the same ambiguity again. Be explicit
> in your new plugin.
> 
> "If you want to check if your ntp server is in sync with its peers,
> use check_ntpd"

Noted.

>> , use check_ntpd.
>> if you want to check the time diff between a server and a remote clock,
> 
> Ambiguous again. "If you want to check if a server is in sync with a
> particular ntp server, use check_ntp_time" (check_time_ntp and check_ntpd
> wouldn't sort next to each other in directory listings, so check_ntp_time
> would be better, imo).

Noted, and yes I hesitated between both; I'll go with check_ntp_time if
you prefer :)

>> run check_time_ntp on them. If you do it that way you don't have to
>> check the jitter.
>>
>> Second, they can keep their setup by using check_ntpd. the other option
>> is broken by design. Checking local offset and remote jitter make no
>> sense.
>>
> 
> No, they CAN'T keep their setup by using check_ntpd, since that will check
> something completely different!

People can remove the meaningless jitter if they want to check offset.
If they really want both they'll setup two checks, though I wouldn't
suggest that.

>> I also think we could start a release notes document that would cover
>> more in-depth the changes in NEWS and current issues in BUGS.
>>
> 
> Sensible idea, although I have a problem finding a target audience. Those
> who just want to know if a particular release of the plugins package will
> solve a problem for them will just look at the news. Those interested in
> technicalia will have no problems reading the info straight out of the
> source, since the plugins are so simple.
> 
> But we're digressing.

That would be a place where we could explain changes and gotchas without
being limited by space. It would also avoid multi-line NEWS items.
Indeed that's a subject for another thread :)

>> Yes and I mentioned creating two checks and not touching check_ntp two
>> posts ago, so I don't understand why it's still going on in the thread.
>>
> 
> Because you want to deprecate it, which means "get ready to remove it in
> the future", while you're saying "oh, but lookee here you can use this
> new plugin to do the exact same thing".

The final removal can be for the next major release. And before
upgrading a major release people should be prepared for non-backward
compatible changes (We'll make sure to document them, though).

>> I mean it checks if the remote server has a sync.peer (noted with a star
>> in "ntpq -p" output. This might not be working well yet, but as I said
>> it's still work in progress...
>>
> 
> So changing from check_ntp to check_ntpd would mean one gets no warning
> if the nagios server starts drifting then? That's a clear change of
> semantics right there, and that's why a LOT of people will think the
> new plugin is just plain broken.

Not it they monitor their local NTP daemon. In my setups the Nagios gets
the same basic check as all other servers, regardless if it's using NRPE
or not.

On a side note we just got a new CMS for nagiosplugins.org. There's
place for more documentation on Nagios plugins and I'll do my best to
fill this gap in the coming months. I can't watch everyone's
implementation of Nagios to see if they do everything right, but I can
give them tools to build it the best way possible.

Thomas


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHN6sd6dZ+Kt5BchYRAhW5AKDk+vVdv4CzO+EPrDR2MgvnwFMYawCgtidh
BHSjXw4uBNBoPQJfwP/efm0=
=gBU6
-----END PGP SIGNATURE-----