fix(operator): reconcile dependents after deletion#483
Conversation
WalkthroughAdds a stack-label enforcement step to dependent reconciliation via a new ChangesStack Label and Dependency Watch Resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Watcher as WatchDependents
participant Resolver as stackNameFromObject
participant StackStore as GetAllStackDependencies
Watcher->>Resolver: stackNameFromObject(object)
Resolver->>Resolver: check Dependent interface
Resolver->>Resolver: check labels[StackLabel]
Resolver->>Resolver: check annotations[StackLabel]
Resolver->>Resolver: check owner references
Resolver-->>Watcher: stackName or ""
alt stackName is empty
Watcher-->>Watcher: return no requests
else stackName resolved
Watcher->>StackStore: GetAllStackDependencies(stackName)
StackStore-->>Watcher: dependency requests
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Approve — automated reviewThe PR adds stack-label enforcement on dependents via a new No findings. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/tests/gateway_controller_test.go (1)
269-293: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing
.To(Succeed())makesLoadResourceassertions no-ops.At Lines 271 and 283,
g.Expect(LoadResource("", gateway.Name, gateway))is never chained with a matcher (e.g.,.To(Succeed())), so Gomega never actually asserts the load succeeded — anyLoadResourceerror is silently swallowed andgatewaymay hold stale/zero-value data on the next line. This pattern already exists elsewhere in the file (e.g. Lines 91, 258) but is copied into this new regression test, which is specifically meant to catch missed-reconciliation bugs like the one described in the PR. A silent failure here could mask exactly the type of issue this test aims to prevent.🐛 Proposed fix
It("Should remove deleted HTTPService from the gateway", func() { Eventually(func(g Gomega) []string { - g.Expect(LoadResource("", gateway.Name, gateway)) + g.Expect(LoadResource("", gateway.Name, gateway)).To(Succeed()) return gateway.Status.SyncHTTPAPIs }).Should(ContainElements(httpAPI.Spec.Name, anotherHttpService.Spec.Name)) Expect(Delete(anotherHttpService)).To(Succeed()) Eventually(func() error { return LoadResource(stack.Name, anotherHttpService.Spec.Name, &corev1.Service{}) }).Should(BeNotFound()) Eventually(func(g Gomega) []string { - g.Expect(LoadResource("", gateway.Name, gateway)) + g.Expect(LoadResource("", gateway.Name, gateway)).To(Succeed()) return gateway.Status.SyncHTTPAPIs }).Should(ConsistOf(httpAPI.Spec.Name))🤖 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 `@internal/tests/gateway_controller_test.go` around lines 269 - 293, The gateway test is missing explicit success assertions on LoadResource, so failures can be silently ignored and stale gateway state can be used. Update the two LoadResource checks inside the relevant Eventually blocks in the gateway controller test to chain an assertion like .To(Succeed()) via g.Expect, matching the pattern used elsewhere in this file. Use the gateway.Status.SyncHTTPAPIs and ConfigMap Caddyfile checks as the surrounding references when locating the regression.internal/core/watch.go (1)
38-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated label/annotation lookup and hardcoded
"any"sentinel.The labels and annotations blocks repeat identical logic, and the
"any"exclusion string is duplicated. Consider factoring into a small helper and a named constant to avoid drift if this sentinel value changes elsewhere (e.g.SkipLabel).♻️ Possible consolidation
+func lookupStackName(m map[string]string) string { + if v := m[v1beta1.StackLabel]; v != "" && v != "any" { + return v + } + return "" +} + func stackNameFromObject(object client.Object) string { if dependent, ok := object.(v1beta1.Dependent); ok && dependent.GetStack() != "" { return dependent.GetStack() } - if labels := object.GetLabels(); labels != nil { - if stackName := labels[v1beta1.StackLabel]; stackName != "" && stackName != "any" { - return stackName - } - } - - if annotations := object.GetAnnotations(); annotations != nil { - if stackName := annotations[v1beta1.StackLabel]; stackName != "" && stackName != "any" { - return stackName - } - } + if name := lookupStackName(object.GetLabels()); name != "" { + return name + } + if name := lookupStackName(object.GetAnnotations()); name != "" { + return name + }🤖 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 `@internal/core/watch.go` around lines 38 - 62, The stack name lookup in stackNameFromObject repeats the same label/annotation filtering logic and hardcodes the "any" sentinel in two places. Refactor the duplicated blocks into a small helper used by stackNameFromObject, and replace the literal with a named constant (for example SkipLabel) so the special value is defined once and stays consistent across labels and annotations.internal/core/controllers.go (1)
106-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit test coverage for the new
ensureStackLabelhelper.This helper is the core fix for the reported reconciliation bug (labels must persist so
stackNameFromObjectin watch.go can resolve the stack on delete). A table-driven test covering: no-op when label already matches, patch when missing, patch when mismatched, and no-op whenGetStack()is empty, would directly validate the root-cause fix rather than relying solely on the Chainsaw e2e scenario.🤖 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 `@internal/core/controllers.go` around lines 106 - 125, Add unit tests for the new ensureStackLabel helper in controllers.go. Cover the main behaviors: return nil when GetStack() is empty, return nil when the existing StackLabel already matches the stack name, and call ctx.GetClient().Patch when the label is missing or mismatched. Use a table-driven test around ensureStackLabel with a fake v1beta1.Dependent object so the label mutation and patch path are verified directly.
🤖 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 `@internal/core/watch.go`:
- Around line 38-62: The Service creation path is missing stack identity
metadata, so `stackNameFromObject` in `watch.go` cannot map `corev1.Service`
events back to their stack. Update `internal/resources/services/services.go` so
the Service object carries the `v1beta1.StackLabel` and/or matching annotation
and includes a `Stack` owner reference in addition to the existing controller
reference. Use the same stack identity used by the dependent owner so
`WatchDependents` can resolve created and deleted Services correctly.
---
Nitpick comments:
In `@internal/core/controllers.go`:
- Around line 106-125: Add unit tests for the new ensureStackLabel helper in
controllers.go. Cover the main behaviors: return nil when GetStack() is empty,
return nil when the existing StackLabel already matches the stack name, and call
ctx.GetClient().Patch when the label is missing or mismatched. Use a
table-driven test around ensureStackLabel with a fake v1beta1.Dependent object
so the label mutation and patch path are verified directly.
In `@internal/core/watch.go`:
- Around line 38-62: The stack name lookup in stackNameFromObject repeats the
same label/annotation filtering logic and hardcodes the "any" sentinel in two
places. Refactor the duplicated blocks into a small helper used by
stackNameFromObject, and replace the literal with a named constant (for example
SkipLabel) so the special value is defined once and stays consistent across
labels and annotations.
In `@internal/tests/gateway_controller_test.go`:
- Around line 269-293: The gateway test is missing explicit success assertions
on LoadResource, so failures can be silently ignored and stale gateway state can
be used. Update the two LoadResource checks inside the relevant Eventually
blocks in the gateway controller test to chain an assertion like .To(Succeed())
via g.Expect, matching the pattern used elsewhere in this file. Use the
gateway.Status.SyncHTTPAPIs and ConfigMap Caddyfile checks as the surrounding
references when locating the regression.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6de35cc9-ede2-4f2c-bf71-428c7a0396b2
📒 Files selected for processing (3)
internal/core/controllers.gointernal/core/watch.gointernal/tests/gateway_controller_test.go
flemzord
force-pushed
the
codex/reconcile-dependents-after-deletion
branch
2 times, most recently
from
9676190 to
10462f5
Compare
flemzord
force-pushed
the
codex/reconcile-dependents-after-deletion
branch
from
10462f5 to
e96a375
Compare
3 participants
Summary
Root cause
The failing
mainCI run timed out in the10-gatewayhttpapi-syncscenario on Kubernetes 1.32 after deletingchainsaw-httpapi-payments. The Service was gone, butGateway.status.syncHTTPAPIsstill listedledger payments, which means the Gateway was not reconciled with the deleted HTTP API removed.Delete events can arrive without reliable dependent spec data, so
WatchDependentscould miss the stack name and skip enqueueing the Gateway reconcile. Also, an object that is already marked for deletion can still appear in list results until finalizers complete, so active Gateway and Stack status must not keep it in generated routes/status.Validation
just pre-commitKUBEBUILDER_ASSETS="$(setup-envtest use 1.32.0 -p path)" go test ./internal/tests -run TestAPIs -ginkgo.focus 'StackController|GatewayController' -count=1\n-just tests\n-./bin/chainsaw test tests/e2e/chainsaw/10-gatewayhttpapi-sync --config tests/e2e/chainsaw/.chainsaw.yaml --report-path /tmp/operator-ci-fix-132-artifactson Kindkindest/node:v1.32.5\n