Add event recording and status conditions for worker deployments by thearcticwatch · Pull Request #203 · temporalio/temporal-worker-controller

thearcticwatch · 2026-02-21T00:39:46Z

What changed: Added Kubernetes events and status conditions

(TemporalConnectionHealthy, RolloutReady) to the worker controller
reconciliation loop.

##Why: Reconciliation failures were only visible in controller logs —
events and conditions let users diagnose issues directly via kubectl.

Closes Add events to the TemporalWorkerDeployment CRD when there is a problem #28
How was this tested:
added unit tests and functional tests
Any docs updates needed?
N/A
Is this risky? Explain

Making a change to the CRD (adding conditions) opens up the risk that users could upgrade the controller but fail to upgrade their CRD. In this case, it is ok if new features are silently ignored, but we don't want the controller to panic or fail to successfully do the actions that were available in the previous CRD version. I believe that this change is safe even if someone forgets to upgrade their CRD, because when this new controller runs against a v1.2.0 CRD:

No panic. The controller calls r.Status().Update(ctx, twd) with conditions populated in memory. The API server validates against the CRD schema and prunes unknown fields (standard behavior for structural schemas without x-kubernetes-preserve-unknown-fields). The status write succeeds with a 200 and the conditions are silently dropped before storage.
Kubernetes Events work fine. Events are written as separate events.k8s.io/v1 resources, completely independent of the TWD CRD schema. All r.Recorder.Eventf(...) calls will succeed normally.
Conditions simply don't persist. kubectl get twd foo -o yaml will show no conditions field. The controller sets them in memory on every reconcile, tries to write, and the API server drops them. Functionally the controller does the right thing, it just can't communicate the health status via conditions until the CRD is upgraded.

CLAassistant · 2026-02-21T00:39:54Z

All committers have signed the CLA.

carlydf

also make fmt-imports will solve some of your lint errors

… usage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

carlydf

looking good! just did initial review, we should still add a functional test once these comments are addressed.

I found https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#events and https://book.kubebuilder.io/reference/raising-events#creating-events helpful while reviewing.

carlydf

really close from my perspective. will push a commit showing what I mean about the stricter string types for EventType and ConditionType.

"Registration" already has a meaning in Temporal versioning (a worker polling for the first time creates a version record). "Promotion" better describes setting a version as current or ramping, which moves it forward in the rollout lifecycle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…est scenarios ranked by trigger difficulty

…asonAuthSecretInvalid is untriggerable

…on-nil with empty name

…lientCreationFailed when Temporal had a problem

carlydf

Approved, but @Shivs11 if you could take a peek at my refactor of ClientPool which I did so we could emit a separate event type for invalid secret vs failed dial to Temporal server, that would be great!

)    ## What was changed - WISOTT - Note: This bug fix was an unfortunate regression that was introduced with [this](#203). This intends on fixing that. ## Why? - Bug fix! ## Checklist  1. Closes  2. How was this tested:  3. Any docs updates needed?

## Summary - Adds `clientpool_test.go` with 8 unit tests covering the auth code paths that had no test coverage - Two tests are explicit regression guards for the bugs fixed in #227 and #232 - Makes `dialFn` and `systemCertPoolFn` injectable on `ClientPool` (no behavior change in production) to enable testing without network I/O or OS trust store dependencies ## Regression tests **`TestFetchMTLS_CACertAppendsToSystemPool`** — guards against the PR #212 bug (fixed in #227): `fetchClientUsingMTLSSecret` used `x509.NewCertPool()` (empty) instead of `x509.SystemCertPool()`, silently dropping system root CAs and breaking Temporal Cloud connections. The test injects a fake system pool and verifies both the injected system CAs and the custom `ca.crt` are present in the returned pool. This test fails if the fix is reverted. **`TestDialAndUpsert_APIKeySkipsCheckHealth`** — guards against the PR #203 bug (fixed in #232): `DialAndUpsertClient` called `CheckHealth` unconditionally, which fails on Temporal Cloud with namespace-scoped API keys. The test uses an injected mock client and asserts `CheckHealth` is never called for `AuthModeAPIKey`. This test fails if the fix is reverted. ## Test plan - [x] `go test ./internal/controller/clientpool/... -v` — all 8 tests pass - [x] `go build ./...` — no compilation errors - [x] Manually revert the PR #227 fix → `TestFetchMTLS_CACertAppendsToSystemPool` fails - [x] Manually revert the PR #232 fix → `TestDialAndUpsert_APIKeySkipsCheckHealth` fails 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

