fix(controller): stop Executor status feedback loop and retry on conflict - #2830
Open
fseldow wants to merge 2 commits into
Open
fix(controller): stop Executor status feedback loop and retry on conflict#2830fseldow wants to merge 2 commits into
fseldow wants to merge 2 commits into
Conversation
…lict Address the concurrent-writer problems described in notaryproject#2797 for the ExecutorReconciler when the deployment is scaled beyond a single replica. - Add GenerationChangedPredicate to the Executor watch so status-only updates no longer re-trigger Reconcile. This cuts the reconcile feedback loop (and the resulting executor rebuild storms that hammer external providers such as Azure Key Vault), amplified xN across replicas. - Wrap the status write in retry.RetryOnConflict and re-fetch the object before re-applying status, so a lost optimistic-concurrency race (HTTP 409) is retried instead of being silently swallowed. - Add unit tests covering the retry-on-conflict and error-recording paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fseldow
requested review from
akashsinghal,
binbin-li,
jimmyraywv,
luisdlp,
susanshi and
toddysm
as code owners
July 27, 2026 03:13
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (67.55%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2830 +/- ##
==========================================
- Coverage 76.36% 76.02% -0.34%
==========================================
Files 88 90 +2
Lines 3999 4176 +177
==========================================
+ Hits 3054 3175 +121
- Misses 799 837 +38
- Partials 146 164 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Ratify’s Executor reconciliation/status reporting to reduce reconcile amplification and handle concurrent status writers when running multiple replicas, while also introducing a per-pod status CRD and an aggregation controller to represent replica-specific health.
Changes:
- Filter the Executor watch with
GenerationChangedPredicateto prevent status-write feedback loops from re-triggering reconciles. - Add conflict-retry behavior around status updates and introduce per-pod
ExecutorPodStatusreporting with aggregation intoExecutor.status.byPod. - Extend CRDs/RBAC/manifests and add unit tests for retry/error recording and per-pod aggregation behavior.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/podstatus/name.go | Adds reversible name packing/unpacking for per-pod status objects. |
| internal/podstatus/name_test.go | Unit tests for pack/unpack, uniqueness, and DNS1123 character compliance. |
| internal/pod/info.go | Adds pod.Name() helper for reading POD_NAME. |
| internal/manager/manager.go | Wires pod identity into ExecutorReconciler and registers ExecutorPodStatusReconciler. |
| internal/controller/executorpodstatus_controller.go | New controller aggregating ExecutorPodStatus into Executor.status.byPod. |
| internal/controller/executorpodstatus_controller_test.go | Tests per-pod status upserts and aggregation logic. |
| internal/controller/executor_controller.go | Adds generation predicate, per-pod status upsert/delete logic, and conflict-retry for direct status updates. |
| internal/controller/executor_controller_retry_test.go | Tests retry-on-conflict and error recording for direct Executor status updates. |
| config/rbac/role.yaml | Updates RBAC rules to include pods + new executorpodstatuses resources. |
| config/manager/manager.yaml | Injects POD_NAME and RATIFY_NAMESPACE via downward API. |
| config/crd/kustomization.yaml | Adds the new ExecutorPodStatus CRD base to kustomize resources. |
| config/crd/bases/config.ratify.dev_executors.yaml | Extends Executor CRD schema with status.byPod entries. |
| config/crd/bases/config.ratify.dev_executorpodstatuses.yaml | Adds new ExecutorPodStatus CRD definition (namespaced + status subresource). |
| api/v2alpha1/zz_generated.deepcopy.go | Updates deep-copies for ExecutorStatus.ByPod and adds deep-copies for new types. |
| api/v2alpha1/executorpodstatus_types.go | Defines PodStatusEntry, ExecutorPodStatus, and list types. |
| api/v2alpha1/executor_types.go | Adds ByPod []PodStatusEntry to ExecutorStatus. |
Files not reviewed (1)
- api/v2alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+68
to
+71
| var list configv2alpha1.ExecutorPodStatusList | ||
| if err := r.List(ctx, &list); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to list ExecutorPodStatus objects: %w", err) | ||
| } |
Comment on lines
+252
to
+258
| // briefError truncates an error message to maxBriefErrorLength characters. | ||
| func briefError(msg string) string { | ||
| if len(msg) <= maxBriefErrorLength { | ||
| return msg | ||
| } | ||
| return msg[:maxBriefErrorLength] + "..." | ||
| } |
Comment on lines
+41
to
+49
| // PackName returns a deterministic, DNS-1123-compliant object name that embeds | ||
| // both the pod name and the executor name. Because the name is unique per | ||
| // (pod, executor) pair, no two pods ever target the same ExecutorPodStatus | ||
| // object, which eliminates write conflicts. The name is reversible via | ||
| // UnpackName so aggregation can recover the executor name even from a delete | ||
| // event (where only the object name is available). | ||
| func PackName(podName, executorName string) string { | ||
| return encode(podName) + "-" + encode(executorName) | ||
| } |
Comment on lines
+123
to
+137
| // updateStatus records the outcome of the reconcile for this pod. | ||
| // | ||
| // When the pod identity is known, the outcome is written to a dedicated per-pod | ||
| // ExecutorPodStatus object (owned by the pod for automatic garbage collection), | ||
| // which a separate aggregation controller folds into Executor.status.byPod. | ||
| // This avoids all replicas writing the same Executor.status concurrently. When | ||
| // the pod identity is unknown, it falls back to writing the Executor status | ||
| // directly (single-writer, e.g. out-of-cluster usage). | ||
| func (r *ExecutorReconciler) updateStatus(ctx context.Context, executor *configv2alpha1.Executor, upsertErr error) { | ||
| if r.PodName == "" { | ||
| r.updateExecutorStatusDirectly(ctx, executor, upsertErr) | ||
| return | ||
| } | ||
| r.upsertPodStatus(ctx, executor, upsertErr) | ||
| } |
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
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 part of #2797.
When the provider deployment is scaled beyond
replicas: 1, every pod runs its ownExecutorReconcilerand they all write the sameExecutor.statusconcurrently. This PR lands the two minimal / immediate remediations from the issue:builder.WithPredicates(predicate.GenerationChangedPredicate{})to theFor(&Executor{})watch. Status-only writes don't bumpmetadata.generation, so they no longer re-triggerReconcile. This stops the reconcile→status-write→reconcile loop that repeatedly rebuilds the in-memory executor and hammers external providers (e.g. Azure Key Vault) — amplified ×N across replicas.retry.RetryOnConflict, re-fetching the object to pick up the latestresourceVersionbefore re-applying status. Previously an HTTP 409 was only logged, so the losing writer's update was silently dropped.Added unit tests (fake client + interceptor) covering the retry-on-conflict path and the error-recording path.
Intentionally out of scope
Remediations 3–5 in the issue (leader election, per-pod readiness/metrics, Gatekeeper-style
ExecutorPodStatus) are not included here. The full per-pod approach is proposed separately as an alternative direction.Testing
go build ./...,go vet ./internal/controller/...✅go test ./internal/controller/ -run TestUpdateStatus✅ (new unit tests)