Skip to content

Owned ConfigMap can permanently desync from spec when a stale Status.ConfigUpdates entry drives a spurious interrupt cycle #245

Description

@ayuskauskas

Summary

HandleConfigUpdates only writes the owned v1.ConfigMap to match spec.packages[*].configMap when completedNodes == nodeCount on the SAME reconcile that sees a CM diff. If that condition only becomes true after the lifecycle reaches a steady state, and no further events fire, the operator goes idle with the K8s ConfigMap permanently out of sync with spec.

The trigger pattern requires three things to line up — each individually plausible — but when all three combine, the resulting flake is non-deterministic but reproducible:

  1. A previous spec generation left a stale entry in Status.ConfigUpdates[<pkg>] (never cleared because no HandleConfigUpdates call in the next generation reached line 1250).
  2. The new spec's configInterrupts contains a glob that happens to match the stale entry's key.
  3. The lifecycle re-runs for the stale entry (interrupt → post-interrupt → complete) without ever satisfying the completedNodes == nodeCount gate for the new CM diff, then the operator goes silent.

Reproducer

k8s-tests/chainsaw/skyhook/config-skyhook step glob update is the canonical reproducer. It flakes in CI — sometimes the post-GC reconcile catches the gate open and the CM gets written; sometimes it doesn't.

The relevant sequence (test step ordering):

  1. no-interrupt update: dexter's ui.properties key changes. HandleConfigUpdates writes Status.ConfigUpdates[dexter] = [\"ui.properties\"]. Lifecycle reaches config/complete with no interrupt fired (no configInterrupt matched ui.properties under the old spec). Entry is never cleared because no further CM diff resets the gate.
  2. glob update: applies a new spec where dexter.configInterrupts now contains *.properties (in addition to game.properties), and dexter.configMap.game.properties becomes \"changed via glob\".

Evidence from a failing run

Captured at assert-timeout (270 s after the spec apply):

```yaml
spec:
packages:
dexter:
configInterrupts:
'*.properties': { services: [rsyslog], type: service }
game.properties: { services: [rsyslog], type: service }
configMap:
game.properties: |
changed via glob # ← new spec applied
ui.properties: |
changed
interrupt: { type: reboot }
status:
observedGeneration: 6
status: complete
configUpdates:
dexter:
- ui.properties # ← stale from the previous step
nodeState:
kind-worker:
dexter|1.2.3:
stage: post-interrupt
state: complete # ← lifecycle 'completed' for the wrong reason
```

The K8s ConfigMap config-skyhook-dexter-1.2.3 still has data.game.properties: \"changed\" (the previous step's value). 4 minutes 30 seconds of zero operator log lines between the last reconcile (19:26:21) and the assert timeout (19:30:51) confirms the controller went idle without ever satisfying the CM-write gate.

Root cause

HandleConfigUpdates (operator/internal/controller/skyhook_controller.go:1185-1281):

```go
if !reflect.DeepEqual(oldConfigMap.Data, newConfigMap.Data) {
for _, node := range skyhook.GetNodes() {
...
if !exists && node.IsPackageComplete(_package) {
completedNodes++
}
...
}
if completedNodes == nodeCount || erroringNode {
...
err := r.Update(ctx, newConfigMap)
}
}
```

The CM write is conditional on every selected node passing \!PodExists && IsPackageComplete in the same reconcile that observes the diff. There are two compounding issues:

  1. Status.ConfigUpdates is not cleaned up on spec changes. It's only cleared by RemoveConfigUpdates (skyhook.go:128), called in three places: package removed from spec (skyhook_controller.go:1038), version change (:1073), or successful HandleConfigUpdates (:1250). A spec edit that changes a configMap value but happens to leave the package lifecycle in a complete state doesn't refresh Status.ConfigUpdates.
  2. A stale Status.ConfigUpdates entry can drive a spurious interrupt cycle when the next spec's configInterrupts contains a pattern (literal or glob) that matches it. NodeState.HasInterrupt (skyhook_types.go:544-554) preferentially uses the matching-interrupt path when Status.ConfigUpdates[pkg] is non-empty:
    ```go
    if len(config[_package.Name]) > 0 {
    hasInterrupt = len(interrupt[_package.Name]) > 0
    } else {
    hasInterrupt = _package.HasInterrupt()
    }
    ```
    NextStage then routes Config → Interrupt for the stale entry's match.
  3. No safety-net requeue. Once the operator reaches status: complete, it relies on event sources (Pod/Node/Skyhook watches) to trigger further reconciles. None of those normally fire just because the owned CM diverges from spec, so a missed window is permanent.

Why it flakes

For the CM to be written, HandleConfigUpdates needs a single reconcile in which:

  • dexter is at post-interrupt/complete in node state
  • the post-interrupt pod has been deleted and the deletion has propagated to the informer cache (PodExists=false)
  • HasInterrupt(dexter) is still computable as true so IsPackageComplete returns true at StagePostInterrupt

pod_controller.go:86-95 writes node state first, then deletes the pod. The intermediate reconcile (triggered by the Node-annotation write) sees dexter=complete but PodExists=true → gate closed. The follow-up reconcile (triggered by Pod deletion) might be the one that opens the gate — but only if the informer cache has converged by then, and only if it isn't coalesced with a stale snapshot. Concurrent 409 contention on node-status patches (visible in the failing run's logs as 4-level-deep errorCauses nesting) widens the miss window.

That's the entire story: the operator gets exactly one good window per spec change. Miss it, the CM is stale forever.

Suggested fixes (pick one)

Option A — unconditional CM sync. Decouple the r.Update(newConfigMap) write at line 1271 from the completedNodes == nodeCount gate. The K8s ConfigMap is owner-referenced by the Skyhook and is the operator's source of truth for what containers will mount on the next stage run; it should always match the spec. Keep the node-state reset (Upsert(StageConfig, InProgress)) gated by completedNodes — that's the lifecycle-progression decision — but write the CM eagerly.

Option B — safety-net requeue. When HandleConfigUpdates observes a CM diff but cannot satisfy the gate, return (false, nil) with the reconciler setting ctrl.Result{RequeueAfter: 30 * time.Second}. Bounded, cheap, and self-healing — eventually the gate opens or the operator notices the persistent divergence.

I'd lean toward A — it's the cleaner invariant ("owned resources match spec") and it eliminates a class of subtle timing dependencies, not just this one. Option B is the less invasive change but leaves the race in place; it just guarantees we eventually win it.

A third option worth weighing: also clear Status.ConfigUpdates[pkg] whenever a generation change is observed for that package's spec, so the stale-entry-drives-spurious-cycle path goes away independently of the CM-sync fix.

Impact

  • chainsaw/config-skyhook flakes in CI on the glob update step
  • Production impact: any operator user who applies a spec that updates a package's ConfigMap content while a stale Status.ConfigUpdates entry exists and the new spec's configInterrupts glob matches that stale entry can have their ConfigMap permanently out of sync until the next spec edit that retriggers HandleConfigUpdates to completion. The user-visible Skyhook status will report complete, masking the divergence.

Workaround for CI

Skip the glob update step in config-skyhook/chainsaw-test.yaml with a # TODO(<this-issue>) comment until the fix lands. Tracked in PR #242 (the StageInterrupt trap fix unmasked this — config-skyhook was previously failing at an earlier step).

Metadata

Metadata

Assignees

Labels

component/operatorSkyhook operator (controller-manager)

Type

Fields

No fields configured for Bug.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions