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 10 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.
| // isGateWorkflowTerminalFailure reports whether a test/gate workflow status | ||
| // represents an ended-but-not-successful terminal state worth alerting on. | ||
| func isGateWorkflowTerminalFailure(status temporaliov1alpha1.WorkflowExecutionStatus) bool { | ||
| switch status { | ||
| case temporaliov1alpha1.WorkflowExecutionStatusFailed, | ||
| temporaliov1alpha1.WorkflowExecutionStatusCanceled, | ||
| temporaliov1alpha1.WorkflowExecutionStatusTerminated, | ||
| temporaliov1alpha1.WorkflowExecutionStatusTimedOut: | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| func (r *WorkerDeploymentReconciler) generateStatus( | ||
| ctx context.Context, | ||
| l logr.Logger, | ||
| temporalClient temporalclient.Client, | ||
| req ctrl.Request, | ||
| workerDeploy *temporaliov1alpha1.WorkerDeployment, | ||
| temporalState *temporal.TemporalWorkerState, | ||
| k8sState *k8s.DeploymentState, | ||
| ) (*temporaliov1alpha1.WorkerDeploymentStatus, error) { | ||
| workerDeploymentName := k8s.ComputeWorkerDeploymentName(workerDeploy) | ||
| targetBuildID := k8s.ComputeBuildID(workerDeploy) | ||
|
|
||
| // Fetch test workflow status for the desired version | ||
| if targetBuildID != temporalState.CurrentBuildID { | ||
| testWorkflows, err := temporal.GetTestWorkflowStatus( | ||
| ctx, | ||
| temporalClient, | ||
| workerDeploymentName, | ||
| targetBuildID, | ||
| workerDeploy, | ||
| temporalState, | ||
| ) | ||
| if err != nil { | ||
| l.Error(err, "error getting test workflow status") | ||
| // 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.
There was a problem hiding this comment.
Good call — done. Renamed ReasonPollersHealthy to ReasonActivePollers. Also moved this signal off ConditionReady entirely: poller presence for the current version is now surfaced only on ConditionProgressing (True/ReasonWaitingForPollers when a task queue has no active poller, False/ReasonActivePollers when all do), mirroring the existing ReasonWaitingForPollers pair rather than affecting ConditionReady. ConditionReady=True/ReasonRolloutComplete no longer depends on poller presence.
| // 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?
There was a problem hiding this comment.
Agreed — removed ReasonNoActivePollers entirely. The missing-poller case now reuses the existing ReasonWaitingForPollers (set on ConditionProgressing=True), same as you suggested.
…ers, drop ReasonNoActivePollers Poller presence is now surfaced only on ConditionProgressing (True/ReasonWaitingForPollers when a task queue has no active poller, False/ReasonActivePollers when all do), matching the existing ReasonWaitingForPollers pair instead of a separate ConditionReady reason. ConditionReady=True/ReasonRolloutComplete no longer depends on poller presence. Addresses review feedback from @jaypipes on temporalio#448.
|
Thanks @jaypipes — addressed both remaining points from your last review:
Re-verified This should be the last of the open feedback on this PR. Would appreciate a merge at your earliest convenience — happy to make any further adjustments if needed. |
|
@jaypipes Can u please merge this PR |
@wankhede04 this PR is going into the v1.9.0 release series. I am working to cut the v1.8.1 release series today so I will merge this PR once that release is cut. |
|
Sounds good, thanks @jaypipes! No rush — appreciate you keeping this queued for right after the v1.8.1 cut. Let me know if there's anything else needed on my end in the meantime. |
@wankhede04 please do update the PR summary to make it accurate with all the changes made! :) |
|
@jaypipes done — updated the PR summary to accurately reflect the current diff (poller health folded into the existing |
| // Poller health for the target version, computed from the task queues already | ||
| // fetched above via DescribeVersion (no additional per-version round trip). | ||
| var pollerErr error | ||
| temporalState.Versions[buildID].PollerHealth, pollerErr = computePollerHealth(ctx, client, versionResp.Info.TaskQueuesInfos) //nolint:revive // TODO(carlydf): consider logging this error |
There was a problem hiding this comment.
I might be wrong, but I think this is not needed, since you only use the PollerHealth information when the version status is current, and in that case, you compute the poller health above.
| // Keyed by task queue name; true = has at least one poller. A task queue is absent | ||
| // from the map if its poller status could not be determined (e.g. a transient | ||
| // DescribeTaskQueue error) -- see PollerHealthUnknown. | ||
| PollerHealth map[string]bool |
There was a problem hiding this comment.
I think the name PollerHealth is not that descriptive as a name as it does not refer to the health of the workers. Based on the computePollerHealthCondition function, you are looking for task queues without pollers, so I'd simplify this to TaskQueuesWithoutPollers []string and only gather task queues of interest in computePollerHealth. (The functions could be renamed accordingly.)
| // PollerHealthUnknown is true if poller status could not be determined for one or | ||
| // more of this version's task queues. Callers must not interpret this as unhealthy; | ||
| // it means "don't know", not "broken". | ||
| PollerHealthUnknown bool |
There was a problem hiding this comment.
Maybe this could be TaskQueueDescribeError error and either be nil or the error directly?
| }) | ||
| if descErr == nil { | ||
| var pollerErr error | ||
| versionInfo.PollerHealth, pollerErr = computePollerHealth(ctx, client, currentDesc.Info.TaskQueuesInfos) //nolint:revive // TODO(carlydf): consider logging this error |
There was a problem hiding this comment.
Since this is a heavy operation, could this be checked less frequently by tracking the last task queue pollers check time? I think once every 5-10 minutes would be sufficient.
| ) (health map[string]bool, err error) { | ||
| health = make(map[string]bool, len(tqs)) | ||
| for _, tqInfo := range tqs { | ||
| pollers, err := getPollers(ctx, client, tqInfo) |
There was a problem hiding this comment.
Based on the comment, if the issue is potentially hitting rate-limits, wouldn't it make sense to pause for a short time between task queue describe calls?
4 participants
What was changed?
internal/temporal/worker_deployment.go: extendVersionInfowithPollerHealth map[string]bool(per-task-queue: has at least one poller) andPollerHealthUnknown 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/workerdeployment_types.go: addReasonActivePollersandReasonPollerStatusUnknown, and broaden the existingReasonWaitingForPollersdoc comment to also cover "current version has become current but has task queues with no active poller" (not just "not yet registered with Temporal").internal/controller/workers_healthy.go(new): a purecomputePollerHealthCondition(pollerHealth, unknown) (status, reason, affectedQueues)function that decides, from a version's poller presence data, whetherConditionProgressingshould reportReasonActivePollers,ReasonWaitingForPollers(naming the affected queues), orReasonPollerStatusUnknown. A confirmed missing-poller queue always takes precedence over an unrelated fetch error.internal/controller/worker_controller.go:syncConditionsnow takes the fetchedtemporalStateand, when the target version isCurrent, factors poller presence for the serving version (current version if set, else target) intoConditionProgressingusingcomputePollerHealthCondition.ConditionReadyis unaffected — it continues to reflect rollout completion only, matching its pre-existing (Kubernetes-readiness-only) semantics for backward compatibility.setConditionnow returns whether the condition actually changed, used to emit a Warning/Normal event only on a poller-status transition (not every reconcile); no event is emitted for theUnknownreason since it isn't itself a state transition worth alerting on.This is a smaller, more conservative shape than earlier revisions of this PR: poller health is folded into the existing
ConditionProgressing/ConditionReadyconditions rather than introduced as a newConditionWorkersHealthycondition type, and the gate/test-workflow-failure event work has been split out into a separate PR (#457, already merged) to keep this change focused on poller health alone.Why?
Closes #447.
Today
ConditionReady/ConditionProgressingare derived only from Kubernetes Deployment/pod readiness and Temporal rollout status — they have no visibility into whether Temporal is actually receiving polls. A Deployment can be fully Ready and its version fully rolled out (Current) 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 otherwise discarded.This 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. The gate-workflow-failure half of that has already landed via #457; this PR covers the poller-health half.
#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:workers_healthy_test.go) coveringcomputePollerHealthCondition's decision logic (nil/never-checked, all-healthy, one-queue-unhealthy, empty-map/no-queues-to-check, fetch-error-treated-as-unknown-not-unhealthy, and a confirmed-unhealthy-queue winning over an unrelated fetch error).reconciler_events_test.gocases coveringConditionProgressingtransitions for a Current version with active pollers, no active pollers, and unknown poller status.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: new condition reasons and new
VersionInfofields, with no changes to rollout/reconciliation decision logic (internal/planner/planner.gois untouched by this PR). 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 as an active poller problem, to avoid false alarms on transient errors.