tzcode: Implement timezone change detection

[trasz]

Summary

Implement optional timezone change detection for local time libc functions.
This is enabled at libc build time by #defining DETECT_TZ_CHANGES.

Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
X-NetApp-PR: #47

[philip]

Is this NetApp code being merged, or is this upstream tzcode? If this is not upstream, it should be submitted to the tz mailing list for wider review by the tz community.

(Which reminds me that it's been a long time since we've merged a newer version of tzcode. Now that we're on Git, I've run out of excuses for not doing the work. :))

What is the use of this?

[rscheff]

On a related note: do the makefiles fetch the current tzinfo files automatically, when building world/universe? (official repository now at https://www.iana.org/time-zones )

[philip]

@rscheff They are not downloaded automatically. That would make it impossible to build world/universe on an offline machine. They are imported when a new tzdb release is made. I've been doing this for the past several years. I actively follow the tz mailing list. Releases of tzdata are usually imported into FreeBSD head within days of the upstream release (often hours). Imports are merged to stable/* branches soon after and we issue errata notices for supported releases not much later.

I've been less diligent about maintaining tzcode. My excuse has been that Subversion made merging hard. That excuse went away recently. I'll start merging the latest tzcode today.

[imp]

contrib/tzcode/stdtime/localtime.c

1281

This is about 1000x too often to check the timezone changes. The files change on a weekly basis. Checking them once a day should suffice and even it's likely 10x overkill, or once an hour if you really are into overkill. Please make it a #define at least if you insist that once a minute is needed for your company so the project can have a less vigorous check since every single daemon that's getting time will be hammering here.

Also, time is the wrong function to use here. Use clock_gettime with CLOCK_MONOTONIC. Then you don't have to worry about time going backwards or have this weird convoluted code. It's 100% guaranteed to be monotonic, and the cheapest way to get the time from the system. There's all manor of other weird pathology this avoids as well, which this code doesn't do. Given the level of precision, you can ignore the fractions of a second...

lib/libc/stdtime/Makefile.inc

12

It might be nice to make this a proper -DWITH_DETECT_TZ_CHANGES otherwise it is almost certain to bitrot in the project as nothing will ever build it.

[imp]

In D30183#677853, @rscheff wrote:

On a related note: do the makefiles fetch the current tzinfo files automatically, when building world/universe? (official repository now at https://www.iana.org/time-zones )

No. Nor should they. That would break a reproducible build, needlessly delay incremental builds, trigger firewall warnings, introduce races into make universe and would be troublesome for disconnected machines.

Having a periodic job that ran once a day or once a week even would likely suffice and would produce better results and would be more controllable by the system admin.

[philip]

@imp Weekly? There were six releases of tzdata in all of 2020...

(I didn't look at the diff in much detail since any changes to tzcode need to happen upstream.)

[imp]

contrib/tzcode/stdtime/localtime.c

1281

I mean 5k too often. There's about 10k minutes in a week. These files change on the average once every few weeks upstream (well, on busy years, other years it is closer to monthly or less). While there are some high-priority changes once in a blue moon when some country announces at the last minute they won't do DST this year (or come off DST), those are adequately handled by polling once a day (every 1440 minutes) which has a lot of nice prime numbers around 86,400, though to avoid a thundering heard problem on systems with lots of daemons, some bit of randomness should likely be introduced as well.. Coupled with the idea of fetching these once a day automatically, this would produce a minimal number of extra stat calls.... If you're trying to cover cases where the user upgrades, then most of the time, the new zone files aren't needed today (with rare exceptions).... If you are trying to cover the case where the user changes the system's timezone, then a different notification mechanism (ala kqueue on the zone file with a notification for when it changes) would be in order.

[imp]

In D30183#678168, @philip wrote:

@imp Weekly? There were six releases of tzdata in all of 2020...

(I didn't look at the diff in much detail since any changes to tzcode need to happen upstream.)

All the more reason that polling every 61 seconds is overkill... 2009 had is the biggest I saw with 21 releases.... And a few years had 15...

[allanjude]

My understanding was this was meant to catch when you switch which timezone you are in, less so detecting when the timezone definition is updated.

[imp]

In D30183#681647, @allanjude wrote:

My understanding was this was meant to catch when you switch which timezone you are in, less so detecting when the timezone definition is updated.

Then it should use kqueue, imho, and not poll once a minute. It only has to monitor one file.

[trasz]

Sorry for delay; I had quite a bit of a backlog...

Indeed, this is intended to detect changes to the timezone. However, it's not just one file: it also needs to detect changes to a symlink components in the timezone file path; so while the stat(2) version is fine as it is - it uses the whole path - the kqueue(2)-based one would require keeping open file descriptors for each path component. And with both kqueue(2), and the current implementation using stat(2), we would need some kind of static timeout to avoid calling into the kernel every time.

Now, I was looking into prior cases like this in libc, and so far the only one I've found was 60b27ebb253, which implements watching /etc/resolv.conf. That change also uses fstat(2), with reload period of two seconds.

[trasz]

Use clock_gettime(2) and make WITH_DETECT_TZ_CHANGES a proper build option.

[trasz]

In D30183#677842, @philip wrote:

Is this NetApp code being merged, or is this upstream tzcode? If this is not upstream, it should be submitted to the tz mailing list for wider review by the tz community.

It's not upstream. I wonder what should be the workflow here: review it here on FreeBSD Phabricator, then send upstream, or perhaps go upstream first? I'm leaning towards the former.

[imp]

In D30183#702874, @trasz wrote:

Indeed, this is intended to detect changes to the timezone. However, it's not just one file: it also needs to detect changes to a symlink components in the timezone file path; so while the stat(2) version is fine as it is - it uses the whole path - the kqueue(2)-based one would require keeping open file descriptors for each path component. And with both kqueue(2), and the current implementation using stat(2), we would need some kind of static timeout to avoid calling into the kernel every time.

Wouldn't it be sufficient to check the destination file as well as the symlink itself to detect any changes in anything other than a pathological deployment?

contrib/tzcode/stdtime/localtime.c

476

where is this function defined?

1279–1280

This comment is now false and should be removed.

1282

I'd use a plain MONOTONIC here. There's no real reason to use the _FAST version here. On modern hardware, this would save, at most, a rdtsc invocation. Plus the CLOCK_MONOTONIC is POSIX standard, so is more likely to be accepted upstream.

1289

I wonder if there's an advantage to having this be aligned among all programs in the system to get better caching from all the processes reading this data.

1304

This looks like it will re-read things once a minute with tzload. Am I reading this wrong? I don't see where that's doing any kind of stat to make sure that the file has actually changed. Shouldn't we at the very least lstat TZDEFAULT here to see if its contents have changed somewhere?

[imp]

We should likely workout a sane change here and then try to submit upstream.
As such, maybe optimization with kqueue might be reserved for later.
I still have reservations about doing this once per mintue.

contrib/tzcode/stdtime/localtime.c

1304

Doh! I meant to edit this: I see where the check was added. It isn't in the current version which I looked at.

[trasz]

contrib/tzcode/stdtime/localtime.c

1289

That sounds like something that would be nice, but I think it would require quite significant changes :-)

1304

The stat(2) check is inside tzload(); look for change_in_tz().

[trasz]

Use CLOCK_MONOTONIC, not CLOCK_MONOTONIC_FAST; remove comment.

[trasz]

In D30183#702980, @imp wrote:

In D30183#702874, @trasz wrote:

Indeed, this is intended to detect changes to the timezone. However, it's not just one file: it also needs to detect changes to a symlink components in the timezone file path; so while the stat(2) version is fine as it is - it uses the whole path - the kqueue(2)-based one would require keeping open file descriptors for each path component. And with both kqueue(2), and the current implementation using stat(2), we would need some kind of static timeout to avoid calling into the kernel every time.

Wouldn't it be sufficient to check the destination file as well as the symlink itself to detect any changes in anything other than a pathological deployment?

It probably would, but this means introducing additional assumptions; I'd strongly prefer a general approach.

[imp]

In D30183#703872, @trasz wrote:

In D30183#702980, @imp wrote:

In D30183#702874, @trasz wrote:

Indeed, this is intended to detect changes to the timezone. However, it's not just one file: it also needs to detect changes to a symlink components in the timezone file path; so while the stat(2) version is fine as it is - it uses the whole path - the kqueue(2)-based one would require keeping open file descriptors for each path component. And with both kqueue(2), and the current implementation using stat(2), we would need some kind of static timeout to avoid calling into the kernel every time.

Wouldn't it be sufficient to check the destination file as well as the symlink itself to detect any changes in anything other than a pathological deployment?

It probably would, but this means introducing additional assumptions; I'd strongly prefer a general approach.

It would just mean modding the 'clock' with the monotonic time. That would likely be sufficient to improve things... But as this is speculative, feel free to opt not to do this.

[imp]

Let's see what upstream has to say before pushing it in, though.

contrib/tzcode/stdtime/localtime.c

377

Nit: This will always fail if old_name >= PATH_MAX and always trigger, but at least its fail safe: we'll just do extra work.

1282

as an aside, I don't think this can fail on FreeBSD.

[trasz]

Now, about the upstream - I've looked at https://github.com/eggert/tz.git, and it... well, it does resemble our code, to an extent. Which is to say, their code is quite a bit different from ours. Not sure how to handle this, tbh.

[philip]

In D30183#704231, @trasz wrote:

Now, about the upstream - I've looked at https://github.com/eggert/tz.git, and it... well, it does resemble our code, to an extent. Which is to say, their code is quite a bit different from ours. Not sure how to handle this, tbh.

We're running quite a bit behind on tracking upstream. I attempted to sync a couple of times but I got turned off by Subversion. Now I don't have that excuse anymore. I've bumped a sync with upstream higher on my todo list.

Meanwhile, the tz mailing list is very active. Instead of posting your patch to GitHub, I'd recommend posting it to the [e-mail] mailing list for discussion.

[trasz]

I've asked for review on tz@, and here's what follows (see https://mm.icann.org/pipermail/tz/2021-September/030335.html and followups): this has a potential of breaking apps that depend on zone information to not change behind their back, but there's a precedent with Apple doing something similar (although not identical). One proposed solution that I particularly like is for tzcode to provide a function which returns the path to the zone file; this way the app could use whatever notification mechanism it likes, and it would also make it possible to implement a LD_PRELOADed wrapper to implement transparent change detection like this change does.

All in all, I think this is fine, as long as it's not enabled by default.