Confirm a missing component with an uncached read before failing - #417
Open
blackcat wants to merge 2 commits into
Open
Confirm a missing component with an uncached read before failing#417blackcat wants to merge 2 commits into
blackcat wants to merge 2 commits into
Conversation
The Running phase treats `deployed != expected` as an externally deleted
component and fails the AppWrapper immediately -- deliberately with no grace
period and no retry, since an operator deleting a component will not
self-correct. That verdict is drawn from `getComponentStatus`, which reads
components through the cache-backed Client.
The cache is not a sound basis for a terminal verdict. It can lag the API
server, so "not in my cache yet" is indistinguishable from "deleted". Two
situations make the lag routine rather than exotic:
* The controller creates components with `r.Create` (an uncached write) and
the Resuming -> Running status patch re-reconciles within milliseconds, so
the read can land before the watch event for the controller's own write.
* The controller does not watch the component types it deploys, so their
informers are created lazily on the first Get and their initial list is
served at resourceVersion=0, which the API server may answer from an
arbitrarily stale watch cache. Nothing later re-triggers reconciliation to
correct the reading.
Observed in production on a batch/v1 Job: created successfully at T+0, then
`MissingComponent: Only found 0 deployed components, but was expecting 1` at
T+119ms, Failed at T+142ms, and the controller deleted the Job it had just
created. Reproducible by submitting ~20 AppWrappers while restarting the
controller mid-admission to cold-start its informer cache.
Fix: when the cached read reports a component missing, re-run
`getComponentStatus` against `mgr.GetAPIReader()` and only fail if the
uncached read agrees. `getComponentStatus` now takes the Reader to use, so the
same code serves both passes. The extra request is confined to the path that
was about to fail an AppWrapper, so the steady-state read pattern is unchanged.
Genuine external deletion still fails the AppWrapper with no grace period, as
before. Both behaviours are covered by new tests: the stale-cache test fails on
the unpatched controller with phase Failed instead of Running.
Collaborator
|
Hi @blackcat Thanks for the PR! Sorry for the slow response; I had some deferred maintenance to do (#419) before CI was going to run successfully on your PR. Followup issues/PRs as you outlined would be welcome. Would it be possible for you to sign your commits? These repo is configured to require verified signatures on all commits. |
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.
In the
Runningphase,deployed != expectedis treated as an externally deleted component and fails the AppWrapper immediately — deliberately with no grace period and no retry, since an operator deleting a component will not self-correct (internal/controller/appwrapper/appwrapper_controller.go:262).That verdict is drawn from
getComponentStatus, which reads components through the cache-backedClient. The cache is not a sound basis for a terminal verdict: it can lag the API server, so "not in my cache yet" is indistinguishable from "deleted". Two things make the lag routine rather than exotic:r.Create— an uncached write straight to the API server — while the check reads from the cache. TheResuming -> Runningstatus patch re-reconciles within tens of milliseconds, so the read can land before the watch event for the controller's own write arrives.SetupWithManagerwatchesAppWrapperandPod, but not the component types the controller deploys. Their informers are created lazily on the firstGet, and because there is no component watch, nothing later re-triggers reconciliation to correct a wrong reading.This is also the only zero-tolerance path in the controller:
AdmissionGracePeriod,WarmupGracePeriod,FailureGracePeriod,RetryPausePeriodandRetryLimitall exist and are honoured by every other failure mode.Observed in production on a
batch/v1Job, from the controller log interleaved with the Kubernetes audit log:The Job had existed for 98 ms when the controller declared it missing, and the same service account that created it then deleted it. A separate incident on another cluster measured 119 ms with an identical shape.
Issue link
No pre-existing issue — this was diagnosed from production logs and is reported here in full. For context, the check being corrected was introduced for #130 ("Detect deletion of deployed resources"), whose target case is a human deleting a wrapped resource. That case is unaffected by this change.
What changes have been made
getComponentStatusnow takes theclient.Readerto use, so one code path serves both a cached and an uncached pass.In the
Runningphase, when the cached read reportsdeployed != expected, the status is re-gathered againstmgr.GetAPIReader()and the AppWrapper is failed only if the uncached read agrees.AppWrapperReconcilergains anAPIReader client.Readerfield, wired frommgr.GetAPIReader()inSetupControllers; a smallapiReader()helper falls back to the cached client if the field is unset, so a partially constructed reconciler keeps working rather than panicking.The extra request is confined to the path that was about to fail an AppWrapper, so the steady-state read pattern is unchanged. Genuine external deletion still fails the AppWrapper with no grace period, exactly as before.
4 files, +114 / -22.
Verification steps
Unit / envtest —
make test. Two cases ininternal/controller/appwrapper/appwrapper_controller_test.gocover both directions:A stale cache must not be treated as a missing component. AcacheBlindToPodsdouble (infixtures_test.go) wraps the reconciler's cached client and returnsNotFoundfor thePartialObjectMetadatareadsgetComponentStatusperforms, whileAPIReaderstill sees the real objects — reproducing informer lag. Asserts the AppWrapper staysRunningwithUnhealthy=False.A genuinely deleted component still fails the AppWrapper. Deletes a deployed component out from under the AppWrapper and asserts it still reachesFailedwithUnhealthy=True.To confirm the first test is not vacuous, neuter the fix by making
apiReader()returnr.Clientunconditionally and re-run it: it fails withExpected <AppWrapperPhase>: Failed to equal <AppWrapperPhase>: Running.Manual, on a live cluster — GKE 1.35.7 with Kueue 0.19, AppWrapper deployed from the v1.2.0
install.yaml, one component per AppWrapper (batch/v1Job,sleep 10).Reproducing the defect requires watch delivery latency on a warm informer. Targeting cold starts does not work and is worth stating explicitly: the
batch/v1:Jobinformer is created lazily on the first Running-phaseGet, which happens after the controller's ownr.Create, so the informer's initial LIST always contains that first Job. 1200 submissions across 25 deliberate controller restarts produced nothing. The vulnerable case is every later AppWrapper, where the informer holds a snapshot at RV=X, the new Job is created at RV=Y>X, and the Running-phase read executes before watch delivers RV=Y.The window was widened by backing up the shared Job informer's event delivery: ~110 resident Jobs each carrying a 180KB annotation, plus a loop creating and deleting 40 more per cycle (every create and delete is a 180KB event the informer must decode and store), with a steady stream of AppWrappers submitted throughout so one is always in the
Resuming -> Runningtransition.MissingComponentfailuresThe 15 survivals are the substantive result — zero failures alone proves nothing, since the window is only entered under load, as the 1200 fruitless submissions above show. They are counted from a
V(2)line the patch emits exactly when the cached read reported a component missing and the uncached read then found it present, across 15 distinct AppWrappers. The complementary line, emitted when the uncached read agrees the component is gone, fired 0 times across those 1352 submissions — so nothing was wrongly kept alive.The controller was OOM-killed 3 times during those passes (unrelated to this change — see observation 1 below); the fix held across those restarts.
Upgrade verification — not applicable. No API, CRD or configuration surface changes: the diff touches only reconciler internals and the reconciler's construction in
SetupControllers.make manifestsproduces no diff. An existing AppWrapper is unaffected on controller upgrade or rollback.Checks
Two related observations, not addressed here
Happy to open separate issues if either is of interest.
cache.ByObjectincmd/main.go), so the lazily createdbatch/v1:Jobinformer lists and caches every Job in the cluster, and the controller's memory scales with total cluster Job count against the 128Mi limit ininstall.yaml. A selector on the AppWrapper-owned label would cut both the memory footprint and the event volume that widens the very window this PR fixes.failurePolicy: Fail, so while it is restarting,kubectl applyof an AppWrapper is rejected withfailed calling webhook "mappwrapper.kb.io". This makes HA or restart-based mitigation of transient controller problems costly, and it is worth knowing before recommending a restart as a workaround for anything.