fix(controller): check storedTemplates in GetNodeTemplate before live CWT lookup - #15661
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a controller bug where assessNodeStatus could incorrectly mark templateRef nodes as Error (“template not found”) when the referenced ClusterWorkflowTemplate was changed after pod creation, by preferring wf.Status.StoredTemplates over the live CWT informer cache during node template lookup.
Changes:
- Update
wfOperationCtx.GetNodeTemplateto checkwf.Status.StoredTemplatesbefore falling back to a live CWT lookup fortemplateRefnodes. - Add a unit test that reproduces the bug scenario by ensuring the live CWT cache is empty while the template exists in
StoredTemplates. - Update quick-start docs to use server-side apply for the minimal install manifest.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
workflow/controller/operator.go |
Changes GetNodeTemplate to prefer StoredTemplates for templateRef nodes, avoiding spurious “template not found” errors after live CWT edits. |
workflow/controller/operator_test.go |
Adds regression coverage ensuring stored templates are used even when live CWT informer cache doesn’t have the referenced template. |
docs/quick-start.md |
Switches the quick-start install command to kubectl apply --server-side. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const storedKey = "cluster/cwt-helper/say-hello" | ||
|
|
||
| wf := &wfv1.Workflow{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "cwt-templateref-bug-test", | ||
| Namespace: "default", | ||
| }, | ||
| Spec: wfv1.WorkflowSpec{ | ||
| Entrypoint: "main", | ||
| }, | ||
| Status: wfv1.WorkflowStatus{ | ||
| StoredTemplates: map[string]wfv1.Template{ | ||
| storedKey: storedTmpl, | ||
| }, |
There was a problem hiding this comment.
The test hard-codes the storedTemplates map key ("cluster/cwt-helper/say-hello"), which couples the test to the internal key format used by resolveTemplateReference. To make the test resilient to future key format changes, populate StoredTemplates via wf.SetStoredTemplate(...) (using the node as the TemplateReferenceHolder) instead of constructing the map entry by hand.
|
/cc @Joibel |
… CWT lookup Signed-off-by: Nancy <9d.24.nancy.sangani@gmail.com>
eeeaa9f to
90ff6d7
Compare
|
@thevilledev Please review it when you get a chance, I have implemented the suggested changes. Thanks! |
thevilledev
left a comment
thevilledev
left a comment
There was a problem hiding this comment.
Thank you, LGTM. Pending for maintainer feedback
|
What is the status of this please?
From my POV This bug fundamentally breaks the immutability guarantee that
/cc @Joibel |
|
👋 Hello, any update on this issue? We're experiencing the same issue @deynekas reported. It would help us tremendously with making changes to any |
|
#15659 is the same fix, but with better tests. Closing this, preferring that one. |
Fixes #15655
When a cross-CWT templateRef node's template is removed from the CWT
after the pod is already running, assessNodeStatus was spuriously marking
the node Error with "template not found" — even though the pod completed
successfully.
The fix makes GetNodeTemplate check wf.Status.StoredTemplates first
(mirroring what resolveTemplateImpl already does during execution) before
falling back to the live CWT informer cache. The fallback is kept for
edge cases like controller restarts.
Added a unit test that reproduces the bug by leaving the live CWT cache
empty while the template exists in storedTemplates.