fix(controllers,kubelet): skip no-op status writes; preserve managed fields#48
Open
indyjonesnl wants to merge 8 commits into
Open
fix(controllers,kubelet): skip no-op status writes; preserve managed fields#48indyjonesnl wants to merge 8 commits into
indyjonesnl wants to merge 8 commits into
Conversation
sync_pod re-derived terminal-pod status on every reconcile (phase, container statuses, conditions) and wrote it back unconditionally at 7 call sites. Each unconditional update emitted a MODIFIED watch event, bumped resourceVersion, and triggered the next reconcile — an infinite loop. Reproduced during a v1.35 conformance run where a single Succeeded pod cycled at resourceVersion 897500+. Add pod_status_equal helper (canonical-JSON compare on Pod.status) and gate each terminal-pod storage.update with it. Mirrors the equality check the Running-pod readiness path already does. Add idempotency unit tests pinning the predicate's contract: identical statuses compare equal, phase/restart count changes are detected, metadata-only changes ignored, None vs Some handled.
Pin no-op behavior under stable state for: - resource_quota: reconcile must not rewrite quota or pod when status.used recomputes to the same content. Verifies Unit 14's pod-event watch path doesn't self-loop on quota status republishes. - garbage_collector: orphan reaping is one-shot; Succeeded/Running pods without ownerRefs are never written. Confirms Unit 14's first-scan reap + delete_orphan race guard converges. - endpointslice: slice content + endpoint ordering stable between reconciles; terminating-pod representation honored once and not rewritten. Confirms Unit 4's terminating retention + publishNotReady paths don't churn. - deployment: stable-state reconcile + annotation-refresh skip + unrelated pod isolation. Confirms Unit 8's scale_replica_sets converges. One deployment test (proportional_scaling_converges_no_oscillation) is #[ignore]'d — it reproduces a known fall-through where the RollingUpdate progression branch reacts to frozen test ReplicaSet status. In real clusters pod status flows back and rolling-update terminates; fixing the test needs simulated status convergence beyond this scope. Tests use serde_json::Value semantic compare to avoid spurious failures from HashMap iteration-order nondeterminism in serialized JSON.
… writes update_hpa_status_success stamped Utc::now() into every Condition's last_transition_time on every reconcile and wrote the HPA unconditionally. Per K8s convention last_transition_time must update ONLY when a condition's .status field actually transitions (e.g. False -> True). The unconditional stamp produced a MODIFIED watch event every reconcile interval (~30s) and churned downstream watchers. Fix: carry forward the previous timestamp when (condition_type, status) matches the prior HPA status. Additionally gate the storage.update on a canonical-JSON diff so a fully-converged HPA never writes at all. Regression test test_update_hpa_status_idempotent_when_nothing_changes reproduces the bug deterministically (verified to fail without the fix: timestamps advance ~22ms between two reconciles spaced 20ms apart).
… writes update_crd_status re-pushed Established and NamesAccepted conditions with fresh chrono::Utc::now() timestamps on every reconcile, then wrote the CRD unconditionally. With the 30s resync interval, this emitted a MODIFIED watch event every 30s for every CRD in the cluster — controller-side churn. Fix: carry forward each condition's prior last_transition_time when the status field hasn't changed, then gate storage.update on a canonical-JSON diff of the new vs prior CRD status. Regression test test_update_crd_status_idempotent_when_unchanged reproduces the bug deterministically (without fix: timestamps advance ~22ms across two back-to-back update_crd_status calls; with fix: byte-equal). Same anti-pattern previously fixed in hpa.rs (commit b695b4e).
update_snapshot_status rebuilt VolumeSnapshotStatus with a fresh chrono::Utc::now() creation_time on every reconcile and wrote the VolumeSnapshot unconditionally. creation_time records when the snapshot was first taken and must NEVER advance once set. The unconditional re-stamp emitted a MODIFIED watch event per reconcile cycle — controller churn. Fix: preserve prior creation_time when present; gate storage.update on a canonical-JSON diff of new vs prior status. Same anti-pattern previously fixed in hpa.rs (b695b4e) and crd.rs (52f46ea).
update_vpa_status rebuilt VerticalPodAutoscalerStatus and wrote the VPA unconditionally. Even when recommendations were stable, every reconcile emitted a MODIFIED watch event. Fix: canonical-JSON diff between old and new status; skip the write when content matches. Same anti-pattern previously addressed in hpa.rs (b695b4e), crd.rs (52f46ea), and volume_snapshot.rs (eb89d27).
update_pod_status built a fresh PodStatus from scratch and wrote it unconditionally, WIPING container_statuses, init_container_statuses, conditions, pod_ip, start_time, qos_class, and every other field. Called on failure paths from sync_pod (FailedToRestart, LivenessProbeFailed, Never restart policy, etc.), this destroyed all the detail a previously- Running pod had accumulated — and emitted a MODIFIED watch event each call. Fix: read the fresh pod from storage, mutate only phase/reason/message, gate the storage.update with pod_status_equal (introduced in f0eea82).
Two bugs collapsing into the same hot-loop: 1. ObjectReferences in CronJob.status.active included resource_version of each owned Job. That value advances on every Job-controller write, so the new status differed from the old on every CronJob reconcile even when the set of active Jobs was unchanged — emitting a MODIFIED watch event each cycle. Upstream K8s omits resource_version from the active list; align with that. 2. The list of active Jobs was built by iterating storage.list output, which on MemoryStorage is HashMap-backed and produces non-deterministic order. The status.active Vec then flickered between reconciles even when content was identical. Sort by Job name before constructing refs. Together, these fix the CronJob skip-write guard so the controller is truly a no-op when nothing has changed.
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
1 participant
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.
Problem
Multiple reconcilers wrote `status` unconditionally on every reconcile, even when nothing had changed. Two failure modes:
Fix
Eight commits, same pattern applied to each reconciler:
Affected:
Verification
`cargo build --workspace --locked` clean. Idempotency regression tests included. Depends on #32 for green CI.