Skip to content

Confirm a missing component with an uncached read before failing - #417

Open
blackcat wants to merge 2 commits into
project-codeflare:mainfrom
blackcat:fix/missing-component-stale-cache
Open

Confirm a missing component with an uncached read before failing#417
blackcat wants to merge 2 commits into
project-codeflare:mainfrom
blackcat:fix/missing-component-stale-cache

Conversation

@blackcat

@blackcat blackcat commented Sep 7, 2026

Copy link
Copy Markdown

In the Running phase, deployed != expected is 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-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 things make the lag routine rather than exotic:

  • Components are created with r.Create — an uncached write straight to the API server — while the check reads from the cache. The Resuming -> Running status patch re-reconciles within tens of milliseconds, so the read can land before the watch event for the controller's own write arrives.
  • SetupWithManager watches AppWrapper and Pod, but not the component types the controller deploys. Their informers are created lazily on the first Get, 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, RetryPausePeriod and RetryLimit all exist and are honoured by every other failure mode.

Observed in production on a batch/v1 Job, from the controller log interleaved with the Kubernetes audit log:

17:16:26.353  Resuming
17:16:26.432  jobs.create status=0        <- the Job exists from here on
17:16:26.501  Running
17:16:26.529  Running                     <- next reconcile, Running-phase cached read
17:16:26.530  MissingComponent: Only found 0 deployed components, but was expecting 1
17:16:26.555  Failed                      <- no grace period, no retry
17:16:26.591  jobs.delete status=0        <- controller deletes the Job it just created

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

getComponentStatus now takes the client.Reader to use, so one code path serves both a cached and an uncached pass.

In the Running phase, when the cached read reports deployed != expected, the status is re-gathered against mgr.GetAPIReader() and the AppWrapper is failed only if the uncached read agrees. AppWrapperReconciler gains an APIReader client.Reader field, wired from mgr.GetAPIReader() in SetupControllers; a small apiReader() 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 / envtestmake test. Two cases in internal/controller/appwrapper/appwrapper_controller_test.go cover both directions:

  • A stale cache must not be treated as a missing component. A cacheBlindToPods double (in fixtures_test.go) wraps the reconciler's cached client and returns NotFound for the PartialObjectMetadata reads getComponentStatus performs, while APIReader still sees the real objects — reproducing informer lag. Asserts the AppWrapper stays Running with Unhealthy=False.
  • A genuinely deleted component still fails the AppWrapper. Deletes a deployed component out from under the AppWrapper and asserts it still reaches Failed with Unhealthy=True.

To confirm the first test is not vacuous, neuter the fix by making apiReader() return r.Client unconditionally and re-run it: it fails with Expected <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/v1 Job, 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:Job informer is created lazily on the first Running-phase Get, which happens after the controller's own r.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 -> Running transition.

Unpatched v1.2.0 Patched
AppWrappers submitted 470 1352 (3 passes)
MissingComponent failures 1 0
Races entered and survived 0 15

The 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 manifests produces no diff. An existing AppWrapper is unaffected on controller upgrade or rollback.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests

Two related observations, not addressed here

Happy to open separate issues if either is of interest.

  1. The manager's cache is unfiltered (no cache.ByObject in cmd/main.go), so the lazily created batch/v1:Job informer lists and caches every Job in the cluster, and the controller's memory scales with total cluster Job count against the 128Mi limit in install.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.
  2. The controller serves its own admission webhook with failurePolicy: Fail, so while it is restarting, kubectl apply of an AppWrapper is rejected with failed 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.

pvyazankin and others added 2 commits September 7, 2026 13:00
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.
@dgrove-oss

Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants