units: disable systemd-time-sync-wait inside containers

[pabigot]

Fixes #8535

[pabigot]

I think this is trivial, so it's submitted, but in honesty I'm still testing it.

[pabigot]

This passes the testing I know how to do (container boots, no failures, systemd-time-wait-sync inactive from start condition failed).

[poettering]

lgtm

[evverx]

I wouldn't say I'm familiar with what is happening here because I'm still reading #8494 (and the pace of my reading appears to be too slow to keep up with you all :-)). Would it make sense to add a comment about why CAP_SYS_TIME is used? I don't think that #8535 (comment) is the most obvious thing I've ever seen. Also, the service seems to have to be in sync with systemd-timesyncd.service, which should probably be mentioned somewhere so that it would be easier to make changes in the future.

[pabigot]

I don't think time-sync-wait has to be in sync with timesyncd; either can be enabled independently and do what they're documented to do. The only intended relation is that time-sync-wait happens to be able to detect an event that's caused by timesyncd, but it detects that event from other (non-systemd) services as well (e.g. connman).

Re CAP_SYS_TIME: I'm not one to complain about adding documentation, but I'm also not qualified to provide it in this case. I'm just 24 hours into my period of understanding how Wants= works....

[evverx]

Indeed, it seems to have to be in sync with whatever is used to advance the system time, so people who will use anything different from systemd-timesynd.service in "system" containers might have to fix the unit file manually.

[evverx]

The CI failure has nothing to do with this PR. I opened #8543.

[pabigot]

@evverx what do you mean by "in sync"? Or, what concerns you about the systemd-time-wait-sync.service unit file that suggests it would need to be modified for use with something other than timesyncd?

time-sync-wait will "work" only with something that uses the NTP standard protocol for adjusting the system realtime clock (i.e. adjtimex(2)), but that's noted in the man page.

[evverx]

I'm sorry for being vague. I'll get back as soon as I've read moby/moby#33126. It seems to me that ConditionVirtualization=!container might be too strict.

[evverx]

Having read moby/moby#33126, I can imagine scenarios where containers are able to call adjtimex with modes=0 without CAP_SYS_TIME (moby/moby#33403) relying on whatever is used on the host to advance the system time. ConditionCapability=CAP_SYS_TIME and ConditionVirtualization=!container make it impossible to use systemd-time-wait-sync.service in such scenarios, which doesn't look correct to me.

[evverx]

I think that it would be better to ignore EPERM in src/time-wait-sync/time-wait-sync.c and print something like "assuming containerized execution, ignoring" as we usually do.

[pabigot]

I can imagine those scenarios exist too, but can't create one to test behavior. adjtimex(2) is not an incidental system call here that can be ignored: it defines the behavior of the service. Unless somebody mocks adjtimex(2) the benefit of allowing systemd-time-sync-wait to run in a container isn't clear: it would have to either run forever (pretend it failed to get a sync) or exit immediately (pretend it got a sync). The former path doesn't seem useful, and the latter is indistinguishable from either failure or blocking the service by condition in its effect on time-sync.target which is the only reason the service exists.

I know nothing about seccomp. I'd be willing to remove ConditionCapability=CAP_SYS_TIME from the unit file in this PR (because it's not supposed to be relevant, and assuming @poettering concurs), but if another approach justified by potential overrides in seccomp is required it should be implemented by somebody who can take responsibility for its impact.

[evverx]

If moby/moby#33403 has been released, any docker container could be used to test this. A more systemd-way would probably be to run systemd-nspawn with --system-call-filter=adjtimex. I actually think adjtimex should be whitelisted in systemd-nspawn too, but that's another matter.

There are people who put everything in containers, so it is possible that containers are being started before or in parallel with systemd-timesyncd or whatever is used on the host to call adjtimex(2). In this case, the assumption that the time in containers has been synced is incorrect. That's why I think that ConditionVirtualization=!container is too strict.

[evverx]

adjtimex(2) is not an incidental system call here that can be ignored: it defines the behavior of the service.

That's true. Maybe it would be better to add something like detect_container > 0 so that the service wouldn't bail out in a container only.

[evverx]

I may be missing the idea behind systemd-time-sync-wait entirely. If it is supposed to work only outside of containers, so it should probably be documented, so nobody will ever assume that it can be relied on inside containers.

[evverx]

Anyway, this PR fixes #8535 in the sense that systemd-time-wait-sync.service has stopped failing. Containers could be discussed later.

[pabigot]

I don't use containers and in fact it never crossed my mind to consider them: I need time-sync.target to work on Yocto-based IoT gateways. AFAICT if a container allows use of adjtime(2) with a zero mode setting just like a "real" system, and whatever implements that system call behaves as a real kernel would including indicating sync status in the return value and supporting timerfd_create and the behavior of cancelling timers on realtime clock set, it should work just fine. If the container environment doesn't do that, then it falls under "won't work" category that is already documented in the NOTES section of the man page.

In such a case, as in any other case where the environment does not support indication that the realtime clock is synchronized the service should be disabled. Hacking the unit file to do this automatically based on ConditionVirtualization is probably not the right solution, though it is the expedient one.

Adding the following might be worthwhile:

See the source code for details on expected behavior of the execution environment. Be aware that container environments may not satisfy the expectations of this service, and it should not be enabled in such situations.

[evverx]

On second thought, it seems that ConditionVirtualization=!container in the unit file makes it clear that containers are somewhat different and it might be tricky to make everything work, so I think additional documentation is unnecessary. People who tend to read the code will read it anyway without any documentation :-)

Thank you!

[poettering]

Hmm, is it really a realistic usecase to run some NTP server in one docker container, and this new tool in a second one, and expect it to work? I am not sure this is likely to ever happen...

[evverx]

I doubt anyone would run an NTP server and systemd-time-wait-sync.service in two different containers. The use case I was trying to describe is a normal NTP server outside of any container and systemd-time-wait-sync.service running in a container before or in parallel with that normal NTP server.

[evverx]

What I don't understand is why the CI didn't run systemd-time-wait-sync.service when #8494 was opened and apparently hasn't run it since then. @martinpitt, @mbiebl could it be that some magic like https://salsa.debian.org/systemd-team/systemd/blob/master/debian/rules#L214 happens preventing the service from starting?