[Nagiosplug-devel] [Nagiosplug-help] check_ntp

[Andreas Ericsson]

sdepThomas Guyot-Sionnest wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 11/11/07 08:29 AM, Andreas Ericsson wrote:
>> Thomas Guyot-Sionnest wrote:
>>> Well, the options for check_ntp will remain the same, the only
>>> difference it that it will return the peer's offset instead of the
>>> offset between localhost and the ntp server. It will also send 8 time
>>> less packets...
>>>
>>> We could call it check_ntpd to avoid any confusion,
>> Sounds good, although I still prefer check_ntp_peers, since that makes
>> it obvious from a directory listing which plugin to use depending on
>> ones needs.
> 
> To me it's not that obvious. OTOH check_time is a check for time offset
> between ourself and a time server, so check_time_ntp for the offset
> check would be the logical one.
> 
> check_ntp's description in --help is:
> 
> "This plugin checks the selected ntp server"
> 
> To me that clearly doesn't mean we're checking the local time against
> the given ntp server.
> 

So amend the message then, perhaps? Changing 400 lines of code (introducing
at least one segfault crash in the process) because a message is wrong
doesn't seem like a stellar plan, tbh.

>>> so we would end up
>>> with check_ntp (marked ad deprecated), check_ntpd and check_time_ntp...
>>> Or something like that.
>>>
>> Sounds bad.
>>
>> check_ntp really should stay with its current semantics.
> 
> If we go that way, it will stay with its current semantics, but it'll
> say that it's deprecated and will be removed much later (Maybe next
> major release).
> 

Still really bad. There's no real reason for it! Simply renaming it is
NOT a valid reason, although I see you've also robbed check_ntp of some
of its functionality in the check_time_ntp incantation.

>> check_ntp{d,_peers}
>> can do the peer checking.
>>
>> Deprecating check_ntp to give it a new name is nearly as user-unfriendly as
>> changing its semantics, but not quite.
>>
>> Think of the plugins as an API that people actively use in programs. If
>> memcpy() all of a sudden was renamed to memcopy(), whole battallions of
>> programs would just stop working over-night.
>>
>> Changing its semantics but maintaining the arguments would be just as
>> broken as if malloc() all of a sudden started returning a pointer to
>> the end of the newly allocated buffer.
> 
> I'm not sure if you follow me well there. By "deprecating" check_ntp I
> just mean we'll yell it all over the place without touching it (apart
> from normal bugfixes). However people will have the option to use two
> new checks that do what they should do.
> 

I understand perfectly well what "deprecating" means, thanks. What I
mean is that there's no real reason for it. None. Zip. Zero. So what
you'll end up doing is forcing people who are using a perfectly fine
plugin to pick another one (that does less btw, as check_time_ntp can't
check for jitter) in order to retain exactly the same setup as they
already have. Can't you see how unforgivably ridiculous this is?

>>> I'm open to discussion but I've seen multiple times people wanting the
>>> peer ofsset in check_ntp, and I feel this is the right thing anyways.
>>>
>> The problem is that you only hear from people who want the behaviour to
>> change. Those who are happy with the way check_ntp works today are much
>> more likely to not say anything. Until it breaks, that is.
> 
> IMHO for many people it's already broken and they just don't realize it.
> When I do a NTP check I check the $HOSTADDRESS$, which mean I want to
> know about the health of the NTP server, not the offset between Nagios
> and the server. Do you know many people that run check_ntp over nrpe and
> specify a time server?
> 

Yes. I also know many people that want to be notified if their nagios
server's clock starts drifting, and I know that everyone will simply
hate whoever makes them sit down to fix some breakage in their nagios
installation simply because a plugin was renamed. They'll be equally
pissed if a plugin that formerly did something now does something
entirely different (although that particular disagreement seems to
have been settled, since you named it check_ntpd instead of replacing
check_ntp).

>>> What might happen if we don't change the name is that people expecting
>>> to check the offset between their server and the remote will actually
>>> monitor the remote server.
>> Hence "check_ntp" and "check_ntp_peers". A directory listing will tell
>> them which plugin to use based on their needs.
> 
> I suggest you take a look at my branch... You can svn co this:
> 
> https://nagiosplug.svn.sourceforge.net/svnroot/nagiosplug/nagiosplug/branches/dermoth_ntp_rework
> 

http:// works equally well, btw (sideband info, but still useful)

>>> OTOH in the past (before working on
>>> check_ntp) I assumed that the offset was the peer's offset and I might
>>> not be alone thinking that. So either way some people won't do the
>>> RightThing(tm) with check_ntp...
>>>
>> In my experience, it's much more common that random servers get out of
>> sync with the stratum >1 server than that server getting out of sync
>> with its peers. I've always expected check_ntp to see if the server it's
>> being run on is in sync with the ntp server.
> 
> A ntp server will loose sync if any of the servers in the path isn't
> sync.

Obviously.

> The newer check specifically warn if the server isn't sync (I may
> add an option to make that configurable - ok/warn/crit/unknown).
> 

Which server? The one check_ntp runs on, or the one it's checking?

-- 
Andreas Ericsson                   andreas.ericsson at op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231