Repository navigation
Conversation
Did you test it with an actual VM ? |
startTimeSync keeps the two most recent completed round-trip durations and passes them into each tick. syncGuestClock sends host_time as the send time plus oneWayDelay, half the smaller of those two samples, and that delay is zero while the latest sample is not positive, so the first sync goes out uncompensated and one long sample stays out of the next estimate. A nil RPC error advances the window even when the guest response carries an Error string; a transport error returns the previous window, and a negative elapsed time is stored as zero. Fixes lima-vm#5543 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
411c2c7 to
3a5e97c
Compare
|
No, not before opening the PR. At that point I had only run gofmt, go vet, and the hostagent unit tests. I have since run it on a VM: limactl built from this branch on macOS (Apple silicon, vz) with a Debian 13 guest. Time sync ran every 10 seconds and reported 0 to 6 ms of drift, and on first contact it corrected a 181 ms offset. I then stopped systemd-timesyncd in the guest and set its clock 5 seconds ahead. The next tick logged "guest clock adjusted (was 5011ms off)", and the ticks after that reported -1 to 1 ms. What this does not cover is the case from #5543 itself. On an idle host the RPC round trip is a few milliseconds, well under the guest's 100 ms threshold, so I could not reproduce the backward step locally. That part is covered only by the unit tests. |
jandubois
left a comment
jandubois
left a comment
There was a problem hiding this comment.
AI review claims that the clock can still step backwards: https://jandubois.github.io/lima/20261004-001957-pr-5565.html
Also actual testing on a real VM is required (as already noted before).
oneWayDelay now compensates only once two positive round trips are known, so a slow first sync can no longer aim the second tick ahead. The delay is also capped at the guest's drift threshold, now shared as TimeSyncDriftThreshold in pkg/guestagent/api, so an estimate left over from a slower tick cannot set the guest ahead of the host on its own. A multi-tick test applies the guest's threshold rule and checks the step sequence for a slow first sync, load ending, and slow replies. The negative round-trip clamp is gone, since time.Now is monotonic. Refs lima-vm#5543 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Thanks for the deep review. 82af3f6 applies both of its fixes: the delay is only compensated once two positive round trips are known (I1), and it is capped at the guest's drift threshold, now shared as I haven't run this on a real VM yet. |
Marking this PR as a draft then. |
|
This comment is by Claude Code, from a test Jan asked it to run on his Mac. We ran the merge base (c51dd2d) and this PR's head (82af3f6) side by side on an M1, in two vz VMs with Ubuntu 26.04 and 4 vCPUs each. Guest chrony synced to time.apple.com, and the builds swapped VMs after the first run. Each build spent 46 minutes under full load, with 12 CPU hogs on the host and 12 in each guest, 4 of those at nice -20. The head works on a real VM and behaves like master at idle. Under load, master stepped the guest clock back twice, by 114 ms and 134 ms, and the PR never did. But both steps came from a latency spike on a single sync. Before the 134 ms one, drift read 33 to 39 ms, and the next sync was back to normal latency. The PR estimates the delay from the smaller of the two previous round trips, so the same spike would have stepped its guest too, and the 2 to 0 split is probably chance. Latency above 100 ms that lasted more than one sync, the case this PR compensates, never occurred. The host clock also ran 32 to 86 ms behind NTP during the load runs. The guest follows NTP, so that offset adds to every drift reading, and the PR cannot see it. So on this machine the PR would not have prevented any step we saw. But master stepped only twice in 46 minutes of full load, far less often than #5543 reports, so this run cannot rule out the sustained latency the PR targets on the reporter's machine. We asked in #5543 whether their steps come from single spikes or sustained latency. |
|
I ran this on a real VM: Lima built from 82af3f6, the default Ubuntu 26.04 template on VZ with 4 vCPUs, on an Apple silicon Mac with macOS 27, started with --debug. Once running it looks right. Over roughly 15 minutes, including stretches with every core busy, each SyncTime drift was between 0 and 9 ms and the guest clock was never set. Right after boot it is different, and I don't think this PR causes or fixes that part. With chrony running in the guest, as the template ships, the first sync steps the guest forward to correct its boot-time offset, chrony keeps slewing the same direction for a few seconds, and a later sync steps it back: forward 254 ms then back 242 ms on one boot, back 112 ms on another. The merge-base build did the same thing (forward 215 ms, back 207 ms). With chrony disabled, two restarts gave a single forward step of 219 ms and no step at all, with nothing backward. So the backward steps I can still produce come from Lima and chrony both correcting the clock at boot. That seems worth its own issue, and it is part of why the commit says Refs #5543 rather than Fixes. |
What This PR Changes
startTimeSync keeps the two most recent completed round-trip durations and passes them into each tick. syncGuestClock sends host_time as the send time plus oneWayDelay, half the smaller of those two samples, and that delay is zero while the latest sample is not positive, so the first sync goes out uncompensated and one long sample stays out of the next estimate. A nil RPC error advances the window even when the guest response carries an Error string; a transport error returns the previous window, and a negative elapsed time is stored as zero.
On a loaded host, each periodic guest time sync steps the guest wall clock backward by about the host-to-guest RPC latency, including when the guest clock was already correct. Software in the guest that orders work by wall-clock stamps then sees time jump backward and can fail intermittently. The host agent captured time.Now() at send time and passed that instant to SyncTime as host_time. The guest treats the gap between its clock and that stamp as drift and, past its threshold, sets the clock to the stamp, so the in-flight delay is counted as the guest running ahead and the correction lands in the past.
Linked Issue (Required in most cases)
Closes #5543
How I Tested This
gofmt -l on pkg/hostagent/timesync.go prints nothing before the next command.
go vet ./pkg/hostagent/... prints nothing before the test run.
go test ./pkg/hostagent/... reports the hostagent package as ok.
AI Usage
AI was used for assistance.