Conversation
…neration Adds the API surface needed for kstatus (sigs.k8s.io/cli-utils) to tell a failed rollout from a slow one. No behaviour change; nothing sets these yet. The condition names are the exact literals kstatus string-matches when assessing a custom resource, which is why they belong in the API package alongside Ready and Progressing rather than in the controller. Stalled and Reconciling must never both be True on the same object. kstatus scans status.conditions in array order and returns on the first match, so an object carrying both would get a verdict decided by insertion order — which could differ across controller restarts. The constants document this; the controller enforces it by removing one whenever it sets the other. WorkerResourceTemplateStatus gains a top-level observedGeneration. kstatus compares it against metadata.generation before it reads any condition, so without it a WRT could never report "I have not seen your latest edit yet". The per-condition observedGeneration the type already carried is not read by kstatus; only the top-level field is. Refs #478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A blocked rollout previously reported Ready=False and Progressing=False. kstatus does not read Progressing, so it fell back to Ready=False and concluded "still working" — meaning a rollout that could never succeed looked identical to a slow one, and Helm --wait or Flux would sit until timeout instead of failing with a reason. WorkerDeployment and WorkerResourceTemplate now set Stalled=True on terminal failures and Reconciling=True while work is genuinely in flight, with exactly one of the two ever set. Successful reconciles remove both, so a resource that recovers stops reporting Failed — meta.SetStatusCondition only upserts, so without an explicit removal Stalled would have been permanent. A failure is treated as terminal only when it is decidable from information already in hand, which for WorkerDeployment means InvalidSpec and ClusterConnectionUnsupported (see stalledReasons). Deliberately excluded are failures waiting on another object to exist — a missing Connection or credential Secret — because applying a WorkerDeployment alongside those in one release gives no ordering guarantee, so a missing reference is often a normal few-second gap rather than a mistake, and a false failure costs more than a slow one. Transient infrastructure failures are excluded for the same reason. WorkerResourceTemplate makes the same split by API error kind, since it has a single reason for every apply failure (see isTerminalWorkerResourceError). The trade-off is that a typo'd connectionRef still hangs to timeout, exactly as before this change; a grace period would fix that and is left as follow-up. Both types also advance status.observedGeneration on blocked paths. Most error paths return before the point where it was previously set, so editing a working resource into a broken one left it stale — and kstatus checks it before any condition, so it would have masked the new conditions entirely in the most common failure case. That last change required removing the generation gate on the connection finalizer release. Once blocked reconciles advance observedGeneration, the gate stops firing, and a connectionRef repointed at a Connection that does not exist yet would leave our finalizer on the old Connection forever, making it undeletable. The inner ObservedConnectionRef comparison was always the real test; the gate only saved two string compares. Note this specific path has no direct test — existing tests call releaseConnectionFinalizerIfUnused directly rather than driving it through Reconcile. The WorkerResourceTemplate status write also no longer skips unconditionally when every apply was a no-op. A spec edit that renders byte-identically bumps metadata.generation without changing any hash — reordering keys in spec.template does exactly that, since ComputeRenderedObjectHash marshals a map and Go sorts map keys — which would have left observedGeneration behind permanently. The refreshing Get is cache-served and the write still only happens when something changed, so the optimisation is preserved. Refs #478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds sigs.k8s.io/cli-utils as a test-only dependency and asserts what kstatus
concludes about each of the six CRD kinds, since that is what Helm --wait and
Flux will conclude. Objects are converted to unstructured and passed to the
real status.Compute, reproducing how those tools read our resources off the
wire rather than trusting a hand-written expectation.
Unit coverage (internal/controller/kstatus_test.go):
- WorkerDeployment across every rollout state, both blocking classes, the
recovery path, and deletion
- WorkerResourceTemplate across applied, terminal failure, transient
failure, an unobserved spec edit, and a missing WorkerDeployment
- the classification invariants: Stalled and Reconciling are never both
True, and a successful reconcile clears both
- isTerminalWorkerResourceError against real apierrors constructors
- Connection and ClusterConnection (Current — configuration only, no
controller), and both deprecated migration stubs (InProgress, plus
Terminating once marked for deletion, which is what keeps the documented
migration from stalling a release)
Integration coverage (internal/tests/internal/kstatus_integration_test.go)
runs the same verdicts against envtest and a real Temporal server, reading
each object back from the API server. Only this level can show that Stalled,
Reconciling and observedGeneration survive the CRD's OpenAPI schema and the
status subresource round trip — Stalled and Reconciling are new condition
types, and every unit test would still pass if the schema pruned them.
The deprecated-stub cases drive the real DeprecatedTWDReconciler and
DeprecatedTCReconciler rather than hand-building conditions, so they assert
what those stubs actually write.
Each exempt kind carries a comment explaining why its verdict is what it is,
so the tests double as the record of that decision: if someone later adds a
Ready condition to Connection, or makes a migration stub report ready, a test
fails and points at the reasoning.
Refs #478
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cd-rollouts.md described two conditions; there are now four. Documents the two pairs and why both exist: Ready and Progressing describe the rollout in the controller's own terms and carry the diagnostic detail, while Stalled and Reconciling say the same thing in the vocabulary kstatus reads. States which failures are terminal and which are not, and the reasoning, so the trade-off is discoverable: a typo'd connectionRef reports in-progress until your tool's timeout rather than failing immediately, because a missing reference cannot be told apart from a normal ordering gap during a deploy. Readers are pointed at the reason field and Events to see what is blocking. The recommended ArgoCD health check now reads Stalled and Reconciling instead of Progressing. This is a user-visible behaviour change: the previous script mapped Progressing=False to Degraded, which after this work would show Degraded for a Connection that is a second away from existing. Anyone still running the old script keeps the old semantics. Also notes the script can be registered for WorkerResourceTemplate, which emits the same conditions. Adds a note that Connection and ClusterConnection are configuration only — no controller, no conditions, so condition-based tools treat them as healthy as soon as they exist, as Kubernetes does for ConfigMap and Secret. Broken connections are still reported, on the referencing WorkerDeployment. migration-crd-rename.md notes that the deprecated kinds read as not-ready to condition-based health checks for as long as they exist, and that a resource already marked for deletion reports Terminating instead — so following the documented migration steps does not leave a release waiting. Refs #478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jaypipes
requested review from
a team,
eniko-dif and
jlegrone
as code owners
September 29, 2026 15:50
jaypipes
changed the title
eniko-dif
reviewed
Sep 30, 2026
eniko-dif
left a comment
eniko-dif
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, but I feel like the comments are extra verbose (sometimes hiding the actual code).
| * `InvalidSpec` is used when the spec you just applied is not valid. | ||
| * `ClusterConnectionUnsupported` is used when the type of Connection is not supported by the controller. | ||
|
|
||
| `Reconciling` is set to `True` when the controller is still working to make the observed state of the resource match the desired state of the resource, with the `Reason` field used to provide more detail about why `Reconciling=True`. |
Collaborator
There was a problem hiding this comment.
nit: this is repeated (first explained on line 52)
| hash string // rendered hash recorded on successful apply; "" on error | ||
| err error | ||
| skipped bool // true if the apply was skipped because the rendered hash is unchanged | ||
| // renderFailed distinguishes a spec.template render failure from an SSA apply |
Collaborator
There was a problem hiding this comment.
nit: this comment seems too verbose to me
| wrt := &temporaliov1alpha1.WorkerResourceTemplate{} | ||
| if err := r.Get(ctx, types.NamespacedName{Namespace: key.namespace, Name: key.name}, wrt); err != nil { | ||
| statusErrs = append(statusErrs, fmt.Errorf("get WRT %s/%s for status update: %w", key.namespace, key.name, err)) | ||
| // Every apply was a no-op and nothing was deleted, so the per-Build-ID |
Collaborator
There was a problem hiding this comment.
nit: this seems like a history rather than a helpful short comment (to me that is)
| // exist yet for example) would otherwise never be noticed again and the old connection | ||
| // would keep the finalizer forever. ObservedConnectionRef is only written on a | ||
| // successful reconcile, so it remains the correct thing to compare against. | ||
| current := workerDeploy.Spec.WorkerOptions.ConnectionRef |
Collaborator
There was a problem hiding this comment.
I might be wrong, but I think this new behavior is not tested? Maybe it would make sense to add a test that reconciles a WD with connection A (so ObservedConnectionRef = A), then edits the spec to connection B and makes the first reconcile block, creates B and reconciles again, and finally asserts the finalizer is removed from A.
| // WorkerDeployment does not exist yet reports Reconciling, not Stalled. See | ||
| // markWRTsWDNotFound. | ||
| var stalledReasons = map[string]bool{ | ||
| temporaliov1alpha1.ReasonInvalidSpec: true, |
Collaborator
There was a problem hiding this comment.
ReasonTemporalStateFetchFailed can arise due to a typo in the namespace too, but that is not a transient error. Could ReasonTemporalStateFetchFailed be split into two reasons, like a terminal ReasonTemporalNamespaceNotFound (added to stalledReasons), and ReasonTemporalStateFetchFailed kept transient for ResourceExhausted and network blips?
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
3 participants
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.
What was changed
The controller can now tell deployment tools the difference between a rollout that
is still working and one that has failed and will never finish.
WorkerDeploymentandWorkerResourceTemplatenow set two extra status conditions,StalledandReconciling. These are the exact names kstatus matches on.status.observedGeneration=metadata.generation.Connectionor missing credentials still report as working, because those can be temporarily absent for a couple seconds when everything is installed at once.ReadyandProgressingare unchanged.Connection,ClusterConnectionand the two deprecated resource types are left exactly as they are.One extra change: When you point a WorkerDeployment at a different Connection, the controller has to take its "in use" tag off the old one, or that old Connection can never be deleted. It used to skip that cleanup whenever the spec looked unchanged since the last run. Recording the spec version on error paths makes the spec look unchanged in exactly the case where the cleanup is needed, so the check now runs every time.
Why?
Resolves #478.
The CRs of this controller did not follow conventions kstatus expects, so nothing downstream could tell a broken rollout from a slow one. Argo users had to write a custom Lua script to read the conditions, and Helm and Flux had to wait out the full timeout on a rollout that does not work.
kstatus reads five things:
metadata.deletionTimestamp,status.observedGeneration, and the conditionsReconciling,StalledandReady. This PR addsReconcilingandStalled.Implementation Decisions
Three implementation decisions. Open to changing these.
Four of the six resource types were left alone on purpose.
ConnectionandClusterConnectionhave no controller behind them, so there is nothing to wait for. kstatus reports them as current the moment they exist, similarly to the way it does for ConfigMap and Secret. A bad connection is still reported, on theWorkerDeploymentthat uses it.The two deprecated resource types were also left alone. Never reporting ready for these types is deliberate and already documented. kstatus reads them as working for as long as they exist. That is fine in practice because the documented migration path deletes them as it goes, and kstatus reports a resource being deleted as terminating instead.
A missing
Connectionor missing credentials count as "still working", not "failed". This matches what the controller does today.Checklist
Closes Ensure that all CRD statuses are compatible with kstatus (used by Argo and Helm) #478
How was this tested:
New unit tests and new integration tests, both of which run the real kstatus
library over our resources and assert the verdict it reaches.
Integration tests run against a real API server and a real
Temporal server, then read each resource back before handing it to kstatus.
Covered: every rollout stage, a failure that can fix itself, a failure that cannot,
recovery afterwards, deletion, a spec edit the controller has not seen yet, and all
six resource types.
Done, in this PR:
docs/cd-rollouts.md: described two conditions, now four. Explains thatReadyand
Progressingare the ones to read yourself whileStalledandReconcilingexist for kstatus.
docs/migration-crd-rename.md: one note that the old resource types read asnot-ready to condition-based health checks until you finish migrating.
Note: the suggested Argo health check script in
cd-rollouts.mdchanged. It now readsStalledandReconcilinginstead ofProgressing, so ArgoCD agrees with Helm and Flux about what counts as a failure. The old version would show a resource as broken while it was waiting for itsConnectionto appear. Anyone still using the old script keeps the old behaviour.