feat(controller): per-pod Executor status (Gatekeeper-style) for multi-replica safety - #2831
Open
fseldow wants to merge 1 commit into
Open
feat(controller): per-pod Executor status (Gatekeeper-style) for multi-replica safety#2831fseldow wants to merge 1 commit into
fseldow wants to merge 1 commit into
Conversation
fseldow
requested review from
akashsinghal,
binbin-li,
jimmyraywv,
luisdlp,
susanshi and
toddysm
as code owners
July 27, 2026 03:14
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a Gatekeeper-style per-pod status reporting model for the cluster-scoped Executor to make multi-replica deployments safe (eliminating concurrent writes to a shared status object) and to stop status-update feedback loops by generation-filtering the Executor watch.
Changes:
- Add a new namespaced CRD
ExecutorPodStatusplus helpers to encode/decode deterministic per-pod object names. - Update
ExecutorReconcilerto write per-podExecutorPodStatus(and fall back to directExecutor.statusupdates whenPOD_NAMEis unavailable), and addGenerationChangedPredicateto break reconcile storms. - Add
ExecutorPodStatusReconcilerto aggregate per-pod objects intoExecutor.status.byPod[], plus RBAC/env wiring and unit tests.
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 name packing/unpacking behavior. |
| internal/pod/info.go | Adds pod.Name() sourced from POD_NAME. |
| internal/manager/manager.go | Wires pod identity into ExecutorReconciler and registers the aggregation controller. |
| internal/controller/executorpodstatus_controller.go | New controller to aggregate per-pod status objects back into Executor.status.byPod. |
| internal/controller/executorpodstatus_controller_test.go | Tests for per-pod write path and aggregation logic. |
| internal/controller/executor_controller.go | Writes per-pod ExecutorPodStatus, adds generation predicate, and adds conflict-retry for direct status fallback. |
| internal/controller/executor_controller_retry_test.go | Tests conflict-retry and error recording for direct Executor.status writes. |
| config/rbac/role.yaml | Adds RBAC for pods and executorpodstatuses resources/subresources. |
| config/manager/manager.yaml | Injects POD_NAME and RATIFY_NAMESPACE via the downward API. |
| config/crd/kustomization.yaml | Includes the new executorpodstatuses CRD base. |
| config/crd/bases/config.ratify.dev_executors.yaml | Extends Executor.status schema with byPod. |
| config/crd/bases/config.ratify.dev_executorpodstatuses.yaml | New CRD definition for ExecutorPodStatus. |
| api/v2alpha1/zz_generated.deepcopy.go | Regenerates deep-copies for new types and ExecutorStatus.ByPod. |
| api/v2alpha1/executorpodstatus_types.go | Adds API types for ExecutorPodStatus and PodStatusEntry. |
| api/v2alpha1/executor_types.go | Adds ExecutorStatus.ByPod field and kubebuilder list-map annotations. |
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
+47
to
+49
| func PackName(podName, executorName string) string { | ||
| return encode(podName) + "-" + encode(executorName) | ||
| } |
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
+79
to
+81
| if itemExecutor == executorName { | ||
| byPod = append(byPod, list.Items[i].Status) | ||
| } |
Comment on lines
+254
to
+258
| if len(msg) <= maxBriefErrorLength { | ||
| return msg | ||
| } | ||
| return msg[:maxBriefErrorLength] + "..." | ||
| } |
fseldow
force-pushed
the
fix/2797-per-pod-executor-status
branch
from
August 6, 2026 06:56
c0323de to
2d40241
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2831 +/- ##
==========================================
+ Coverage 76.89% 77.27% +0.37%
==========================================
Files 90 92 +2
Lines 4276 4457 +181
==========================================
+ Hits 3288 3444 +156
- Misses 831 849 +18
- Partials 157 164 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fseldow
force-pushed
the
fix/2797-per-pod-executor-status
branch
4 times, most recently
from
August 6, 2026 08:38
319b0ff to
8dc3bce
Compare
…i-replica Fixes notaryproject#2797. When the provider is scaled beyond replicas: 1, every pod runs its own ExecutorReconciler and they all write the same Executor.status concurrently: 409 write conflicts are silently lost, a status-write feedback loop amplifies reconciles xN, and a single succeeded/error field flaps under last-writer-wins so you cannot tell '2 of 5 replicas are unhealthy'. This adopts Gatekeeper's per-pod *PodStatus pattern: - New namespaced CRD ExecutorPodStatus. Each pod owns exactly one object per Executor; the name embeds the pod identity via a reversible base32 packing (PackName/UnpackName), so no two pods share an object -> no write conflicts. The object carries an owner reference to the pod, so it is garbage-collected automatically when the pod goes away. - ExecutorReconciler now writes its own ExecutorPodStatus (create-or- update + status update with RetryOnConflict) instead of the shared Executor.status. Each pod still builds its in-memory executor, so the data plane keeps working on every replica. - New ExecutorPodStatusReconciler watches all ExecutorPodStatus objects and rebuilds Executor.status.byPod[] (full, idempotent rebuild; a deleted pod's entry simply disappears). Parent updates are retried on conflict, not swallowed. Top-level succeeded/error/briefError are derived from the aggregate (e.g. 'N/M replicas unhealthy'). - Keep GenerationChangedPredicate on the Executor watch to break the status feedback loop. Out-of-cluster usage (no POD_NAME) falls back to writing Executor.status directly with RetryOnConflict. - Inject POD_NAME via the downward API; add pods get/list/watch and executorpodstatuses RBAC; regenerate CRDs, RBAC and deepcopy. - Unit tests for name packing, the per-pod write path, and aggregation. - Sync the ratify-gatekeeper-provider Helm chart: add the ExecutorPodStatus CRD, pods + executorpodstatuses RBAC, and the POD_NAME downward-API env so the deployed manager can start its per-pod status controllers. - Make per-pod status writes reliable: write the status subresource from the object CreateOrUpdate returns (a re-read through the informer cache can miss a just-created object and drop the status via a non-retried NotFound), and requeue the Executor after a failed build so a transient startup error recovers on its own despite the generation-filtered watch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: ratify-contrib <noreply@github.com>
fseldow
force-pushed
the
fix/2797-per-pod-executor-status
branch
from
August 6, 2026 08:49
8dc3bce to
8f43a3a
Compare
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 #2797. This is the full Gatekeeper-style per-pod status remediation (item 5 in the issue). It is an alternative to the minimal #2830 (predicate + retry only); maintainers can pick either direction.
Problem
With
replicas > 1, every pod runs its ownExecutorReconcilerand they all write the sameExecutor.statusconcurrently:GenerationChangedPredicate→ each status write re-triggers reconcile → executor rebuild storms against providers (e.g. AKV), amplified ×N.succeeded/errorfield is last-writer-wins → it flaps and can't express "2 of 5 replicas unhealthy".Approach — Gatekeeper's per-pod
*PodStatuspatternExecutorPodStatus. Each pod owns exactly one object per Executor. The name embeds the pod identity via a reversible base32 packing (PackName/UnpackName), so no two pods share an object → no write conflicts. An owner reference to the pod means the object is garbage-collected when the pod is deleted. (Namespaced, because a cluster-scoped object can't be owned by a namespaced pod — same as Gatekeeper.)ExecutorReconcilerwrites its ownExecutorPodStatus(create-or-update +Status().UpdatewithRetryOnConflict) instead of the shared status. Each pod still builds its in-memory executor, so the data plane keeps working on every replica.ExecutorPodStatusReconcilerwatches allExecutorPodStatusobjects and rebuildsExecutor.status.byPod[](full, idempotent rebuild; a deleted pod's entry just disappears — recovered from the reversible name even on delete events). Parent updates are retried on conflict, not swallowed. Top-levelsucceeded/error/briefErrorare derived from the aggregate (N/M replicas unhealthy).GenerationChangedPredicateon the Executor watch to break the feedback loop. Out-of-cluster usage (noPOD_NAME) falls back to writingExecutor.statusdirectly withRetryOnConflict.POD_NAMEvia the downward API; addpods get/list/watch+executorpodstatusesRBAC; regenerate CRDs/RBAC/deepcopy.No feedback loop
ExecutorPodStatusis written only byExecutorReconciler; the aggregator writes onlyExecutor.status; the Executor watch is generation-filtered → the aggregator's writes never re-triggerExecutorReconciler, and the aggregator never writesExecutorPodStatus. Stable.Testing
go build ./...,go vet ./...,gofmt✅make manifests generate(controller-gen v0.18.0) ✅ — CRDs/RBAC/deepcopy regeneratedExecutorPodStatusReconciler.Reconcile).Note on RBAC / merge order with #2832
This PR keeps the legacy
config.ratify.deislabs.ioClusterRole rules untouched so its RBAC diff is limited to the new per-pod permissions. Removing those dead rules is split into #2832. If #2832 merges first, rebase this branch ontomainand drop the reintroducedconfig.ratify.deislabs.ioblock fromconfig/rbac/role.yaml(make manifestsregenerates the cleaned-up result).Refs #2830, #2832.