…poralio#203) ## What changed: Added Kubernetes events and status conditions (TemporalConnectionHealthy, RolloutReady) to the worker controller reconciliation loop. ##Why: Reconciliation failures were only visible in controller logs — events and conditions let users diagnose issues directly via kubectl. 1. Closes temporalio#28 2. How was this tested: added unit tests and functional tests 3. Any docs updates needed? N/A 4. Is this risky? Explain Making a change to the CRD (adding conditions) opens up the risk that users could upgrade the controller but fail to upgrade their CRD. In this case, it is ok if new features are silently ignored, but we don't want the controller to panic or fail to successfully do the actions that were available in the previous CRD version. I believe that this change is safe even if someone forgets to upgrade their CRD, because when this new controller runs against a v1.2.0 CRD: - No panic. The controller calls r.Status().Update(ctx, twd) with conditions populated in memory. The API server validates against the CRD schema and prunes unknown fields (standard behavior for structural schemas without x-kubernetes-preserve-unknown-fields). The status write succeeds with a 200 and the conditions are silently dropped before storage. - Kubernetes Events work fine. Events are written as separate events.k8s.io/v1 resources, completely independent of the TWD CRD schema. All r.Recorder.Eventf(...) calls will succeed normally. - Conditions simply don't persist. kubectl get twd foo -o yaml will show no conditions field. The controller sets them in memory on every reconcile, tries to write, and the API server drops them. Functionally the controller does the right thing, it just can't communicate the health status via conditions until the CRD is upgraded. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Carly de Frondeville <cdefrondeville@berkeley.edu>

…emporalio#232)    ## What was changed - WISOTT - Note: This bug fix was an unfortunate regression that was introduced with [this](temporalio#203). This intends on fixing that. ## Why? - Bug fix! ## Checklist  1. Closes  2. How was this tested:  3. Any docs updates needed?

## Summary - Adds `clientpool_test.go` with 8 unit tests covering the auth code paths that had no test coverage - Two tests are explicit regression guards for the bugs fixed in temporalio#227 and temporalio#232 - Makes `dialFn` and `systemCertPoolFn` injectable on `ClientPool` (no behavior change in production) to enable testing without network I/O or OS trust store dependencies ## Regression tests **`TestFetchMTLS_CACertAppendsToSystemPool`** — guards against the PR temporalio#212 bug (fixed in temporalio#227): `fetchClientUsingMTLSSecret` used `x509.NewCertPool()` (empty) instead of `x509.SystemCertPool()`, silently dropping system root CAs and breaking Temporal Cloud connections. The test injects a fake system pool and verifies both the injected system CAs and the custom `ca.crt` are present in the returned pool. This test fails if the fix is reverted. **`TestDialAndUpsert_APIKeySkipsCheckHealth`** — guards against the PR temporalio#203 bug (fixed in temporalio#232): `DialAndUpsertClient` called `CheckHealth` unconditionally, which fails on Temporal Cloud with namespace-scoped API keys. The test uses an injected mock client and asserts `CheckHealth` is never called for `AuthModeAPIKey`. This test fails if the fix is reverted. ## Test plan - [x] `go test ./internal/controller/clientpool/... -v` — all 8 tests pass - [x] `go build ./...` — no compilation errors - [x] Manually revert the PR temporalio#227 fix → `TestFetchMTLS_CACertAppendsToSystemPool` fails - [x] Manually revert the PR temporalio#232 fix → `TestDialAndUpsert_APIKeySkipsCheckHealth` fails 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

thearcticwatch requested review from a team and jlegrone as code owners February 21, 2026 00:39

carlydf reviewed Feb 21, 2026

View reviewed changes

Comment thread internal/controller/worker_controller.go Outdated

carlydf reviewed Feb 21, 2026

View reviewed changes

thearcticwatch enabled auto-merge (squash) February 21, 2026 14:14

thearcticwatch and others added 4 commits February 21, 2026 12:26

