@AkihiroSuda I see you added the cherry-pick label, but the 20.10 release still uses the copy of the systemd units in the docker-ce-packaging repo, so a PR would need to be opened in that repository (against the 20.10 branch); https://github.com/docker/docker-ce-packaging/blob/20.10/systemd/docker.service

I wanted to double-check if this could cause potential issues in startup delays (I recall we had some change at some point that caused startup to be delayed, waiting for a user session to be started).

From the systemd docs; https://www.freedesktop.org/software/systemd/man/systemd.special.html#time-set.target

time-set.target

Services responsible for setting the system clock (CLOCK_REALTIME) from a local
source (such as a maintained timestamp file or imprecise real-time clock) should
pull in this target and order themselves before it. Services where approximate,
roughly monotonic time is desired should be ordered after this unit, but not
pull it in.

This target does not provide the accuracy guarantees of time-sync.target (see below),
however does not depend on remote clock sources to be reachable, i.e. the target
is typically not delayed by network problems and similar. Use of this target is
recommended for services where approximate clock accuracy and rough monotonicity
is desired but activation shall not be delayed for possibly unreliable network
communication.

The above looks ok to me

The last paragraph also describes:

Note that ordering a unit after time-set.target only has effect if there's actually
a service ordered before it that delays it until the clock is adjusted for rough
monotonicity. Otherwise, this target might get reached before the clock is adjusted
to be roughly monotonic. Enable systemd-timesyncd.service(8), or an alternative
NTP implementation to delay the target.

I think that's ok, but thought it wouldn't hurt to mention here, in case there's concerns (i.e., could a system have some custom service installed that delays time-set.target ??)

/cc @tianon ptal (for extra eyes in case I missed something)

Also wondering if containerd's unit should have this as well?

CI failures are unrelated (I kicked CI again, but don't think we test changes in this file)

i.e., could a system have some custom service installed that delays time-set.target ??

i think thats the point. The target actually needs to be delayed by whatever syncs the local clock - otherwise the target might be reached (and thus dockerd might start) before the time is correct.

One thing that just came to my mind: as dockerd already is ordered after network-online.target it could be potentially okay to even order dockerd after the time-sync.target to have even more guarantees regarding correct time. I'm not sure if there are cases where the clock after time-set.target is still off by too much (systems that were powered off for an extended period of time or sth?).

Also i'm not sure if time-sync.target implies time-set.target or if there might be configurations where between time-set.target and time-sync.target the clock might be not adjusted.

I chose time-set.target though because it seemed the most defensive approach but i'm happy to re-submit a patch where we order after time-sync.target or both time-sync.target and time-set.target.

Yes, I'm definitely more wary of using time-sync.target as that seems risky in situations where a full sync didn't happen. time-set.target looks to be a "best effort" approximation which (for this case) seems appropriate.

i think thats the point. The target actually needs to be delayed by whatever syncs the local clock - otherwise the target might be reached (and thus dockerd might start) before the time is correct.

So (see above) my concern was a bit; does the meaning of time-set.target change if a NTP service is installed? In other words: would it in that case become time-set.target === time-sync.target, and would it wait for NTP service to have completed)? Or would there still be an intermediate state ("time was set, but a full sync didn't happen yet"). I'm hoping it means the latter (time-set.target => (meanwhile wait for sync) => time-sync.target).

I see - okay so we were actually wondering about the same. :)

Here is my research results:

Chrony for example bundles two services files:

• chronyd.service and
• chrony-wait.service.

In case chrony-wait.service is used (i guess this is up to distros and end-users), time-sync.target is only reached after the sync happened - as the unit orders itself before. As the unit-file only uses a weak-dependency (Wants= instead of Requires=), the service failing has no impact (i.e. target will be considered reached).

In case systemd-timesyncd is used, the same behavior can be accomplished if systemd-time-wait-sync.service is enabled. This is because i guess the binary initially sets the clock (so it implements time-set.target) and i guess after that the daemon forks into background and does NTP sync (hence it's also part of time-sync.target). And the controlling of it's blocking or not is done by systemd-time-wait-sync. Actually an elegant solution.

would it in that case become time-set.target === time-sync.target

Kinda. Because systemctl show time-sync.target outputs:
Wants=time-set.target
After=time-set.target

So time-set will be reached before that. Or phrases differently: time-sync.target always implies time-set.target.

If something in time-set will do a local clock-sync depends on the system configuration. I.e. when using

systemd-timesyncd (w/ systemd-time-wait-sync.service)

1. time-set.target STARTED
2. local clock sync
3. time-set.target REACHED
4. time-sync.target STARTED
5. (wait until network time was set/synced)
6. time-sync.target REACHED

systemd-timesyncd (w/o systemd-time-wait-sync.service)

1. time-set.target STARTED
2. local clock sync
3. time-set.target REACHED
4. time-sync.target STARTED
5. time-sync.target REACHED
6. (some amount of time passes)
7. network time was set/synced

chronyd.service

1. time-set.target STARTED
2. (probably no-op if the user has no binary here that just adjusts local clock drift)
3. time-set.target REACHED
4. time-sync.target STARTED
5. chronyd is started in the background (async)
6. time-sync.target REACHED
7. (some amount of time passes)
8. network time was set/synced

chrony-wait.service

1. time-set.target STARTED
2. (probably no-op if the user has no binary here that just adjusts local clock drift)
3. time-set.target REACHED
4. time-sync.target STARTED
5. chronyd is started in the background
6. chronyd waits for successful time-sync over network
7. time-sync.target REACHED

So that means that the meaning of time-sync.target depends on host configuration: on some systems it might be async, on some it might be sync.

So that means

1. time-set.target is probably a no-op on many systems (as i assume many use different time-syncing software like chrony which doesn't support that target)
2. if someone wants to delay the startup of container after time is somehow guaranteed, most time-syncing software uses time-sync.target. That means this patch is useless for them. Even if they would use chrony-wait.service, time-set.target will do nothing for them to ensure time and they might still face issues w/ out-of-the-box installation.

However, we could say it's fine to require the user to configure the dockerd.service to manually add After=[...] time-sync.target. This could be easily done. See Example 2. Overriding vendor settings

As it's easier to add to the dependencies then to remove, i guess the most flexible configuration is to keep the patch as is and then people need to be knowledgable about their setup (or distributions have to take care of that):

Dependencies (After=, etc.) cannot be reset to an empty list, so dependencies can only be added in drop-ins. If you want to remove dependencies, you have to override the entire unit.

However, the best "out of the box" experience would be to order dockerd after time-sync.service. As this implicitly has time-set.target it's behaving the same as in this proposed patch (and i assume it's no-op if no one uses-and then configurations that wait for time-sync via NTP before continuing booting would never have any certificate-timing issues. People just starting the time-sync daemon but not waiting for the initial could still run into the issue however - so it's not 100% reliable.

However we continue to do it here - both approaches are fine and will be an improvement to having no ordering after any time-*.target at all in my opinion.

Sorry for the lengthy reply and not doing/stating this research in the initial PR.

Sorry for the lengthy reply and not doing/stating this research in the initial PR.

Wow, thanks for doing the extra digging, and detailed information; definitely useful! (and it's good to double-check the expected behaviour; these "seemingly small and trivial" changes can sometimes turn out to be "not so trivial" 😅