feat(controller): per-pod Executor status (Gatekeeper-style) for multi-replica safety - #2827
Closed
fseldow wants to merge 1 commit into
Closed
feat(controller): per-pod Executor status (Gatekeeper-style) for multi-replica safety#2827fseldow 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 26, 2026 08:30
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2827 +/- ##
==========================================
- 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
Implements Gatekeeper-style per-replica status reporting for multi-replica safety by introducing a per-pod ExecutorPodStatus CRD written by each Ratify pod, plus an aggregation controller that rebuilds Executor.status.byPod without concurrent-writer conflicts. This fits into the controller/CRD layer by separating “per-pod health reporting” from “parent CR aggregate status”, while also breaking the status-update feedback loop via a generation predicate.
Changes:
- Add namespaced
ExecutorPodStatusCRD + Go API types/deepcopies and a reversible (base32)(pod, executor)name pack/unpack helper. - Update
ExecutorReconcilerto write per-pod status objects (with pod ownerRef) and addGenerationChangedPredicateto stop status-write feedback loops; add an aggregation reconciler to computeExecutor.status.byPodand derived top-level fields. - Update RBAC and manager deployment to support pod identity discovery (
POD_NAME) and the new CRD permissions.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/podstatus/name.go | Adds base32 pack/unpack helpers and label keys for per-pod status object identity. |
| internal/podstatus/name_test.go | Unit tests for name pack/unpack, DNS-1123 character compliance, and uniqueness. |
| internal/pod/info.go | Adds pod.Name() helper to read POD_NAME. |
| internal/manager/manager.go | Wires pod identity into ExecutorReconciler and registers the new aggregation reconciler. |
| internal/controller/executorpodstatus_controller.go | New controller that aggregates ExecutorPodStatus into Executor.status.byPod and derived status. |
| internal/controller/executorpodstatus_controller_test.go | Unit tests for per-pod write path and aggregation behavior. |
| internal/controller/executor_controller.go | Moves status writes to per-pod objects (with conflict retries), adds generation predicate, and adds helper utilities. |
| internal/controller/executor_controller_retry_test.go | Unit tests for retry-on-conflict behavior in the direct (out-of-cluster) status path. |
| config/rbac/role.yaml | Updates manager ClusterRole for pods + executorpodstatuses + executor status updates. |
| config/manager/manager.yaml | Injects POD_NAME and RATIFY_NAMESPACE into the manager pod via downward API. |
| config/crd/kustomization.yaml | Adds the new executorpodstatuses CRD to the CRD kustomization. |
| config/crd/bases/config.ratify.dev_executors.yaml | Extends Executor.status schema with byPod entries. |
| config/crd/bases/config.ratify.dev_executorpodstatuses.yaml | New CRD definition for namespaced ExecutorPodStatus with status subresource. |
| 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 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
+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
+68
to
+76
| env: | ||
| - name: POD_NAME | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.name | ||
| - name: RATIFY_NAMESPACE | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.namespace |
…i-replica Fixes #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. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fseldow
force-pushed
the
fix/2797-per-pod-executor-status
branch
from
July 27, 2026 03:09
bcf48d0 to
c0323de
Compare
Contributor
Author
|
Note on merge order with #2829: this PR intentionally keeps the legacy If #2829 merges first, please rebase this branch onto |
Contributor
Author
|
Superseded by #2831, opened from a fork following the standard contribution flow. The branch pushed directly to the upstream repo has been removed. |
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 #2825 (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 regeneratedinternal/podstatus: name pack/unpack round-trip, DNS-1123 compliance, uniqueness.internal/controller: per-pod write path (writesExecutorPodStatuswith owner ref, leavesExecutor.statusuntouched), aggregation logic +ExecutorPodStatusReconciler.Reconcile.Note: the existing ginkgo envtest suite was not run locally due to an unrelated
/etc/hostsmisconfiguration (envtest apiserver can't bind); it compiles and its direct-Reconcileassertions still hold via the out-of-cluster fallback path.Refs #2825.