Repository navigation
A restored clone replaces the snapshot's chronyd around its clock step - #1353
Merged
Merged
Conversation
The restore steps the clone's clock to the host's with clock_settime, behind chronyd's back. chronyd detects an unannounced forward jump only beyond 10 s (JUMP_DETECT_THRESHOLD in chrony's sched.c), and a clone restored seconds after its snapshot is under that, so the daemon keeps the correction it was making in the snapshot. A source VM falls behind by the length of each snapshot pause, so its chronyd is often slewing that lag away when the next snapshot is taken. The clone then applied the leftover correction to an already correct clock: as a step when the restore's `chronyc makestep` ran, otherwise as a slew at chronyd's maximum rate. Main CI run 40411315700 failed test_clone_clock_matches_the_host_to_a_tenth_of_a_second both ways (x86_64: +2186 ms from the first sample; arm64: +10 ms per sample up to +146 ms), each time on a baseline that cold-booted and so paused for the pre-start snapshot about 2 s before the fixture's snapshot. - agent.rs: retire_chronyd_for_restore SIGKILLs every chronyd (SIGTERM would write the daemon's frequency estimate to the drift file for the next daemon to load), waits on each pidfd, and zeroes the kernel's frequency correction a killed slew leaves behind. restart_chronyd_after_restore starts a fresh daemon on /etc/chrony.conf. A mutex serializes both with the boot setup, which a pre-start snapshot can capture mid-way; the restore holds it from the kill until after the step, and leaves starting the daemon to the boot setup if it has not written the config yet. - restore.rs: the kill starts with the restore and is awaited just before the step; the fresh daemon starts after it as a side job. The `chronyc makestep` + burst side job is gone. - bootplan.rs: the clock-sync log line reports the size of the step. - snapshot_network.rs: chrony_burst is private, used by the source only. Tests: - test_clone_runs_a_chronyd_started_after_the_restore: the clone's chronyd PID differs from the source's. Red without the fix (323 == 323 on both tries), green with it (430 vs 540). - the_snapshots_chronyd_is_gone_before_the_clock_step_and_a_fresh_one_starts_after: source order in handle_clone_restore. Red without the fix. - A local reproduction (baseline cold-booted with --no-snapshot, one throwaway snapshot to make it fall behind, snapshot 2.5 s later, clone 3 s after that) reported chronyd "0.88 s slow of NTP time" in the baseline. Without the fix, the clone's chronyd logged "System clock was stepped by 0.426088 seconds" after the restore's step, and the worst sample was 449, 422, 407 ms in 3 of 4 runs; with it, 0.0 ms. make test-root on binary(test_clone_restore_fixes), test_sanity_chrony_ntp_configured, binary(test_serial_console): 8 passed. make test-unit FILTER="-p fc-agent": 286 passed. make lint: clean.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 34 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A restored clone kills the chronyd it inherited from the snapshot before the restore steps its clock, and starts a fresh one after the step. This fixes the TRY 1 failures of
test_clone_clock_matches_the_host_to_a_tenth_of_a_secondin main CI run 40411315700 (Container-arm64 and Container-x64).Mechanism
clock_settime, and chronyd is not told. chronyd only detects an unannounced forward jump larger than 10 s (JUMP_DETECT_THRESHOLD, chronysched.c). A clone restored a few seconds after its snapshot stays under that, so the daemon keeps whatever correction it was making when the snapshot was taken.chronyc makestepruns, the correction lands as a step (x86_64: +2186 ms from the first sample, removed about 2 s later by a fresh NTP sample). Otherwise it lands as a slew at chronyd's maximum rate (arm64: +10 ms per ~120 ms sample, up to +146 ms, then back down).Local reproduction, not committed: cold-boot the baseline with
--no-snapshot, take a throwaway snapshot so it falls behind, take the real snapshot 2.5 s later, and clone 3 s after that. The baseline'schronyc trackingreadSystem time: 0.878573060 seconds slow of NTP time. Without the fix, the clone's chronyd loggedSystem clock was stepped by 0.426088 secondsright after the restore's own step. The worst samples were 449, 422 and 407 ms in 3 of 4 runs. With the fix, the worst sample was 0.0 ms.Change
fc-agent/src/agent.rs:retire_chronyd_for_restoreSIGKILLs every chronyd, waits on each pidfd, and zeroes the kernel frequency correction (adjtimexwithADJ_FREQUENCY | ADJ_TICK) that a killed slew leaves in place. It uses SIGKILL because on SIGTERM chronyd writes its frequency estimate to the drift file, and the next daemon would load it.restart_chronyd_after_restorestarts a fresh daemon on/etc/chrony.conf.start_chronyd, which a pre-start snapshot can capture mid-way. The restore holds the mutex from the kill until after the step. If the boot setup has not written the config yet, the restore leaves starting the daemon to it.fc-agent/src/restore.rs: the kill starts with the restore, overlapping the network phases, and is awaited just before the step. The fresh daemon starts after the step as a side job, off the ACK path. Thechronyc makestep+ burst side job is removed.fc-agent/src/bootplan.rs: the clock-sync log line reports the size of the step (stepped +5.783919 s).fc-agent/src/snapshot_network.rs:chrony_burstis now private. Only the source VM uses it.Tests
test_clone_runs_a_chronyd_started_after_the_restore(new, integration): the clone's chronyd PID must differ from the source's. Without the fix it failed on both tries ("323"vs"323"). With the fix it passed ("430"vs"540").the_snapshots_chronyd_is_gone_before_the_clock_step_and_a_fresh_one_starts_after(new, unit): source order inhandle_clone_restore. Fails without the fix.make test-root FILTER="-E 'binary(test_clone_restore_fixes) | test(test_sanity_chrony_ntp_configured) | binary(test_serial_console)'": 8 passed.make test-unit FILTER="-p fc-agent": 286 passed.make lint: clean.Contract: a restored clone's clock stays where the restore set it; no chronyd state from before the step survives the restore. Downstream impact: every clone restore (fc-agent restore path). The restore now waits for the old chronyd to exit before stepping (normally done during the ~45 ms network phase; bounded at 2 s).
Follow-up (not in this PR)
The source VM still falls behind by the length of each snapshot pause until its chronyd slews the lag away (about 0.9 s at chronyd's maximum slew rate takes over 10 s). This PR does not cause or change that. It only stops a clone from inheriting the correction.