feat: add cronWorkflowDefaults to controller config. Fixes #12627#16277
feat: add cronWorkflowDefaults to controller config. Fixes #12627#16277alien2003 wants to merge 2 commits into
Conversation
…2627 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: alien2003 <alien2003@protonmail.ch>
bc7dd6b to
8e50693
Compare
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: alien2003 <alien2003@protonmail.ch>
|
I'm not sure we should include workflowSpec in the merged result, I think it should be banned from the cronWorkflowDefaults. What do you think @isubasinghe? My reasoning is that we've then got two ways to set workflow defaults for crons, and that's confusing and error prone. I don't want to support fixing that when it's surprising. |
Joibel
left a comment
Joibel
left a comment
There was a problem hiding this comment.
Code review
Re-posting as inline comments. Nice, well-scoped feature mirroring workflowDefaults. I verified several things are deliberately correct and not issues: the strategic-merge direction (CronWorkflow wins), the DeepCopy() for concurrency safety, no informer-cache corruption (both call sites build fresh objects from unstructured), the LabelsFrom deref is safely guarded just like MergeTo, and new(int64(60)) in tests is valid Go 1.26. No CLAUDE.md compliance issues.
14 findings inline below (confidence in parentheses; ~70+ are worth acting on, the rest are nits).
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
| wfClientset versioned.Interface | ||
| wfClient typed.WorkflowInterface | ||
| wfDefaults *v1alpha1.Workflow | ||
| cronWfDefaults *v1alpha1.CronWorkflowSpec |
There was a problem hiding this comment.
Dead struct field (confidence 80). cronWfDefaults is stored here and assigned in newCronWfOperationCtx (line 87), but never read again — defaults are applied eagerly in place by applyCronWorkflowDefaults. Contrast wfDefaults, which is read later (at submit/validate). The constructor only needs the parameter; this field can be dropped.
| // If the target defines a field, that value takes precedence over the patch. This mirrors MergeTo, | ||
| // including the manual handling of the embedded WorkflowSpec's hooks and labelsFrom, which strategic | ||
| // merge does not handle. | ||
| func MergeToCronWorkflowSpec(patch, target *wfv1.CronWorkflowSpec) error { |
There was a problem hiding this comment.
A non-zero default can't be overridden back to its zero value (confidence 75). The CronWorkflow is applied as the strategic-merge patch over the defaults, so omitempty value-typed fields can't be reset. Sharpest case: CronWorkflowSpec.Suspend is a plain bool, so cronWorkflowDefaults: {suspend: true} would force-suspend every CronWorkflow with no opt-out. The Workflow path explicitly compensates in JoinWorkflowSpec (and uses *bool); there's no equivalent here. Also affects when/timezone.
| if err := util.MergeToCronWorkflowSpec(cronWfDefaults.DeepCopy(), &cronWf.Spec); err != nil { | ||
| logging.RequireLoggerFromContext(ctx).WithError(err).Error(ctx, "Failed to apply CronWorkflow defaults") | ||
| } |
There was a problem hiding this comment.
Merge error is swallowed, and the merge can wipe the spec (confidence 75). On error this only logs and continues. Worse, MergeToCronWorkflowSpec does *target = wfv1.CronWorkflowSpec{} before its final unmarshal (merge.go:231), so an unmarshal failure leaves cronWf.Spec zeroed and reconciliation proceeds on an empty spec. The analogous setWorkflowDefaults/MergeTo returns the error to the caller instead. Rare in practice, but weaker than the pattern it copies.
| If a `CronWorkflow` sets a value that also has a default, the `CronWorkflow`'s value takes precedence. | ||
| See the [Field Reference](./fields.md#cronworkflowspec) for the full list of `CronWorkflowSpec` fields. | ||
|
|
||
| Defaults are applied to the controller's in-memory copy during reconciliation and are never written back to the `CronWorkflow`, so they do not conflict with GitOps tools that manage the resource. |
There was a problem hiding this comment.
This "never written back" claim isn't strictly true for timezone (confidence 72). A defaulted Spec.Timezone flows into SetSchedule(...GetScheduleWithTimezoneString()) and the resulting annotationKeyLatestSchedule annotation, which persistUpdate patches into metadata.annotations. So a defaulted value does reach the stored object (a controller-managed annotation, not the spec). Worth qualifying the claim.
| defer runtimeutil.HandleCrashWithContext(ctx, runtimeutil.PanicHandlers...) | ||
|
|
||
| cronController := cron.NewCronController(ctx, wfc.wfclientset, wfc.dynamicInterface, wfc.namespace, wfc.GetManagedNamespace(), wfc.Config.InstanceID, wfc.metrics, wfc.eventRecorderManager, cronWorkflowWorkers, wfc.wftmplInformer, wfc.cwftmplInformer, wfc.Config.WorkflowDefaults) | ||
| cronController := cron.NewCronController(ctx, wfc.wfclientset, wfc.dynamicInterface, wfc.namespace, wfc.GetManagedNamespace(), wfc.Config.InstanceID, wfc.metrics, wfc.eventRecorderManager, cronWorkflowWorkers, wfc.wftmplInformer, wfc.cwftmplInformer, wfc.Config.WorkflowDefaults, wfc.Config.CronWorkflowDefaults) |
There was a problem hiding this comment.
Defaults are controller-only; not applied on the API-server / argo cron lint path (confidence 72). workflowDefaults is also wired into validate/submit on the server, but cronWorkflowDefaults is reconcile-only — so a CronWorkflow that only validates because of a default passes at the controller yet can't be linted with defaults via the API. Likely deliberate (GitOps-drift avoidance), but an undocumented behavioral asymmetry worth calling out.
| func TestApplyCronWorkflowDefaults(t *testing.T) { | ||
| ctx := logging.TestContext(t.Context()) | ||
|
|
||
| newCronWf := func() *v1alpha1.CronWorkflow { | ||
| cronWf := &v1alpha1.CronWorkflow{} | ||
| v1alpha1.MustUnmarshal([]byte(scheduledWf), cronWf) | ||
| return cronWf | ||
| } |
There was a problem hiding this comment.
The DeepCopy safety property is untested at this layer (confidence 60). applyCronWorkflowDefaults deep-copies precisely because the shared cronWfDefaults is mutated transiently during the merge. No case here asserts that the shared defaults object is left unmutated (especially when it contains Hooks/LabelsFrom). A future refactor dropping the DeepCopy() wouldn't be caught.
| For example, to keep at most four successful and four failed child Workflows for every `CronWorkflow`: | ||
|
|
||
| ```yaml | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: workflow-controller-configmap | ||
| data: | ||
| cronWorkflowDefaults: | | ||
| concurrencyPolicy: "Replace" | ||
| successfulJobsHistoryLimit: 4 | ||
| failedJobsHistoryLimit: 4 | ||
| timezone: "America/New_York" |
There was a problem hiding this comment.
The example sets more than the prose explains (confidence 70). The lead-in says only "to keep at most four successful and four failed child Workflows", but the YAML also sets concurrencyPolicy: "Replace" and timezone: "America/New_York". Either trim the example to match, or broaden the lead-in.
| WorkflowDefaults *wfv1.Workflow `json:"workflowDefaults,omitempty"` | ||
|
|
||
| // CronWorkflowDefaults are values that will apply to all CronWorkflows from this controller, unless overridden on the CronWorkflow-level | ||
| CronWorkflowDefaults *wfv1.CronWorkflowSpec `json:"cronWorkflowDefaults,omitempty"` |
There was a problem hiding this comment.
Undocumented two-layer defaulting (confidence 55). Because the type is CronWorkflowSpec (which embeds WorkflowSpec), cronWorkflowDefaults can also set arbitrary workflow-spec fields (entrypoint, templates, etc.), which then re-merge with workflowDefaults on the spawned Workflow. The docs only advertise cron-level fields and don't explain this overlap or its precedence.
There was a problem hiding this comment.
I've made a separate comment - but my feeling is we must disallow this.
| // Temporarily remove hooks and labelsFrom as they don't merge | ||
| patchHooks := patch.WorkflowSpec.Hooks | ||
| patch.WorkflowSpec.Hooks = nil | ||
| var patchLabelsFrom map[string]wfv1.LabelValueFrom | ||
| if patch.WorkflowSpec.WorkflowMetadata != nil { | ||
| patchLabelsFrom = patch.WorkflowSpec.WorkflowMetadata.LabelsFrom | ||
| patch.WorkflowSpec.WorkflowMetadata.LabelsFrom = nil | ||
| } | ||
|
|
||
| patchBytes, err := json.Marshal(patch) | ||
| patch.WorkflowSpec.Hooks = patchHooks | ||
| if len(patchLabelsFrom) != 0 { | ||
| patch.WorkflowSpec.WorkflowMetadata.LabelsFrom = patchLabelsFrom | ||
| } |
There was a problem hiding this comment.
DRY: this is a near-verbatim copy of MergeTo (confidence 50). The subtle hooks/labelsFrom "remove-before-marshal, restore-after, re-add-to-target" dance is duplicated; a fix to one copy won't propagate to the other, and nothing enforces parity. Consider a single generic helper parameterized over the embedded WorkflowSpec. Maintainability only.
| > v4.1 and after | ||
|
|
||
| Default `CronWorkflow` spec values can be specified under the `cronWorkflowDefaults` key in the `workflow-controller-configmap`. | ||
| These apply to all `CronWorkflows` executed from the controller, such as `concurrencyPolicy`, `startingDeadlineSeconds`, `successfulJobsHistoryLimit`, `failedJobsHistoryLimit` and `timezone`. |
There was a problem hiding this comment.
Dangling "such as" clause (confidence 50). "These apply to all CronWorkflows executed from the controller, such as concurrencyPolicy, ..." reads as if the CronWorkflows are "such as concurrencyPolicy". It should attach to the default fields, e.g. "Default values can be set for fields such as ...".
2 participants
Fixes #12627
Motivation
workflowDefaultslets an operator set controller-wide defaults forWorkflows, but there is no equivalent forCronWorkflows. Fields likesuccessfulJobsHistoryLimit,failedJobsHistoryLimit,concurrencyPolicyandtimezonehave to be repeated on everyCronWorkflow, or enforced with an external policy engine. This adds acronWorkflowDefaultsto set them once.Modifications
Add a
cronWorkflowDefaultskey to theworkflow-controller-configmap(config.Config.CronWorkflowDefaults *wfv1.CronWorkflowSpec).The defaults are merged into each
CronWorkflowduring reconciliation by a newutil.MergeToCronWorkflowSpec, which mirrorsutil.MergeTo: a strategic merge that lets theCronWorkflow's own values win over the defaults. The merge runs on the controller's in-memory copy innewCronWfOperationCtx, before validation, scheduling and runtime-policy checks, so every read sees the defaulted spec.The defaults are not written back to the resource. The cron reconcile loop only persists
statusandmetadata, neverspec, so this keeps defaults out of the stored object and avoids fighting GitOps controllers that own theCronWorkflowmanifest (the concern @anarsen raised on the issue). The shared defaults are deep-copied per reconcile because the merge temporarily mutates its patch argument and workers run concurrently.Verification
go test ./config/... ./workflow/util/... ./workflow/cron/...TestMergeToCronWorkflowSpec: default fills unset field, target overrides default, nil patch is a no-op, the shared patch is not mutated.TestApplyCronWorkflowDefaults: defaults fill unset fields on the in-memoryCronWorkflow, an explicitCronWorkflowvalue is preserved, and theCronWorkflow's ownschedulessurvive the merge.Documentation
docs/default-workflow-specs.mdgets a "Setting Default CronWorkflow Values" section; the exampledocs/workflow-controller-configmap.yamlgets the new key;docs/workflow-controller-configmap.mdis regenerated. Feature file added under.features/pending/.AI
AI-assisted (Claude Code). I scoped the design (in-memory application vs writing back to the resource, and the config type), reviewed and tested the implementation, and ran the builds, tests and linter in a pinned container. The decision to apply defaults in-memory rather than mutate the resource follows the GitOps concern raised by @anarsen on the issue.