feat(controller): surface poller and gate-workflow health as conditions and events#448
feat(controller): surface poller and gate-workflow health as conditions and events#448wankhede04 wants to merge 7 commits into
Conversation
…scribeTaskQueue calls Extend VersionInfo with PollerHealth/PollerHealthUnknown, populated for the current version (via DescribeVersion) and the target version (reusing task queues already fetched in GetTestWorkflowStatus), so poller presence data is no longer discarded after being reduced to AllTaskQueuesHaveUnversionedPoller.
Add ConditionWorkersHealthy plus ReasonPollersHealthy/ReasonNoActivePollers/ ReasonPollerStatusUnknown, so the WorkerDeployment CRD can report whether workers are actually polling Temporal rather than merely Ready at the Kubernetes level.
…ns and events Set ConditionWorkersHealthy from poller health of the version serving production traffic (current, falling back to target pre-rollout), emitting a Normal/Warning event only on transition. Also emit a Warning event the first time a gate/test workflow ends in Failed/Canceled/Terminated/TimedOut. This is reporting-only: no rollout/reconciliation logic changes. Closes temporalio#447
|
|
jaypipes
left a comment
jaypipes
left a comment
There was a problem hiding this comment.
Some good stuff in here, thank you @wankhede04 :) However, I'd love to see this split into two PRs, one that adds the warning event emission for gate workflow failures and another that adds the poller health checking.
| // serving production traffic (or, before the first rollout completes, the target | ||
| // version) are actively polling Temporal -- as opposed to merely being Ready at | ||
| // the Kubernetes level. | ||
| ConditionWorkersHealthy = "WorkersHealthy" |
There was a problem hiding this comment.
Is it necessary to create a new Condition type for this? How about adding ReasonNoActivePollers and ReasonPollerStatusUnknown but using the existing ConditionReady and adding the poller health check as part of the calculation of ConditionReady status of True for WorkerDeployment?
There was a problem hiding this comment.
Good call — done. Removed ConditionWorkersHealthy entirely. ReasonNoActivePollers and ReasonPollerStatusUnknown are now set directly on the existing ConditionReady (False/Unknown respectively) as part of its calculation when the target version's rollout has otherwise completed (VersionStatusCurrent), instead of a separate condition type. See the updated syncConditions in internal/controller/worker_controller.go.
| @@ -42,6 +57,25 @@ func (r *WorkerDeploymentReconciler) generateStatus( | |||
| // Continue without test workflow status | |||
| } | |||
|
|
|||
| // Emit a Warning event the first time a gate/test workflow is observed to have | |||
| // ended in a non-successful terminal state. Compare against the previous | |||
| // reconcile's recorded status (still on workerDeploy.Status at this point, since | |||
| // it hasn't been overwritten yet) so this doesn't re-fire on every loop. | |||
| prevStatusByWorkflowID := make(map[string]temporaliov1alpha1.WorkflowExecutionStatus, len(workerDeploy.Status.TargetVersion.TestWorkflows)) | |||
| for _, wf := range workerDeploy.Status.TargetVersion.TestWorkflows { | |||
| prevStatusByWorkflowID[wf.WorkflowID] = wf.Status | |||
| } | |||
| for _, wf := range testWorkflows { | |||
| if !isGateWorkflowTerminalFailure(wf.Status) { | |||
| continue | |||
| } | |||
| if prevStatusByWorkflowID[wf.WorkflowID] == wf.Status { | |||
| continue | |||
| } | |||
| r.Recorder.Eventf(workerDeploy, corev1.EventTypeWarning, ReasonGateWorkflowFailed, | |||
| "Gate/test workflow %s for version %s ended with status %s", wf.WorkflowID, targetBuildID, wf.Status) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I would support adding this code in a separate PR. It's technically orthogonal to the poller health check work.
| if err != nil { //nolint:revive // TODO(carlydf): consider logging this error | ||
| unknown = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
I suspect it would be wiser to just return the error here instead of continuing on to the next task queue. Considering that the error is most likely either going to be hitting a rate limit or a network partition/connectivity failure, making a call to DescribeTaskQueue directly after receiving either one of those errors is just going to exacerbate things.
There was a problem hiding this comment.
Makes sense, done. computePollerHealth now returns immediately on the first getPollers/DescribeTaskQueue error instead of continuing on to the remaining task queues. Task queues already checked before the error are still returned in the health map, and callers set PollerHealthUnknown=true on a non-nil error, so the 'unknown, not unhealthy' semantics are preserved.
Per review feedback from @jaypipes on PR temporalio#448: this PR should stay scoped to the poller-health-checking work. The gate/test workflow terminal-failure Warning event emission (isGateWorkflowTerminalFailure and the associated event loop in generateStatus, plus ReasonGateWorkflowFailed) is orthogonal to poller health and will be proposed in a separate PR instead.
…w condition Per review feedback from @jaypipes: rather than introducing a new ConditionWorkersHealthy condition type, reuse the existing ConditionReady and factor poller health into its calculation. - Drop ConditionWorkersHealthy from api/v1alpha1/conditions.go. - ReasonNoActivePollers and ReasonPollerStatusUnknown are now set on ConditionReady=False/Unknown (instead of a separate condition) when the target version has otherwise completed rollout (Status=Current) but its workers are not confirmed to be actively polling Temporal. ReasonPollersHealthy remains for the Normal event emitted on recovery. - syncConditions now takes the TemporalWorkerState so it can look up PollerHealth/PollerHealthUnknown for the relevant build ID and reflect it in Ready before declaring rollout success; Progressing and the deprecated RolloutComplete condition are unaffected, since those track rollout state only. - workers_healthy.go now holds only the pure, unit-tested computePollerHealthCondition helper; the condition-setting/event logic that used to live in syncWorkersHealthyCondition moved into syncConditions. - Updated TestSyncConditions plus added NotReadyWhenCurrentVersionHasNoActivePollers and ReadyUnknownWhenCurrentVersionPollerStatusUnknown cases.
…Queue error Per review feedback from @jaypipes: a DescribeTaskQueue failure is most likely a rate limit or a network partition/connectivity issue, so continuing on to call DescribeTaskQueue for the remaining task queues right after one of those errors would only make things worse. computePollerHealth now returns an error and stops on the first getPollers failure instead of setting an 'unknown' flag and continuing the loop. Task queues already checked before the error are still returned, and callers set PollerHealthUnknown=true on a non-nil error, preserving the existing 'unknown, not unhealthy' semantics.
|
Thanks for the review @jaypipes! Addressed all three points:
Re-verified |
… failure (#457) ## What was changed? - `internal/controller/genstatus.go`: add `isGateWorkflowTerminalFailure` and emit a Warning event (`GateWorkflowFailed`) the first time a gate/test workflow for the target version is observed to have ended in a non-successful terminal state (`Failed`, `Canceled`, `Terminated`, `TimedOut`). The previous reconcile's recorded status is compared against the freshly-fetched status so the event only fires once per transition, not on every reconcile loop. - `internal/controller/util.go`: add the `ReasonGateWorkflowFailed` event reason constant. ## Why? Split out of #448 per [review feedback from @jaypipes](#448 (review)): *"I'd love to see this split into two PRs, one that adds the warning event emission for gate workflow failures and another that adds the poller health checking."* This PR is the gate-workflow-failure half; #448 now contains only the poller-health-checking work. Also directly relates to #447 / #50: @carlydf's comment on #50 states *"We should surface worker healthcheck error and Gate workflow failure in the TemporalWorkerDeployment events for sure,"* calling out Gate workflow failure surfacing as in-scope independent of the post-GA rollout-acceptance-testing work. ## How was this tested? - `gofmt -l .` — clean - `go vet ./...` — clean - `go build ./...` — succeeds - `make GOLANGCI_LINT_FIX=false GOLANGCI_LINT_BASE_REV=main lint-code` — 0 issues - `go test ./internal/controller/...` — all pass ## Risks This is purely additive: one new event-reason constant and one new Warning event emission, with no changes to rollout/reconciliation decision logic or existing status fields. Co-authored-by: Jay Pipes <jay.pipes@temporal.io>
| // ReasonPollersHealthy is used on ConditionReady=True (in place of | ||
| // ReasonRolloutComplete) and on the Normal Event emitted when a version's workers | ||
| // transition from having no active pollers (or unknown status) back to actively | ||
| // polling every known task queue. | ||
| ReasonPollersHealthy = "PollersHealthy" |
There was a problem hiding this comment.
Please change this to ReasonActivePollers. The actual health of the poller isn't known :) Just that it's actively polling the server.
Also, I have changed my mind about having this reason affect ConditionReady. Instead, let's make this reason only impact ConditionProgressing=False so that it matches the existing ReasonWaitingForPollers that is set on ConditionProgressing=True.
| // Ready at the Kubernetes level while its workers are misconfigured, stuck, or | ||
| // unable to reach Temporal; this reason distinguishes that case from a healthy | ||
| // rollout. | ||
| ReasonNoActivePollers = "NoActivePollers" |
There was a problem hiding this comment.
I'm wondering if we need ReasonNoActivePollers at all since we have the existing ReasonWaitingForPollers. thoughts?
3 participants
What was changed?
internal/temporal/worker_deployment.go: extendVersionInfowithPollerHealth map[string]bool/PollerHealthUnknown bool, populated from poller data already fetched viaDescribeTaskQueue(no new round trips for the target version's task queues; one additionalDescribeVersioncall for the current version, since nothing previously fetched its task queues).api/v1alpha1/conditions.go/workerdeployment_types.go: addConditionWorkersHealthywith reasonsReasonPollersHealthy,ReasonNoActivePollers,ReasonPollerStatusUnknown.internal/controller/workers_healthy.go(new): a purecomputePollerHealthConditionfunction plussyncWorkersHealthyCondition, which sets the condition on the version currently serving production traffic (falling back to the target version before the first rollout completes) and emits a Warning/Normal event only on transition (not every reconcile).internal/controller/genstatus.go: emit aGateWorkflowFailedWarning event the first time a gate/test workflow transitions intoFailed/Canceled/Terminated/TimedOut, compared against the previous reconcile's recorded status so it doesn't re-fire every loop.Why?
Closes #447.
Today
HealthySince(api/v1alpha1/workerdeployment_types.go) is derived only fromk8s.IsDeploymentHealthy, i.e. Kubernetes Deployment/pod readiness. It has no visibility into whether Temporal is actually receiving polls — a Deployment can be fully Ready while its workers are misconfigured, stuck, or unable to reach Temporal, and today the controller has no way to surface that. Meanwhileinternal/temporal/worker_deployment.go'sgetPollers()already fetches the real poller list viaDescribeTaskQueue, but today it's reduced to a single internal bool (AllTaskQueuesHaveUnversionedPoller) used only for a rollout-strategy heuristic — the poller data itself is discarded.This also directly answers the non-post-GA part of #50: @carlydf's comment there states "We should surface worker healthcheck error and Gate workflow failure in the TemporalWorkerDeployment events for sure," explicitly separating this from the post-GA rollout-acceptance-testing work.
#61 (add URL field to version metadata) is intentionally out of scope here — it's a separately milestoned Post-GA follow-up that this work sets up for but doesn't attempt.
How was this tested?
gofmt -l .— cleango vet ./...— cleango build ./...— succeedsgo test ./internal/controller/... ./internal/temporal/... ./internal/planner/... ./internal/k8s/...— all pass, including a new table-driven unit test (workers_healthy_test.go) coveringcomputePollerHealthCondition's decision logic (nil/never-checked, all-healthy, one-queue-unhealthy, fetch-error-treated-as-unknown-not-unhealthy, and a confirmed-unhealthy-queue winning over an unrelated fetch error).make test-unit's webhook (Ginkgo) suite could not be run in my environment becausehelmisn't installed there; confirmed this failure is pre-existing on unmodifiedmaintoo (unrelated to this change) and is orthogonal to the reconciler/controller logic touched here.Risks
This is reporting-only: a new condition type, new event emissions, and new struct fields, with no changes to rollout/reconciliation decision logic (
internal/planner/planner.gois untouched). The one behavior-affecting addition is a newDescribeVersioncall for the current version each reconcile (needed since nothing previously fetched its task queues); aDescribeTaskQueuefailure is treated asUnknown, never surfaced asFalse/unhealthy, to avoid false alarms on transient errors.