feat(wrt): auto-inject KEDA Temporal trigger metadata for per-version ScaledObjects#351
feat(wrt): auto-inject KEDA Temporal trigger metadata for per-version ScaledObjects#351nikoul wants to merge 4 commits into
Conversation
537199f to
1b0f764
Compare
| assert.Equal(t, "my-tq", md["taskQueue"]) | ||
| }) | ||
|
|
||
| t.Run("overwrites pre-existing values when keys are present", func(t *testing.T) { |
There was a problem hiding this comment.
I think this is valid, but just an idea, in preparation for the auto-injection of spec.metrics[*].external.metric.selector.matchLabels, the WorkerResourceTemplate validating webhook rejects objects that have the fields set, to avoid confusion and be very explicit where the fields are coming from. I think it would be consistent to do KEDA templating similarly. See https://github.com/temporalio/temporal-worker-controller/blob/main/docs/worker-resource-templates.md#auto-injection for existing semantics.
There was a problem hiding this comment.
Also, it would be most consistent if the controller templates the namespace field in the Temporal trigger too. The idea being, we know the Temporal namespace that the Worker Deployment is in, so no need for the user to mis-configure it.
There was a problem hiding this comment.
Thanks, I think I've implemented something that answers both those suggestions now.
…mentBuildId Mirror the scaleTargetRef opt-in pattern for KEDA Temporal trigger metadata: present + empty string is the opt-in sentinel; present + non-empty value is rejected by the validating webhook with a clear error message. Absent keys are unchanged (no injection). Runtime injection in appendTemporalTriggerMetadata becomes strict: only overwrites when the value is the empty-string sentinel. This is defence in depth — if a non-empty value somehow reaches runtime (webhook bypassed, existing resources), the user-provided value is preserved rather than silently overwritten, matching the scaleTargetRef isEmptyMap pattern. Existing tests updated to assert the new strict semantics. New TestWorkerResourceTemplate_ValidateCreate_TemporalTriggerMetadata covers opt-in, rejection, and non-temporal-trigger pass-through. A small KEDA-aware test validator (newValidatorNoAPIWithKEDA) is added so KEDA-specific cases can reach the new validation without being rejected by the allow-list. Addresses review comment on PR temporalio#351.
… triggers The Temporal Cloud namespace is known to the controller via the TemporalConnection, so users should not need to repeat it on every WorkerResourceTemplate. Extend the trigger-metadata injection to also fill in namespace when the user opts in with the empty-string sentinel, and reject non-empty values in the webhook (mirroring the existing workerDeploymentName / workerDeploymentBuildId pattern). Threads temporalNamespace through autoInjectFields and appendTemporalTriggerMetadata. New test asserts injection happens for the empty-string sentinel and that an absent key remains absent (opt-in semantics). New webhook test case asserts rejection of a hardcoded value. Addresses review comment on PR temporalio#351.
d875de6 to
2a34ef4
Compare
… ScaledObjects Extend WorkerResourceTemplate auto-injection to populate workerDeploymentName and workerDeploymentBuildId in KEDA ScaledObject triggers[*].metadata when the trigger type is "temporal". Unblocks using KEDA's Temporal scaler (kedacore/keda#7672) as the templated resource for per-version backlog scaling. Injection is opt-in: keys are only touched when already present in the template's metadata map, mirroring the existing metrics matchLabels merge pattern. Non-temporal triggers in the same ScaledObject are left untouched. Refs temporalio#286.
…wdName = WorkerDeployment k8s resource name
…mentBuildId Mirror the scaleTargetRef opt-in pattern for KEDA Temporal trigger metadata: present + empty string is the opt-in sentinel; present + non-empty value is rejected by the validating webhook with a clear error message. Absent keys are unchanged (no injection). Runtime injection in appendTemporalTriggerMetadata becomes strict: only overwrites when the value is the empty-string sentinel. This is defence in depth — if a non-empty value somehow reaches runtime (webhook bypassed, existing resources), the user-provided value is preserved rather than silently overwritten, matching the scaleTargetRef isEmptyMap pattern. Existing tests updated to assert the new strict semantics. New TestWorkerResourceTemplate_ValidateCreate_TemporalTriggerMetadata covers opt-in, rejection, and non-temporal-trigger pass-through. A small KEDA-aware test validator (newValidatorNoAPIWithKEDA) is added so KEDA-specific cases can reach the new validation without being rejected by the allow-list. Addresses review comment on PR temporalio#351.
… triggers The Temporal Cloud namespace is known to the controller via the TemporalConnection, so users should not need to repeat it on every WorkerResourceTemplate. Extend the trigger-metadata injection to also fill in namespace when the user opts in with the empty-string sentinel, and reject non-empty values in the webhook (mirroring the existing workerDeploymentName / workerDeploymentBuildId pattern). Threads temporalNamespace through autoInjectFields and appendTemporalTriggerMetadata. New test asserts injection happens for the empty-string sentinel and that an absent key remains absent (opt-in semantics). New webhook test case asserts rejection of a hardcoded value. Addresses review comment on PR temporalio#351.
2a34ef4 to
5e96a4e
Compare
Summary
Extends
WorkerResourceTemplateauto-injection to populate theworkerDeploymentNameandworkerDeploymentBuildIdfields in KEDAScaledObjecttriggers[*].metadatawhen the triggertypeis"temporal". This unblocks using KEDA's Temporal scaler (kedacore/keda#7672) as the templated resource for per-version backlog scaling, as requested in #286.Closes (or contributes towards) #286.
Motivation
For users who already run KEDA cluster-wide for non-Temporal workloads, KEDA owns the
external.metrics.k8s.ioAPIService — only one external metrics provider can be installed per cluster. That makes the documented WRT-HPA + prometheus-adapter path unavailable: prometheus-adapter cannot co-exist with KEDA on the same APIService.KEDA's recent Worker Deployment Versioning support (kedacore/keda#7672) makes it possible for a KEDA
ScaledObjectto scale a single worker version by querying Temporal Cloud directly withworkerDeploymentName+workerDeploymentBuildId. WRT already supports templating arbitrary kinds (verified:spec.templateis unstructured, SSA-applied, with a kind allow-list in the chart) andscaleTargetRef: {}auto-fill works recursively. The only missing piece was injecting the per-version identifiers into the KEDA trigger metadata — same source values the controller already injects into HPA External metric selectors, different JSON path.Behaviour
For each templated resource that contains:
the controller sets:
workerDeploymentName←wrt.Spec.EffectiveWorkerDeploymentName()workerDeploymentBuildId← the per-version build IDInjection is opt-in: the keys are only touched if already present in the template's
metadatamap (mirroring the existingmetrics[*].external.metric.selector.matchLabelsmerge pattern). Non-temporaltriggers in the sameScaledObjectare left untouched.Example
The controller will stamp one
ScaledObjectper active worker version, each with the correctworkerDeploymentBuildId, and delete them on sunset via the existingDeleteWorkerResourcesplanner (kind-agnostic).Implementation
appendTemporalTriggerMetadata(spec, twdName, buildID)— new helper that walksspec.triggers[*], filters ontype == "temporal", and sets the two keys if already present.autoInjectFieldssignature now takestwdName, buildIDso it can pass them through. Behaviour for HPA / matchLabels / scaleTargetRef is unchanged.Helm-side: adding
ScaledObjecttoworkerResourceTemplate.allowedResources+ the controller'sClusterRoleis left as a follow-up (or a separate commit in this PR if maintainers prefer). For users who want this today, they can add the entry to their chart values without further controller changes.Tests
New
TestAutoInjectFields_TemporalTriggerMetadatacovers:temporaltriggers are untouched (heterogeneous trigger array)scaleTargetRef: {}auto-fill still works onScaledObjectspec.triggersis absentAll existing tests still pass (
go test ./...).