fix(operator): requeue when an owned ConfigMap sync is deferred#266
Merged
Conversation
lockwobr
requested review from
ayuskauskas,
mskalka and
rice-riley
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR tracks deferred owned-ConfigMap writes behind the completed-nodes gate and requeues sooner when such a sync is pending. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 1394-1395: The error returned when updating the config map uses
fmt.Errorf("...%s", err) which prevents proper error wrapping; change the
fmt.Errorf call that constructs the error for the configmap update failure to
use the %w verb (e.g., fmt.Errorf("error updating config map [%s]: %w",
newCM.Name, err)) so callers can use errors.Is/errors.As to inspect the
underlying error; locate the fmt.Errorf that references newCM.Name in
skyhook_controller.go and replace %s with %w (no other logic changes required).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b1cb75fe-0f37-415b-9442-c80628578df6
📒 Files selected for processing (2)
operator/internal/controller/skyhook_controller.gooperator/internal/controller/skyhook_controller_test.go
Coverage Report for CI Build 26922809437Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 81.591%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
HandleConfigUpdates only writes the owned ConfigMap inside the completedNodes == nodeCount gate. When the gate is closed on the reconcile that observes the diff (e.g. a stale Status.ConfigUpdates entry matching a new configInterrupt glob drives a spurious interrupt cycle), the write is deferred and the only fallback is the 10m MaxInterval requeue. The CM can stay diverged from spec for minutes while status reads complete, and the config-skyhook glob-update chainsaw step flakes when the one race-prone post-interrupt window is missed. Surface a pendingSync signal from HandleConfigUpdates/UpsertConfigmaps and shorten the otherwise idle requeue from MaxInterval to a 30s configSyncRetryInterval so the deferred write retries promptly. pendingSync deliberately does not short-circuit the reconcile: progression toward the gate opening happens in processSkyhooksPerNode, so an early return would deadlock. The interrupt semantics and Status.ConfigUpdates handling are unchanged. Closes #245 Signed-off-by: Brian Lockwood <[email protected]>
lockwobr
force-pushed
the
worktree-config-bug
branch
from
c9feb43 to
d03d82a
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/internal/controller/skyhook_controller.go (1)
1218-1255:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap early error returns in
HandleConfigUpdateswith operation context
Thereturn false, false, errstatements at lines 1220, 1247, and 1254 return a bareerr, losing reconcile context.♻️ Proposed fix
exists, err := r.PodExists(ctx, node.GetNode().Name, skyhook.GetSkyhook().Name, &_package) if err != nil { - return false, false, err + return false, false, fmt.Errorf("checking existing pod for node %s package %s: %w", + node.GetNode().Name, _package.GetUniqueName(), err) } @@ if err != nil { - return false, false, err + return false, false, fmt.Errorf("listing pods for node %s package %s: %w", + node.GetNode().Name, _package.GetUniqueName(), err) } @@ err := r.Delete(ctx, &pod) if err != nil { - return false, false, err + return false, false, fmt.Errorf("deleting pod %s/%s for node %s package %s: %w", + pod.Namespace, pod.Name, node.GetNode().Name, _package.GetUniqueName(), err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/internal/controller/skyhook_controller.go` around lines 1218 - 1255, The early returns in HandleConfigUpdates currently return raw errors from calls to PodExists, dal.GetPods, and r.Delete (see PodExists, r.dal.GetPods, and r.Delete usage) which loses reconcile context; update each return to wrap the error with contextual information (e.g., include operation name "HandleConfigUpdates" plus node name and package unique name or action being performed) using fmt.Errorf or errors.Wrap so the error becomes something like "HandleConfigUpdates: failed to <action> for node <node.GetNode().Name> package <_package.GetUniqueName()>: %w" before returning; apply this to the three spots where return false, false, err appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 1218-1255: The early returns in HandleConfigUpdates currently
return raw errors from calls to PodExists, dal.GetPods, and r.Delete (see
PodExists, r.dal.GetPods, and r.Delete usage) which loses reconcile context;
update each return to wrap the error with contextual information (e.g., include
operation name "HandleConfigUpdates" plus node name and package unique name or
action being performed) using fmt.Errorf or errors.Wrap so the error becomes
something like "HandleConfigUpdates: failed to <action> for node
<node.GetNode().Name> package <_package.GetUniqueName()>: %w" before returning;
apply this to the three spots where return false, false, err appears.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fb0a907d-b1d0-496b-b486-0c4a4d7c399d
📒 Files selected for processing (2)
operator/internal/controller/skyhook_controller.gooperator/internal/controller/skyhook_controller_test.go
ayuskauskas
approved these changes
Jun 4, 2026
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
3 participants
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.
Description
Fixes the permanent ConfigMap desync described in #245, which also surfaces as the flaky
config-skyhookglob updatechainsaw step.Root cause
HandleConfigUpdatesonly writes the ownedConfigMapinside thecompletedNodes == nodeCountgate. When that gate is closed on the reconcile that observes the diff, the write is deferred and the only fallback is the 10mMaxIntervalrequeue.The trigger in #245: a stale
Status.ConfigUpdatesentry left by a prior config-only spec change matches a newly-addedconfigInterruptglob, flippingHasInterrupttrue. That drives a spurious interrupt cycle which holds the gate closed for the whole cycle, leaving a single race-prone reconcile (post-interrupt pod deletion vs. informer-cache convergence) to catch the gate open. Miss it and the CM stays diverged from spec for up to 10 minutes whilestatusreadscomplete— and the chainsaw step times out at 270s, hence the CI flake.Fix (issue Option B)
pendingSyncsignal fromHandleConfigUpdates/UpsertConfigmaps: a CM diff was observed but the write was deferred because the gate was closed.MaxInterval(10m) to a newconfigSyncRetryInterval(30s) so the deferred write retries promptly. Extracted into a smallreconcileResulthelper (keepsReconcileunder the gocyclo threshold and makes the clamp unit-testable).pendingSyncdeliberately does not short-circuit the reconcile: progression toward the gate opening happens inprocessSkyhooksPerNode, so an early return would deadlock (the gate would never open). It only influences the final idle requeue.Interrupt semantics,
Status.ConfigUpdatesclearing, and static-interrupt suppression are untouched, so there's no risk of spurious reboots from this change.A deeper follow-up remains:
Status.ConfigUpdatesdoes double duty (changed-keys + static-interrupt suppression), which is what lets a stale entry trigger a spurious cycle. This PR makes that benign (the CM always converges); eliminating the spurious interrupt itself is worth a separate design discussion.Tests
HandleConfigUpdatessignals / doesn't signalpendingSync;validateAndUpsertSkyhookDatadoes not short-circuit on a deferred sync (deadlock guard); purereconcileResultclamp test.make vet+golangci-lint: 0 issues.config-skyhookglob updatechainsaw step is the e2e guard for this fix.Docs
No
docs/change: this alters only requeue cadence (no CRD field, CLI, annotation, metric, or flag), and no doc page covers config-update internals.Checklist
git commit -s) per the DCO.Closes #245