Skip to content

Commit

Permalink
fix: use correct node name as args to PodName. Fixes argoproj#9906 (a…
Browse files Browse the repository at this point in the history 
…rgoproj#9995)
  • Loading branch information
@isubasinghe @mweibel
isubasinghe authored and mweibel committed Nov 24, 2022
1 parent eddb1b7 commit 9e0ac23
Show file tree
Hide file tree
Showing 10 changed files with 37 additions and 45 deletions.
4 changes: 3 additions & 1 deletion cmd/argo/commands/common/get.go
View file
Expand Up @@ -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, ""}
Expand Down
56 changes: 23 additions & 33 deletions cmd/argo/commands/common/get_test.go
View file
Expand Up @@ -3,7 +3,6 @@ package common
import (
"bytes"
"fmt"
"hash/fnv"
"testing"
"text/tabwriter"
"time"
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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"
Expand Down Expand Up @@ -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))
})
}

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/fixtures/then.go
View file
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion workflow/controller/exec_control.go
View file
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion workflow/controller/operator.go
View file
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion workflow/controller/workflowpod.go
View file
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions workflow/util/pod_name.go
View file
Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions workflow/util/pod_name_v1_test.go
View file
Expand Up @@ -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
Expand All @@ -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)

}
4 changes: 2 additions & 2 deletions workflow/util/pod_name_v2_test.go
View file
Expand Up @@ -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)

///////////////////////////////////////////////////////////////////////////////////////////
Expand All @@ -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)

}
2 changes: 1 addition & 1 deletion workflow/util/util.go
View file
Expand Up @@ -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)
Expand Down

0 comments on commit 9e0ac23

Please sign in to comment.