feat(controllers): add hard-limit cleanup reconciler - #29
williamsena-vtex wants to merge 9 commits into
Conversation
Implements the other half of the cross-repo contract with faststore-proxy-launcher's release limiter (PR #760/#761): that limiter never deletes anything itself, it only PATCHes cleaner.vtex.io/hard-limit- exceeded (plus -reason/-tenant/-marked-at) onto a tenant's oldest over-cap release. This reconciler watches for that annotation and performs the actual deletion, opt-in via HARD_LIMIT_CLEANUP_ENABLED so it ships dark. Two release shapes, cleaned up differently: - ksvc-owned (Configuration has an owning Service): deletes the Service, which cascades to its Configuration and Route automatically via Knative's own ownerReferences. - standalone Configuration+Route pair with no owning Service (e.g. kobeio): deletes both explicitly. A Route is only deleted if every one of its traffic targets points at the marked Configuration -- one split across multiple Configurations (e.g. an in-progress canary) is left alone, since deleting it would cut live traffic to a sibling release this reconciler was never asked to remove. Also updates the LikeC4 diagrams (previously documenting this reconciler as "planned, not yet implemented") to reflect the real, implemented behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🛡️ SDD Check — action requiredI couldn't detect an SDD in this PR. Please check one option below (requires write access to the repo):
|
This repo is public -- kobeio is a real VTEX account used as the standalone Configuration+Route example while this reconciler was being designed. Replaced with acmecorp, no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This repo is public -- tupan and umbroco are real VTEX accounts that ended up in test fixtures (one via a comment referencing a real incident by name). Replaced with examplecorp, no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t-cleanup-reconciler
…y Route Real incident found live on dr0: a tenant's stable "current production" Route -- unrelated to any one release's own ksvc, created separately to alias whichever Configuration is current -- kept pointing at a Configuration after this reconciler deleted its owning Service. Deleting a Service only cascades to the Route it owns, not to unrelated Routes elsewhere referencing the same Configuration. The alias Route survived, but broke (Ready: False, "Configuration ... not found") -- exactly the kind of outage this mechanism is supposed to never cause. Before deleting anything, this now always lists every Route in the namespace and checks which ones reference the marked Configuration: - ksvc-owned: safe only if no Route *other than the one its own Service owns* references it. - standalone: safe only if no referencing Route splits traffic with another Configuration -- previously only the split Route itself was spared, but the Configuration it depended on was still deleted regardless, breaking that Route just the same. If any external/split reference exists, the whole deletion is skipped with a Warning event, not just the Route. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The reconciler otherwise only emits Kubernetes Events (kubectl describe/get events), invisible to Prometheus/Grafana. Adds a counter per tenant, action (deleted, skipped_external_reference, skipped_split_reference, failed), and release_shape (ksvc/standalone), mirroring proxy-launcher's release_limiter_action_total so the two sides of this flow can be dashboarded together. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…:S1192) SonarCloud flagged controllers/hard_limit_cleanup_controller.go:51 -- "serving.knative.dev" was inlined 3 times across its Configuration/ Route/RouteList GVKs. Extracted into serviceKnativeDev, package-level so idle_knative_cleanup_controller.go's own knativeServiceGVK reuses it too instead of carrying a separate copy of the same literal. Not a full dedup of the string across the package: RBAC marker comments (//+kubebuilder:rbac:groups=serving.knative.dev,...) still spell it out, since controller-gen parses those as plain text and can't reference a Go identifier. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| func referencesConfig(targets []interface{}, configName string) bool { | ||
| for _, t := range targets { | ||
| m, ok := t.(map[string]interface{}) | ||
| if !ok { | ||
| continue | ||
| } | ||
| if name, _, _ := unstructured.NestedString(m, "configurationName"); name == configName { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
referencesConfig only checks spec.traffic[].configurationName, never revisionName. A Route that pins traffic to one of this Configuration's Revisions by revisionName (a normal Knative pattern, e.g. canary/rollback pinning) won't be detected as a reference at all, so referencingRoutes — which gates every deletion decision in this reconciler — can miss it, and the reconciler ends up deleting a Configuration that a live Route still serves traffic through. That's the exact class of outage the doc comment on referencingRoutes (line 253) says this code exists to prevent.
There was a problem hiding this comment.
Confirmed, good catch -- fixed in 0103650 via effectiveConfigurationName, which resolves a revisionName target back to its owning Configuration using the serving.knative.dev/configuration label Knative stamps on every Revision. Added a regression test that reproduces this exact scenario (a Route pinning traffic via revisionName alongside a sibling configurationName target).
| func (r *HardLimitCleanupReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| cfg := &unstructured.Unstructured{} | ||
| cfg.SetGroupVersionKind(knativeConfigurationGVK) | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(cfg). | ||
| Complete(r) |
There was a problem hiding this comment.
SetupWithManager only watches Configuration (For(cfg)), with no Watches for Route. Combined with the skip paths at line 172 (actionSkippedExternalRef) and line 215 (actionSkippedSplitRef), which return ctrl.Result{}, nil with no requeue: once a marked Configuration is skipped because of a blocking Route, it will never be reconciled again unless the Configuration object itself changes — even after the conflicting Route disappears or its traffic split changes. The resource is stuck permanently until something unrelated touches the Configuration.
There was a problem hiding this comment.
Confirmed -- fixed in 0103650 by also watching Route and re-enqueuing every Configuration it references (mapRouteToConfigurations) whenever a Route changes. Added a regression test that updates a blocking Route's traffic mid-test and asserts the Configuration then gets deleted, proving the re-reconcile actually fires.
| if err := r.Delete(ctx, svc); err != nil && !apierrors.IsNotFound(err) { | ||
| hardLimitCleanupActionTotal.WithLabelValues(tenant, actionFailed, releaseShapeKsvc).Inc() | ||
| r.Recorder.Eventf(cfg, corev1.EventTypeWarning, "HardLimitCleanupFailed", "failed to delete owning Service %s: %s", ownerName, err.Error()) | ||
| return ctrl.Result{}, err | ||
| } | ||
| hardLimitCleanupActionTotal.WithLabelValues(tenant, actionDeleted, releaseShapeKsvc).Inc() | ||
| r.Recorder.Eventf(cfg, corev1.EventTypeNormal, "HardLimitCleanupDeleted", | ||
| "deleted Service %s (tenant=%s reason=%s), cascading to its Configuration and Route", ownerName, tenant, reason) |
There was a problem hiding this comment.
apierrors.IsNotFound(err) on delete is treated the same as a successful delete: it falls through to increment actionDeleted and emit a HardLimitCleanupDeleted event even when the Service was already gone and nothing was actually deleted this reconcile. Same pattern recurs at lines 226-233 for the Configuration delete. This misattributes the metric/event ('deleted' vs. no-op) whenever the resource was removed by a prior reconcile or externally.
There was a problem hiding this comment.
Confirmed -- fixed in 0103650 with a distinct already_gone action for both the Service and Configuration delete calls, so it no longer increments the deleted metric or fires HardLimitCleanupDeleted when nothing was actually deleted this reconcile. Added a regression test asserting the metric split.
None of these existed on main before this branch -- all four were
either new (Reconcile's own complexity, in a function this PR adds) or
newly triggered by it (main() crossed both thresholds once
HardLimitCleanupReconciler's wiring became a 3rd occurrence of
patterns that were fine at 2).
- hard_limit_cleanup_controller.go: split Reconcile's ksvc-owned and
standalone branches into their own methods (reconcileKsvcOwned,
reconcileStandalone) plus a partitionRoutesByExclusivity helper.
Reconcile itself is now a short dispatcher; cognitive complexity
21 -> well under the 15 limit for all three.
- main.go: extracted exitOnControllerSetupErr and a
cleanerControllerEventSource constant, collapsing the three
near-identical "SetupWithManager then log-and-exit-on-error" blocks.
Fixes both duplicated literals ("cleaner-controller",
"unable to create controller") and, by removing the branching, main()'s
own complexity (16 -> under 15).
No behavior change; go test ./controllers/... still 22/22.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three real gaps found in review (all in hard_limit_cleanup_controller.go): 1. referencesConfig/trafficTargetsOnly only ever checked spec.traffic[].configurationName, never revisionName. A Route pinning traffic to a specific Revision by name (a normal canary/rollback pattern) was invisible to referencingRoutes, which gates every deletion decision here -- the exact class of outage this reconciler exists to prevent, just reached through a different field. Fixed via effectiveConfigurationName, which resolves a revisionName target back to its owning Configuration using the label Knative stamps on every Revision (serving.knative.dev/configuration). 2. SetupWithManager only watched Configuration. Once a Configuration was skipped because a Route referenced it, nothing would ever reconcile it again after that Route's traffic changed or it was deleted -- proxy-launcher never re-touches an already-marked Configuration (pkg/limiter's partitionReleases excludes marked ones from consideration), so a resolved block stayed stuck forever. Fixed by also watching Route and re-enqueuing every Configuration it references (mapRouteToConfigurations). 3. apierrors.IsNotFound(err) on a Delete call fell through to the same path as an actual deletion, incrementing the "deleted" metric and firing a "Deleted" event even when the resource was already gone. Fixed with a distinct "already_gone" action. Adds the Revision CRD to the envtest fixture set and three regression tests, one per fix -- including one that updates a Route's traffic mid-test to prove the new watch actually re-triggers reconciliation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
|
Canceled |
Summary
Implements the other half of the cross-repo contract with
faststore-proxy-launcher's release limiter (PR #760 / PR #761): that limiter never deletes anything itself, it only PATCHescleaner.vtex.io/hard-limit-exceeded(plus-reason/-tenant/-marked-at) onto a tenant's oldest over-cap release. This reconciler watches for that annotation and performs the actual deletion.HardLimitCleanupReconciler, opt-in viaHARD_LIMIT_CLEANUP_ENABLED(same env-flag pattern as the existing idle-cleanup controller), so it ships dark.acmecorp): safe only if no referencing Route splits traffic with another Configuration (e.g. an in-progress canary). Then deletes the Configuration, plus any Route whose traffic pointed exclusively at it.configurations.serving.knative.devandroutes.serving.knative.devCRDs to the envtest fixture set (controllers/testdata/) -- onlyservices.serving.knative.devexisted before.Critical fix: a real incident found live on dr0
An earlier version of this reconciler deleted a ksvc-owned Configuration's Service under the assumption that "deleting the Service cascades to its Configuration and Route automatically" was the whole story. It is not: a tenant's stable "current production" Route -- unrelated to any one release's own ksvc, created separately to alias whichever Configuration is current -- kept referencing that Configuration after the Service was deleted, since the cascade only reaches the Route the Service itself owns. The alias Route survived, but broke:
That is exactly the kind of outage this mechanism must never cause -- the entire point of proxy-launcher's limiter is to stay best-effort and never break something live. Fixed by making the Route-reference check unconditional for both release shapes, and by making it block the whole deletion (not just skip deleting an individual Route) whenever an external or split reference exists.
Context
The annotation this reconciler acts on was live-validated on dr0 against the real
faststore-proxy-launcherrelease limiter before this reconciler was written: confirmed the marker lands correctly and survives unrelated reconciles from other controllers (see CPU-1236).Test plan
go test ./controllers/...-- 22/22 specs pass: ignores an unmarked Configuration, deletes the owning Service for the ksvc-owned case (no external reference), deletes a standalone Configuration plus its exclusive Route, leaves a standalone Configuration and its Route alone when traffic is split with a sibling Configuration, and -- reproducing the dr0 incident directly -- leaves a ksvc-owned Service (and its Configuration) alone when an external alias Route still references itlikec4 validatepasses for the updated diagrams🤖 Generated with Claude Code