fix: correct finding the closest ancestor retry node. Fixes #14517 (#14576)

Signed-off-by: oninowang <oninowang@tencent.com>
Co-authored-by: oninowang <oninowang@tencent.com>

Author: jswxstw

pkg/apis/workflow/v1alpha1/workflow_types.go

@@ -1771,20 +1771,24 @@ var _ ArgumentsProvider = &Arguments{}
 
 type Nodes map[string]NodeStatus
 
-func (in Nodes) FindByDisplayName(name string) *NodeStatus {
-	return in.Find(NodeWithDisplayName(name))
+func (n Nodes) FindByDisplayName(name string) *NodeStatus {
+	return n.Find(NodeWithDisplayName(name))
 }
 
-func (in Nodes) FindByName(name string) *NodeStatus {
-	return in.Find(NodeWithName(name))
+func (n Nodes) FindByName(name string) *NodeStatus {
+	return n.Find(NodeWithName(name))
 }
 
-func (in Nodes) Any(f func(NodeStatus) bool) bool {
-	return in.Find(f) != nil
+func (n Nodes) FindByChild(childID string) *NodeStatus {
+	return n.Find(NodeWithChild(childID))
 }
 
-func (in Nodes) Find(f func(NodeStatus) bool) *NodeStatus {
-	for _, i := range in {
+func (n Nodes) Any(f func(NodeStatus) bool) bool {
+	return n.Find(f) != nil
+}
+
+func (n Nodes) Find(f func(NodeStatus) bool) *NodeStatus {
+	for _, i := range n {
 		if f(i) {
 			return &i
 		}
@@ -1794,57 +1798,57 @@ func (in Nodes) Find(f func(NodeStatus) bool) *NodeStatus {
 
 // Get a NodeStatus from the hashmap of Nodes.
 // Return a nil along with an error if non existent.
-func (in Nodes) Get(key string) (*NodeStatus, error) {
-	val, ok := in[key]
+func (n Nodes) Get(key string) (*NodeStatus, error) {
+	val, ok := n[key]
 	if !ok {
 		return nil, fmt.Errorf("key was not found for %s", key)
 	}
 	return &val, nil
 }
 
 // Check if the Nodes map has a key entry
-func (in Nodes) Has(key string) bool {
-	_, err := in.Get(key)
+func (n Nodes) Has(key string) bool {
+	_, err := n.Get(key)
 	return err == nil
 }
 
 // Get the Phase of a Node
-func (in Nodes) GetPhase(key string) (*NodePhase, error) {
-	val, err := in.Get(key)
+func (n Nodes) GetPhase(key string) (*NodePhase, error) {
+	val, err := n.Get(key)
 	if err != nil {
 		return nil, err
 	}
 	return &val.Phase, nil
 }
 
 // Set the status of a node by key
-func (in Nodes) Set(key string, status NodeStatus) {
+func (n Nodes) Set(key string, status NodeStatus) {
 	if status.Name == "" {
 		log.Warnf("Name was not set for key %s", key)
 	}
 	if status.ID == "" {
 		log.Warnf("ID was not set for key %s", key)
 	}
-	_, ok := in[key]
+	_, ok := n[key]
 	if ok {
 		log.Tracef("Changing NodeStatus for %s to %+v", key, status)
 	}
-	in[key] = status
+	n[key] = status
 }
 
 // Delete a node from the Nodes by key
-func (in Nodes) Delete(key string) {
-	has := in.Has(key)
+func (n Nodes) Delete(key string) {
+	has := n.Has(key)
 	if !has {
 		log.Warnf("Trying to delete non existent key %s", key)
 		return
 	}
-	delete(in, key)
+	delete(n, key)
 }
 
 // Get the name of a node by key
-func (in Nodes) GetName(key string) (string, error) {
-	val, err := in.Get(key)
+func (n Nodes) GetName(key string) (string, error) {
+	val, err := n.Get(key)
 	if err != nil {
 		return "", err
 	}
@@ -1858,6 +1862,12 @@ func NodeWithDisplayName(name string) func(n NodeStatus) bool {
 	return func(n NodeStatus) bool { return n.DisplayName == name }
 }
 
+func NodeWithChild(childID string) func(n NodeStatus) bool {
+	return func(n NodeStatus) bool {
+		return n.HasChild(childID)
+	}
+}
+
 func FailedPodNode(n NodeStatus) bool {
 	return n.Type == NodeTypePod && n.Phase == NodeFailed
 }
@@ -1867,14 +1877,14 @@ func SucceededPodNode(n NodeStatus) bool {
 }
 
 // Children returns the children of the parent.
-func (in Nodes) Children(parentNodeID string) Nodes {
+func (n Nodes) Children(parentNodeID string) Nodes {
 	childNodes := make(Nodes)
-	parentNode, ok := in[parentNodeID]
+	parentNode, ok := n[parentNodeID]
 	if !ok {
 		return childNodes
 	}
 	for _, childID := range parentNode.Children {
-		if childNode, ok := in[childID]; ok {
+		if childNode, ok := n[childID]; ok {
 			childNodes[childID] = childNode
 		}
 	}
@@ -1883,8 +1893,8 @@ func (in Nodes) Children(parentNodeID string) Nodes {
 
 // NestedChildrenStatus takes in a nodeID and returns all its children, this involves a tree search using DFS.
 // This is needed to mark all children nodes as failed for example.
-func (in Nodes) NestedChildrenStatus(parentNodeID string) ([]NodeStatus, error) {
-	parentNode, ok := in[parentNodeID]
+func (n Nodes) NestedChildrenStatus(parentNodeID string) ([]NodeStatus, error) {
+	parentNode, ok := n[parentNodeID]
 	if !ok {
 		return nil, fmt.Errorf("could not find %s in nodes when searching for nested children", parentNodeID)
 	}
@@ -1896,7 +1906,7 @@ func (in Nodes) NestedChildrenStatus(parentNodeID string) ([]NodeStatus, error)
 		childNode := toexplore[0]
 		toexplore = toexplore[1:]
 		for _, nodeID := range childNode.Children {
-			toexplore = append(toexplore, in[nodeID])
+			toexplore = append(toexplore, n[nodeID])
 		}
 
 		if childNode.Name == parentNode.Name {
@@ -1909,9 +1919,9 @@ func (in Nodes) NestedChildrenStatus(parentNodeID string) ([]NodeStatus, error)
 }
 
 // Filter returns the subset of the nodes that match the predicate, e.g. only failed nodes
-func (in Nodes) Filter(predicate func(NodeStatus) bool) Nodes {
+func (n Nodes) Filter(predicate func(NodeStatus) bool) Nodes {
 	filteredNodes := make(Nodes)
-	for _, node := range in {
+	for _, node := range n {
 		if predicate(node) {
 			filteredNodes[node.ID] = node
 		}
@@ -1920,9 +1930,9 @@ func (in Nodes) Filter(predicate func(NodeStatus) bool) Nodes {
 }
 
 // Map maps the nodes to new values, e.g. `x.Hostname`
-func (in Nodes) Map(f func(x NodeStatus) interface{}) map[string]interface{} {
+func (n Nodes) Map(f func(x NodeStatus) interface{}) map[string]interface{} {
 	values := make(map[string]interface{})
-	for _, node := range in {
+	for _, node := range n {
 		values[node.ID] = f(node)
 	}
 	return values

workflow/controller/retry_tweak.go

@@ -11,21 +11,14 @@ import (
 // RetryTweak is a 2nd order function interface for tweaking the retry
 type RetryTweak = func(retryStrategy wfv1.RetryStrategy, nodes wfv1.Nodes, pod *apiv1.Pod)
 
-// FindRetryNode locates the closes retry node ancestor to nodeID
+// FindRetryNode locates the closest retry node ancestor to nodeID
 func FindRetryNode(nodes wfv1.Nodes, nodeID string) *wfv1.NodeStatus {
+	if parentNode := nodes.FindByChild(nodeID); parentNode != nil && parentNode.Type == wfv1.NodeTypeRetry {
+		return parentNode
+	}
 	boundaryID := nodes[nodeID].BoundaryID
-	boundaryNode := nodes[boundaryID]
-	for _, node := range nodes {
-		if node.Type != wfv1.NodeTypeRetry {
-			continue
-		}
-		if boundaryID == "" && node.HasChild(nodeID) {
-			return &node
-		} else if boundaryNode.TemplateName != "" && node.TemplateName == boundaryNode.TemplateName {
-			return &node
-		} else if boundaryNode.TemplateRef != nil && node.TemplateRef != nil && node.TemplateRef.Name == boundaryNode.TemplateRef.Name && node.TemplateRef.Template == boundaryNode.TemplateRef.Template {
-			return &node
-		}
+	if parentNode := nodes.FindByChild(boundaryID); parentNode != nil && parentNode.Type == wfv1.NodeTypeRetry {
+		return parentNode
 	}
 	return nil
 }
@@ -36,10 +29,12 @@ func RetryOnDifferentHost(retryNodeName string) RetryTweak {
 		if retryStrategy.Affinity == nil {
 			return
 		}
-		hostNames := wfretry.GetFailHosts(nodes, retryNodeName)
-		hostLabel := env.GetString("RETRY_HOST_NAME_LABEL_KEY", "kubernetes.io/hostname")
-		if hostLabel != "" && len(hostNames) > 0 {
-			pod.Spec.Affinity = wfretry.AddHostnamesToAffinity(hostLabel, hostNames, pod.Spec.Affinity)
+		if retryStrategy.Affinity.NodeAntiAffinity != nil {
+			hostNames := wfretry.GetFailHosts(nodes, retryNodeName)
+			hostLabel := env.GetString("RETRY_HOST_NAME_LABEL_KEY", "kubernetes.io/hostname")
+			if hostLabel != "" && len(hostNames) > 0 {
+				pod.Spec.Affinity = wfretry.AddHostnamesToAffinity(hostLabel, hostNames, pod.Spec.Affinity)
+			}
 		}
 	}
 }

workflow/controller/retry_tweak_test.go

@@ -15,66 +15,67 @@ func TestFindRetryNode(t *testing.T) {
 			Type:         wfv1.NodeTypeSteps,
 			Phase:        wfv1.NodeRunning,
 			BoundaryID:   "",
-			Children:     []string{"B1", "B2"},
+			Children:     []string{"B1", "B2", "B3"},
 			TemplateName: "tmpl1",
 		},
 		"B1": wfv1.NodeStatus{
 			ID:           "B1",
 			Type:         wfv1.NodeTypeSkipped,
 			Phase:        wfv1.NodeSkipped,
-			BoundaryID:   "",
+			BoundaryID:   "A1",
 			Children:     []string{},
 			TemplateName: "tmpl2",
 		},
-		// retry node
+		// retry node containing steps
 		"B2": wfv1.NodeStatus{
 			ID:           "B2",
 			Type:         wfv1.NodeTypeRetry,
 			Phase:        wfv1.NodeRunning,
-			BoundaryID:   "",
+			BoundaryID:   "A1",
 			Children:     []string{"C1"},
 			TemplateName: "tmpl1",
 		},
 		"C1": wfv1.NodeStatus{
 			ID:           "C1",
 			Type:         wfv1.NodeTypeSteps,
 			Phase:        wfv1.NodeRunning,
-			BoundaryID:   "",
+			BoundaryID:   "A1",
 			Children:     []string{"D1", "D2"},
 			TemplateName: "tmpl2",
 		},
 		"D1": wfv1.NodeStatus{
 			ID:           "D1",
 			Type:         wfv1.NodeTypeSkipped,
 			Phase:        wfv1.NodeSkipped,
-			BoundaryID:   "A1",
+			BoundaryID:   "C1",
 			Children:     []string{},
 			TemplateName: "tmpl2",
 		},
 		"D2": wfv1.NodeStatus{
 			ID:           "D2",
 			Type:         wfv1.NodeTypePod,
 			Phase:        wfv1.NodeRunning,
-			BoundaryID:   "A1",
+			BoundaryID:   "C1",
 			Children:     []string{},
 			TemplateName: "tmpl2",
 		},
-		"E1": wfv1.NodeStatus{
-			ID:         "E1",
+		// retry node containing single step and templteRef
+		"B3": wfv1.NodeStatus{
+			ID:         "B3",
 			Type:       wfv1.NodeTypeRetry,
 			Phase:      wfv1.NodeRunning,
 			BoundaryID: "A1",
-			Children:   []string{},
+			Children:   []string{"C2"},
 			TemplateRef: &wfv1.TemplateRef{
 				Name:     "tmpl1",
 				Template: "tmpl3",
 			},
 		},
-		"E2": wfv1.NodeStatus{
-			ID:           "E2",
+		"C2": wfv1.NodeStatus{
+			ID:           "C2",
 			Type:         wfv1.NodeTypePod,
 			Phase:        wfv1.NodeRunning,
-			BoundaryID:   "E1",
+			BoundaryID:   "A1",
 			Children:     []string{},
 			TemplateName: "tmpl2",
 		},
@@ -88,7 +89,7 @@ func TestFindRetryNode(t *testing.T) {
 		assert.Nil(t, a)
 	})
 	t.Run("Expect to find retry node has TemplateRef", func(t *testing.T) {
-		node := allNodes["E1"]
-		assert.Equal(t, FindRetryNode(allNodes, "E2"), &node)
+		node := allNodes["B3"]
+		assert.Equal(t, FindRetryNode(allNodes, "C2"), &node)
 	})
 }

workflow/util/util.go

@@ -1169,9 +1169,7 @@ func FormulateRetryWorkflow(ctx context.Context, wf *wfv1.Workflow, restartSucce
 		if node.FailedOrError() && isExecutionNodeType(node.Type) {
 			// Check its parent if current node is retry node
 			if node.NodeFlag != nil && node.NodeFlag.Retried {
-				node = *wf.Status.Nodes.Find(func(nodeStatus wfv1.NodeStatus) bool {
-					return nodeStatus.HasChild(node.ID)
-				})
+				node = *wf.Status.Nodes.FindByChild(nodeID)
 			}
 			if !isDescendantNodeSucceeded(wf, node, deleteNodesMap) {
 				failed[nodeID] = true