Add kstatus functionality to CRD statuses - #585
Open
zainawaisn wants to merge 5 commits into
Open
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>
The kstatus integration tests added in the previous commit import sigs.k8s.io/cli-utils, which the tests module's go.mod did not declare. It still built, because CI and `make test-integration` construct a go.work workspace whose build list supplies the dependency from the root module. `make tidy` runs `GOWORK=off go mod tidy` per module, so without this the linters workflow's tidy job leaves the tree dirty and fails. Refs #478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zainawaisn
requested review from
a team,
eniko-dif and
jlegrone
as code owners
September 10, 2026 17:57
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
1 participant
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 statusconditions,
StalledandReconciling. These are the exact nameskstatus
matches on — the library behind Helm 4's
--waitand Flux's health checks. Itnever reads our existing
Progressingcondition, which is why we could not justkeep using that one.
even when they stopped because of an error. kstatus checks that before it reads
any condition, so without it the two new conditions are ignored entirely and the
resource just reports "still working".
or a connection type this controller is not set up to read. A missing
Connectionor missing credentials still report "still working", because when you install
everything at once those can genuinely be absent for a second or two through no
fault of yours. Failing a deploy that was about to succeed is worse than one that
takes a while.
ReadyandProgressingare unchanged, so existing dashboards and alerts keepworking.
Connection,ClusterConnectionand the two deprecated resource types are leftexactly 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.
Our resources did not follow the 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 our 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. Everything this PR adds is one of those five.Open Questions
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 waitfor. kstatus reports them as current the moment they exist, the same treatment
it gives ConfigMap and Secret. A bad connection is still reported, on the
WorkerDeploymentthat 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 "still working" for
as long as they exist. That is fine in practice because the documented migration
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.