From 9e0ac239374f0c6eadeb6bf8de02ca1fe534f558 Mon Sep 17 00:00:00 2001 From: Isitha Subasinghe Date: Thu, 10 Nov 2022 13:14:06 +1100 Subject: [PATCH] fix: use correct node name as args to PodName. Fixes #9906 (#9995) --- cmd/argo/commands/common/get.go | 4 +- cmd/argo/commands/common/get_test.go | 56 ++++++++++++---------------- test/e2e/fixtures/then.go | 2 +- workflow/controller/exec_control.go | 2 +- workflow/controller/operator.go | 2 +- workflow/controller/workflowpod.go | 2 +- workflow/util/pod_name.go | 4 +- workflow/util/pod_name_v1_test.go | 4 +- workflow/util/pod_name_v2_test.go | 4 +- workflow/util/util.go | 2 +- 10 files changed, 37 insertions(+), 45 deletions(-) diff --git a/cmd/argo/commands/common/get.go b/cmd/argo/commands/common/get.go index 47405f0014b2..c1fffe00a1d5 100644 --- a/cmd/argo/commands/common/get.go +++ b/cmd/argo/commands/common/get.go @@ -479,7 +479,9 @@ func printNode(w *tabwriter.Writer, node wfv1.NodeStatus, wfName, nodePrefix str var args []interface{} duration := humanize.RelativeDurationShort(node.StartedAt.Time, node.FinishedAt.Time) if node.Type == wfv1.NodeTypePod { - podName := util.PodName(wfName, nodeName, templateName, node.ID, podNameVersion) + // node.Name is used here because nodeName may contain additionally formatting. + // We want to use the original naming to ensure the correct hash is dervied + podName := util.GeneratePodName(wfName, node.Name, templateName, node.ID, podNameVersion) args = []interface{}{nodePrefix, nodeName, templateName, podName, duration, node.Message, ""} } else { args = []interface{}{nodePrefix, nodeName, templateName, "", "", node.Message, ""} diff --git a/cmd/argo/commands/common/get_test.go b/cmd/argo/commands/common/get_test.go index 42652d5bec98..135e1bc8ef9b 100644 --- a/cmd/argo/commands/common/get_test.go +++ b/cmd/argo/commands/common/get_test.go @@ -3,7 +3,6 @@ package common import ( "bytes" "fmt" - "hash/fnv" "testing" "text/tabwriter" "time" @@ -16,9 +15,7 @@ import ( "github.com/argoproj/argo-workflows/v3/workflow/util" ) -var ( - workflowName string = "testWF" -) +var workflowName string = "testWF" func init() { // these values get used as part of determining node name and would normally be set as part of @@ -63,33 +60,32 @@ func TestPrintNode(t *testing.T) { Time: time.Now(), } node := wfv1.NodeStatus{ - Name: nodeName, - Phase: wfv1.NodeRunning, - DisplayName: nodeName, - Type: wfv1.NodeTypePod, - ID: nodeID, - StartedAt: timestamp, - FinishedAt: timestamp, - Message: nodeMessage, + Name: nodeName, + Phase: wfv1.NodeRunning, + DisplayName: nodeName, + Type: wfv1.NodeTypePod, + ID: nodeID, + StartedAt: timestamp, + FinishedAt: timestamp, + Message: nodeMessage, + TemplateName: nodeTemplateName, } - node.HostNodeName = kubernetesNodeName // derive expected pod name: - h := fnv.New32a() - _, _ = h.Write([]byte(fmt.Sprintf("%s %s", JobStatusIconMap[wfv1.NodeRunning], nodeName))) - expectedPodName := fmt.Sprintf("%s-%s-%v", workflowName, node.TemplateName, h.Sum32()) - testPrintNodeImpl(t, fmt.Sprintf("%s %s\t%s\t%s\t%s\t%s\t%s\n", JobStatusIconMap[wfv1.NodeRunning], nodeName, "", expectedPodName, "0s", nodeMessage, ""), node, getArgs) + expectedPodName := util.GeneratePodName(workflowName, nodeName, nodeTemplateName, nodeID, util.GetPodNameVersion()) + t.Log(expectedPodName) + testPrintNodeImpl(t, fmt.Sprintf("%s %s\t%s\t%s\t%s\t%s\t%s\n", JobStatusIconMap[wfv1.NodeRunning], nodeName, nodeTemplateName, expectedPodName, "0s", nodeMessage, ""), node, getArgs) // Compatibility test getArgs.Status = "Running" - testPrintNodeImpl(t, fmt.Sprintf("%s %s\t\t%s\t%s\t%s\t\n", JobStatusIconMap[wfv1.NodeRunning], nodeName, expectedPodName, "0s", nodeMessage), node, getArgs) + testPrintNodeImpl(t, fmt.Sprintf("%s %s\t%s\t%s\t%s\t%s\t%s\n", JobStatusIconMap[wfv1.NodeRunning], nodeName, nodeTemplateName, expectedPodName, "0s", nodeMessage, ""), node, getArgs) getArgs.Status = "" getArgs.NodeFieldSelectorString = "phase=Running" - testPrintNodeImpl(t, fmt.Sprintf("%s %s\t\t%s\t%s\t%s\t\n", JobStatusIconMap[wfv1.NodeRunning], nodeName, expectedPodName, "0s", nodeMessage), node, getArgs) + testPrintNodeImpl(t, fmt.Sprintf("%s %s\t%s\t%s\t%s\t%s\t%s\n", JobStatusIconMap[wfv1.NodeRunning], nodeName, nodeTemplateName, expectedPodName, "0s", nodeMessage, ""), node, getArgs) getArgs.NodeFieldSelectorString = "phase!=foobar" - testPrintNodeImpl(t, fmt.Sprintf("%s %s\t\t%s\t%s\t%s\t\n", JobStatusIconMap[wfv1.NodeRunning], nodeName, expectedPodName, "0s", nodeMessage), node, getArgs) + testPrintNodeImpl(t, fmt.Sprintf("%s %s\t%s\t%s\t%s\t%s\t%s\n", JobStatusIconMap[wfv1.NodeRunning], nodeName, nodeTemplateName, expectedPodName, "0s", nodeMessage, ""), node, getArgs) getArgs.NodeFieldSelectorString = "phase!=Running" testPrintNodeImpl(t, "", node, getArgs) @@ -108,7 +104,7 @@ func TestPrintNode(t *testing.T) { } node.TemplateName = nodeTemplateName - expectedPodName = fmt.Sprintf("%s-%s-%v", workflowName, node.TemplateName, h.Sum32()) + expectedPodName = util.GeneratePodName(workflowName, nodeName, nodeTemplateName, nodeID, util.GetPodNameVersion()) testPrintNodeImpl(t, fmt.Sprintf("%s %s\t%s\t%s\t%s\t%s\t%s\n", JobStatusIconMap[wfv1.NodeRunning], nodeName, nodeTemplateName, expectedPodName, "0s", nodeMessage, ""), node, getArgs) node.Type = wfv1.NodeTypeSuspend @@ -119,7 +115,7 @@ func TestPrintNode(t *testing.T) { Template: nodeTemplateRefName, } templateName := fmt.Sprintf("%s/%s", node.TemplateRef.Name, node.TemplateRef.Template) - expectedPodName = fmt.Sprintf("%s-%s-%v", workflowName, templateName, h.Sum32()) + expectedPodName = util.GeneratePodName(workflowName, nodeName, templateName, nodeID, util.GetPodNameVersion()) testPrintNodeImpl(t, fmt.Sprintf("%s %s\t%s/%s\t%s\t%s\t%s\t%s\n", NodeTypeIconMap[wfv1.NodeTypeSuspend], nodeName, nodeTemplateRefName, nodeTemplateRefName, "", "", nodeMessage, ""), node, getArgs) getArgs.Output = "wide" @@ -397,20 +393,14 @@ status: output := PrintWorkflowHelper(&wf, GetFlags{}) // derive expected pod name: - h := fnv.New32a() - _, _ = h.Write([]byte(fmt.Sprintf("%s %s", JobStatusIconMap[wfv1.NodeSucceeded], "sleep(9:nine)"))) - expectedPodName := fmt.Sprintf("many-items-z26lj-sleep-%v", h.Sum32()) - assert.Contains(t, output, fmt.Sprintf("sleep(9:nine) sleep %s 19s", expectedPodName)) + expectedPodName := util.GeneratePodName(wf.GetObjectMeta().GetName(), "many-items-z26lj[0].sleep(9:nine)", "sleep", "many-items-z26lj-2619926859", util.GetPodNameVersion()) + assert.Contains(t, output, fmt.Sprintf("sleep(9:nine) sleep %s 19s", expectedPodName)) - h.Reset() - _, _ = h.Write([]byte(fmt.Sprintf("%s %s", JobStatusIconMap[wfv1.NodeSucceeded], "sleep(10:ten)"))) - expectedPodName = fmt.Sprintf("many-items-z26lj-sleep-%v", h.Sum32()) + expectedPodName = util.GeneratePodName(wf.GetObjectMeta().GetName(), "many-items-z26lj[0].sleep(10:ten)", "sleep", "many-items-z26lj-1052882686", util.GetPodNameVersion()) assert.Contains(t, output, fmt.Sprintf("sleep(10:ten) sleep %s 23s", expectedPodName)) - h.Reset() - _, _ = h.Write([]byte(fmt.Sprintf("%s %s", JobStatusIconMap[wfv1.NodeSucceeded], "sleep(11:eleven)"))) - expectedPodName = fmt.Sprintf("many-items-z26lj-sleep-%v", h.Sum32()) - assert.Contains(t, output, fmt.Sprintf("sleep(11:eleven) sleep %s 22s", expectedPodName)) + expectedPodName = util.GeneratePodName(wf.GetObjectMeta().GetName(), "many-items-z26lj[0].sleep(11:eleven)", "sleep", "many-items-z26lj-3011405271", util.GetPodNameVersion()) + assert.Contains(t, output, fmt.Sprintf("sleep(11:eleven) sleep %s 22s", expectedPodName)) }) } diff --git a/test/e2e/fixtures/then.go b/test/e2e/fixtures/then.go index f52bdfb19b4f..eb2898a6dbe0 100644 --- a/test/e2e/fixtures/then.go +++ b/test/e2e/fixtures/then.go @@ -92,7 +92,7 @@ func (t *Then) ExpectWorkflowNode(selector func(status wfv1.NodeStatus) bool, f ObjectMeta: *metadata, } version := util.GetWorkflowPodNameVersion(wf) - podName := util.PodName(t.wf.Name, n.Name, n.TemplateName, n.ID, version) + podName := util.GeneratePodName(t.wf.Name, n.Name, n.TemplateName, n.ID, version) var err error ctx := context.Background() diff --git a/workflow/controller/exec_control.go b/workflow/controller/exec_control.go index fa9d9b552708..fc77e1087f9d 100644 --- a/workflow/controller/exec_control.go +++ b/workflow/controller/exec_control.go @@ -100,7 +100,7 @@ func (woc *wfOperationCtx) killDaemonedChildren(nodeID string) { if !childNode.IsDaemoned() { continue } - podName := util.PodName(woc.wf.Name, childNode.Name, childNode.TemplateName, childNode.ID, util.GetWorkflowPodNameVersion(woc.wf)) + podName := util.GeneratePodName(woc.wf.Name, childNode.Name, childNode.TemplateName, childNode.ID, util.GetWorkflowPodNameVersion(woc.wf)) woc.controller.queuePodForCleanup(woc.wf.Namespace, podName, terminateContainers) childNode.Phase = wfv1.NodeSucceeded childNode.Daemoned = nil diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index 933b9db9f433..b393b8baa057 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -3736,7 +3736,7 @@ func (woc *wfOperationCtx) substituteGlobalVariables(params common.Parameters) e // POD_NAMES environment variable func (woc *wfOperationCtx) getPodName(nodeName, templateName string) string { version := wfutil.GetWorkflowPodNameVersion(woc.wf) - return wfutil.PodName(woc.wf.Name, nodeName, templateName, woc.wf.NodeID(nodeName), version) + return wfutil.GeneratePodName(woc.wf.Name, nodeName, templateName, woc.wf.NodeID(nodeName), version) } func (woc *wfOperationCtx) getServiceAccountTokenName(ctx context.Context, name string) (string, error) { diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index fed3b697cffb..1f53a95d72cc 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -159,7 +159,7 @@ func (woc *wfOperationCtx) createWorkflowPod(ctx context.Context, nodeName strin pod := &apiv1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: util.PodName(woc.wf.Name, nodeName, tmpl.Name, nodeID, util.GetWorkflowPodNameVersion(woc.wf)), + Name: util.GeneratePodName(woc.wf.Name, nodeName, tmpl.Name, nodeID, util.GetWorkflowPodNameVersion(woc.wf)), Namespace: woc.wf.ObjectMeta.Namespace, Labels: map[string]string{ common.LabelKeyWorkflow: woc.wf.ObjectMeta.Name, // Allows filtering by pods related to specific workflow diff --git a/workflow/util/pod_name.go b/workflow/util/pod_name.go index 2fcca6647c6a..d7dad3b3b09e 100644 --- a/workflow/util/pod_name.go +++ b/workflow/util/pod_name.go @@ -45,8 +45,8 @@ func GetPodNameVersion() PodNameVersion { } } -// PodName return a deterministic pod name -func PodName(workflowName, nodeName, templateName, nodeID string, version PodNameVersion) string { +// GeneratePodName return a deterministic pod name +func GeneratePodName(workflowName, nodeName, templateName, nodeID string, version PodNameVersion) string { if version == PodNameV1 { return nodeID } diff --git a/workflow/util/pod_name_v1_test.go b/workflow/util/pod_name_v1_test.go index 38786f0554a4..154573dee482 100644 --- a/workflow/util/pod_name_v1_test.go +++ b/workflow/util/pod_name_v1_test.go @@ -19,7 +19,7 @@ func TestPodNameV1(t *testing.T) { actual := ensurePodNamePrefixLength(expected) assert.Equal(t, expected, actual) - name := PodName(shortWfName, nodeName, shortTemplateName, nodeID, PodNameV1) + name := GeneratePodName(shortWfName, nodeName, shortTemplateName, nodeID, PodNameV1) assert.Equal(t, nodeID, name) // long case @@ -34,7 +34,7 @@ func TestPodNameV1(t *testing.T) { assert.Equal(t, maxK8sResourceNameLength-k8sNamingHashLength-1, len(actual)) - name = PodName(longWfName, nodeName, longTemplateName, nodeID, PodNameV1) + name = GeneratePodName(longWfName, nodeName, longTemplateName, nodeID, PodNameV1) assert.Equal(t, nodeID, name) } diff --git a/workflow/util/pod_name_v2_test.go b/workflow/util/pod_name_v2_test.go index 2744e36b38b9..ca450a20c229 100644 --- a/workflow/util/pod_name_v2_test.go +++ b/workflow/util/pod_name_v2_test.go @@ -27,7 +27,7 @@ func TestPodNameV2(t *testing.T) { _, _ = h.Write([]byte(nodeName)) expectedPodName := fmt.Sprintf("wfname-templatename-%v", h.Sum32()) - name := PodName(shortWfName, nodeName, shortTemplateName, nodeID, PodNameV2) + name := GeneratePodName(shortWfName, nodeName, shortTemplateName, nodeID, PodNameV2) assert.Equal(t, expectedPodName, name) /////////////////////////////////////////////////////////////////////////////////////////// @@ -47,7 +47,7 @@ func TestPodNameV2(t *testing.T) { longPrefix := fmt.Sprintf("%s-%s", longWfName, longTemplateName) expectedPodName = fmt.Sprintf("%s-%v", longPrefix[0:maxK8sResourceNameLength-k8sNamingHashLength-1], h.Sum32()) - name = PodName(longWfName, nodeName, longTemplateName, nodeID, PodNameV2) + name = GeneratePodName(longWfName, nodeName, longTemplateName, nodeID, PodNameV2) assert.Equal(t, expectedPodName, name) } diff --git a/workflow/util/util.go b/workflow/util/util.go index 739b786136b7..d4e29784aee6 100644 --- a/workflow/util/util.go +++ b/workflow/util/util.go @@ -757,7 +757,7 @@ func getDescendantNodeIDs(wf *wfv1.Workflow, node wfv1.NodeStatus) []string { func deletePodNodeDuringRetryWorkflow(wf *wfv1.Workflow, node wfv1.NodeStatus, deletedPods map[string]bool, podsToDelete []string) (map[string]bool, []string) { templateName := GetTemplateFromNode(node) version := GetWorkflowPodNameVersion(wf) - podName := PodName(wf.Name, node.Name, templateName, node.ID, version) + podName := GeneratePodName(wf.Name, node.Name, templateName, node.ID, version) if _, ok := deletedPods[podName]; !ok { deletedPods[podName] = true podsToDelete = append(podsToDelete, podName)