First attempt at adding events to worker controller

15d7b9a

Adding events to worker controller

4495d80

fix the breaking tests

6c8e4f8

Fix golangci-lint errors: RBAC comment spacing and deprecated Requeue…

5900793

… usage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

thearcticwatch force-pushed the add_events branch from 5632069 to 5900793 Compare February 21, 2026 20:27

Refactor executePlan to reduce cognitive complexity

e65c9d1

carlydf reviewed Feb 24, 2026

View reviewed changes

Comment thread internal/controller/execplan.go Outdated

Comment thread internal/controller/execplan.go

Comment thread internal/controller/worker_controller.go Outdated

Comment thread internal/controller/worker_controller.go Outdated

addressing the comments

df3448a

Shivs11 reviewed Feb 27, 2026

View reviewed changes

Comment thread internal/controller/worker_controller.go Outdated

Comment thread internal/controller/execplan.go

Comment thread internal/controller/execplan.go Outdated

Comment thread internal/controller/worker_controller.go Outdated

thearcticwatch added 2 commits February 27, 2026 14:52

additional refactoring and addressing comments

0b76e06

Merge branch 'main' into add_events

06882a8

carlydf reviewed Mar 3, 2026

View reviewed changes

Comment thread internal/controller/execplan.go Outdated

Comment thread internal/controller/worker_controller.go Outdated

carlydf and others added 3 commits March 3, 2026 15:33

standardize event/condition reason constants and split by API stability

6e92e3a

add functional tests for events and conditions, enumerate remaining t…

11d382a

…est scenarios ranked by trigger difficulty

carlydf reviewed Mar 4, 2026

View reviewed changes

Comment thread internal/controller/worker_controller.go Outdated

carlydf added 3 commits March 3, 2026 17:09

only write to status once per reconcile loop

371a02f

print percentage correctly when it's an integer in [0,100]

65c8236

move all unhealthy connection tests into one function, explain why Re…

ddef655

…asonAuthSecretInvalid is untriggerable

carlydf reviewed Mar 4, 2026

View reviewed changes

Comment thread internal/controller/worker_controller.go Outdated

carlydf added 4 commits March 4, 2026 19:04

unit test failure events that are hard to test end to end

5fb51d2

refactor unit tests, do AuthSecretInvalid event when auth secret is n…

a8ac8fa

…on-nil with empty name

refactor to emit AuthSecretInvalid in more cases, only emit TemporalC…

9a0c6a0

…lientCreationFailed when Temporal had a problem

fix lint

5593873

carlydf approved these changes Mar 5, 2026

View reviewed changes

fix lint fmt.Errorf -> errors.New when unformatted

76069f1

carlydf disabled auto-merge March 6, 2026 02:43

carlydf enabled auto-merge (squash) March 6, 2026 02:43

carlydf closed this Mar 6, 2026

auto-merge was automatically disabled March 6, 2026 02:44
Pull request was closed

carlydf reopened this Mar 6, 2026

make planner nil-safe when DrainedSince is nil

dbff0f7

carlydf merged commit 872bc38 into main Mar 7, 2026
14 checks passed

carlydf deleted the add_events branch March 7, 2026 00:08

Shivs11 mentioned this pull request Mar 20, 2026

Bug fix: Do not call CheckHealth when authenticating with API keys #232

Merged

carlydf mentioned this pull request Mar 21, 2026

Add unit tests for clientpool auth code paths #236

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add event recording and status conditions for worker deployments#203

Add event recording and status conditions for worker deployments#203
carlydf merged 20 commits intomainfrom
add_events

thearcticwatch commented Feb 21, 2026 •

edited by carlydf

Loading

Uh oh!

CLAassistant commented Feb 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

carlydf left a comment

Uh oh!

carlydf left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlydf left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlydf left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

thearcticwatch commented Feb 21, 2026 • edited by carlydf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed: Added Kubernetes events and status conditions

Uh oh!

CLAassistant commented Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

carlydf left a comment

Choose a reason for hiding this comment

Uh oh!

carlydf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlydf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlydf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thearcticwatch commented Feb 21, 2026 •

edited by carlydf

Loading

CLAassistant commented Feb 21, 2026 •

edited

Loading