chore: Remove onExit naming transition scaffolding code (#6297)

Signed-off-by: Simon Behar <simbeh7@gmail.com>
argoproj · Aug 22, 2021 · 48d7ad3 · 48d7ad3
1 parent b0e050e
commit 48d7ad3
Show file tree

Hide file tree

Showing 5 changed files with 7 additions and 254 deletions.
diff --git a/workflow/controller/controller_test.go b/workflow/controller/controller_test.go
@@ -281,7 +281,9 @@ func withAnnotation(key, val string) with {
 
 // createRunningPods creates the pods that are marked as running in a given test so that they can be accessed by the
 // pod assessor
-func createRunningPods(ctx context.Context, woc *wfOperationCtx) {
+// This function is called `createRunningPods`, but since it is currently not used and should not be removed it has been
+// renamed to `_` until a use is found.
+func _(ctx context.Context, woc *wfOperationCtx) {
 	podcs := woc.controller.kubeclientset.CoreV1().Pods(woc.wf.GetNamespace())
 	for _, node := range woc.wf.Status.Nodes {
 		if node.Type == wfv1.NodeTypePod && node.Phase == wfv1.NodeRunning {

diff --git a/workflow/controller/dag.go b/workflow/controller/dag.go
@@ -249,7 +249,7 @@ func (woc *wfOperationCtx) executeDAG(ctx context.Context, nodeName string, tmpl
 			if taskNode.Completed() {
 				// Run the node's onExit node, if any. Since this is a target task, we don't need to consider the status
 				// of the onExit node before continuing. That will be done in assesDAGPhase
-				_, _, err := woc.runOnExitNode(ctx, dagCtx.GetTask(taskName).GetExitHook(woc.execWf.Spec.Arguments), taskName, taskNode.Name, dagCtx.boundaryID, dagCtx.tmplCtx, "tasks."+taskName, taskNode.Outputs)
+				_, _, err := woc.runOnExitNode(ctx, dagCtx.GetTask(taskName).GetExitHook(woc.execWf.Spec.Arguments), taskNode.Name, dagCtx.boundaryID, dagCtx.tmplCtx, "tasks."+taskName, taskNode.Outputs)
 				if err != nil {
 					return node, err
 				}
@@ -338,7 +338,7 @@ func (woc *wfOperationCtx) executeDAGTask(ctx context.Context, dagCtx *dagContex
 
 		if node.Completed() {
 			// Run the node's onExit node, if any.
-			hasOnExitNode, onExitNode, err := woc.runOnExitNode(ctx, task.GetExitHook(woc.execWf.Spec.Arguments), task.Name, node.Name, dagCtx.boundaryID, dagCtx.tmplCtx, "tasks."+taskName, node.Outputs)
+			hasOnExitNode, onExitNode, err := woc.runOnExitNode(ctx, task.GetExitHook(woc.execWf.Spec.Arguments), node.Name, dagCtx.boundaryID, dagCtx.tmplCtx, "tasks."+taskName, node.Outputs)
 			if hasOnExitNode && (onExitNode == nil || !onExitNode.Fulfilled() || err != nil) {
 				// The onExit node is either not complete or has errored out, return.
 				return
@@ -697,19 +697,6 @@ func (d *dagContext) evaluateDependsLogic(taskName string) (bool, bool, error) {
 			}
 		}
 
-		// Previously we used `depNode.DisplayName` to generate all onExit node names. However, as these can be non-unique
-		// we transitioned to using `depNode.Name` instead, which are guaranteed to be unique. In order to not disrupt
-		// running workflows during upgrade time, we do an additional check to see if there is an onExit node with the old
-		// name (`depNode.DisplayName`).
-		// TODO: This scaffold code should be removed after a couple of "grace period" version upgrades to allow transitions. It was introduced in v3.0.0
-		// See more: https://github.com/argoproj/argo-workflows/issues/5502
-		legacyOnExitNodeName := common.GenerateOnExitNodeName(depNode.DisplayName)
-		if onExitNode := d.wf.GetNodeByName(legacyOnExitNodeName); onExitNode != nil && d.wf.GetNodeByName(depNode.Name).HasChild(onExitNode.ID) {
-			if !onExitNode.Fulfilled() {
-				return false, false, nil
-			}
-		}
-
 		evalTaskName := strings.Replace(taskName, "-", "_", -1)
 		if _, ok := evalScope[evalTaskName]; ok {
 			continue

diff --git a/workflow/controller/exit_handler.go b/workflow/controller/exit_handler.go
@@ -11,26 +11,11 @@ import (
 	"github.com/argoproj/argo-workflows/v3/workflow/templateresolution"
 )
 
-func (woc *wfOperationCtx) runOnExitNode(ctx context.Context, exitHook *wfv1.LifecycleHook, parentDisplayName, parentNodeName, boundaryID string, tmplCtx *templateresolution.Context, prefix string, outputs *wfv1.Outputs) (bool, *wfv1.NodeStatus, error) {
+func (woc *wfOperationCtx) runOnExitNode(ctx context.Context, exitHook *wfv1.LifecycleHook, parentNodeName, boundaryID string, tmplCtx *templateresolution.Context, prefix string, outputs *wfv1.Outputs) (bool, *wfv1.NodeStatus, error) {
 	if exitHook != nil && woc.GetShutdownStrategy().ShouldExecute(true) {
 		woc.log.WithField("lifeCycleHook", exitHook).Infof("Running OnExit handler")
 
-		// Previously we used `parentDisplayName` to generate all onExit node names. However, as these can be non-unique
-		// we transitioned to using `parentNodeName` instead, which are guaranteed to be unique. In order to not disrupt
-		// running workflows during upgrade time, we first check if there is an onExit node that currently exists with the
-		// legacy name AND said node is a child of the parent node. If it does, we continue execution with the legacy name.
-		// If it doesn't, we use the new (and unique) name for all operations henceforth.
-		// TODO: This scaffold code should be removed after a couple of "grace period" version upgrades to allow transitions. It was introduced in v3.0.0
-		// When the scaffold code is removed, we should only have the following:
-		//
-		// 		onExitNodeName := common.GenerateOnExitNodeName(parentNodeName)
-		//
-		// See more: https://github.com/argoproj/argo-workflows/issues/5502
 		onExitNodeName := common.GenerateOnExitNodeName(parentNodeName)
-		legacyOnExitNodeName := common.GenerateOnExitNodeName(parentDisplayName)
-		if legacyNameNode := woc.wf.GetNodeByName(legacyOnExitNodeName); legacyNameNode != nil && woc.wf.GetNodeByName(parentNodeName).HasChild(legacyNameNode.ID) {
-			onExitNodeName = legacyOnExitNodeName
-		}
 		resolvedArgs := exitHook.Arguments
 		var err error
 		if !resolvedArgs.IsEmpty() && outputs != nil {

diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go
@@ -6781,227 +6781,6 @@ func TestRootRetryStrategyCompletes(t *testing.T) {
 	assert.Equal(t, wfv1.WorkflowSucceeded, woc.wf.Status.Phase)
 }
 
-const testOnExitNameBackwardsCompatibility = `
-apiVersion: argoproj.io/v1alpha1
-kind: Workflow
-metadata:
-  name: hello-world-69h5d
-spec:
-  entrypoint: main
-  templates:
-  - name: main
-    steps:
-    - - name: run
-        onExit: pass
-        template: pass
-  - container:
-      args:
-      - exit 0
-      command:
-      - sh
-      - -c
-      image: alpine
-    name: pass
-  ttlStrategy:
-    secondsAfterCompletion: 600
-status:
-  nodes:
-    hello-world-69h5d:
-      children:
-      - hello-world-69h5d-4087924081
-      displayName: hello-world-69h5d
-      finishedAt: "2021-03-24T14:53:32Z"
-      id: hello-world-69h5d
-      name: hello-world-69h5d
-      outboundNodes:
-      - hello-world-69h5d-928074325
-      phase: Running
-      startedAt: "2021-03-24T14:53:18Z"
-      templateName: main
-      templateScope: local/hello-world-69h5d
-      type: Steps
-    hello-world-69h5d-928074325:
-      boundaryID: hello-world-69h5d
-      displayName: run.onExit
-      finishedAt: "2021-03-24T14:53:31Z"
-      hostNodeName: k3d-k3s-default-server-0
-      id: hello-world-69h5d-928074325
-      name: run.onExit
-      phase: Running
-      startedAt: "2021-03-24T14:53:25Z"
-      templateName: pass
-      templateScope: local/hello-world-69h5d
-      type: Pod
-    hello-world-69h5d-2500098386:
-      boundaryID: hello-world-69h5d
-      children:
-      - hello-world-69h5d-928074325
-      displayName: run
-      finishedAt: "2021-03-24T14:53:24Z"
-      hostNodeName: k3d-k3s-default-server-0
-      id: hello-world-69h5d-2500098386
-      name: hello-world-69h5d[0].run
-      phase: Succeeded
-      startedAt: "2021-03-24T14:53:18Z"
-      templateName: pass
-      templateScope: local/hello-world-69h5d
-      type: Pod
-    hello-world-69h5d-4087924081:
-      boundaryID: hello-world-69h5d
-      children:
-      - hello-world-69h5d-2500098386
-      displayName: '[0]'
-      finishedAt: "2021-03-24T14:53:32Z"
-      id: hello-world-69h5d-4087924081
-      name: hello-world-69h5d[0]
-      phase: Running
-      startedAt: "2021-03-24T14:53:18Z"
-      templateScope: local/hello-world-69h5d
-      type: StepGroup
-  phase: Running
-  startedAt: "2021-03-24T14:53:18Z"
-`
-
-// Previously we used `parentNodeDisplayName` to generate all onExit node names. However, as these can be non-unique
-// we transitioned to using `parentNodeName` instead, which are guaranteed to be unique. In order to not disrupt
-// running workflows during upgrade time, we first check if there is an onExit node that currently exists with the
-// legacy name AND said node is a child of the parent node. If it does, we continue execution with the legacy name.
-// If it doesn't, we use the new (and unique) name for all operations henceforth.
-//
-// Here we test to see if this backwards compatibility works. This test workflow contains a running onExit node with the
-// old name. When we call operate on it, we should NOT create another onExit node with the new name and instead respect
-// the old onExit node.
-//
-// TODO: This test should be removed after a couple of "grace period" version upgrades to allow transitions. It was introduced in v3.0.0
-// See more: https://github.com/argoproj/argo-workflows/issues/5502
-func TestOnExitNameBackwardsCompatibility(t *testing.T) {
-	wf := wfv1.MustUnmarshalWorkflow(testOnExitNameBackwardsCompatibility)
-	cancel, controller := newController(wf)
-	defer cancel()
-
-	ctx := context.Background()
-	woc := newWorkflowOperationCtx(wf, controller)
-
-	createRunningPods(ctx, woc)
-
-	nodesBeforeOperation := len(woc.wf.Status.Nodes)
-	woc.operate(ctx)
-
-	assert.Equal(t, wfv1.WorkflowRunning, woc.wf.Status.Phase)
-	// Number of nodes should not change (no new name node was created)
-	assert.Equal(t, nodesBeforeOperation, len(woc.wf.Status.Nodes))
-	node := woc.wf.Status.Nodes.FindByDisplayName("run.onExit")
-	if assert.NotNil(t, node) {
-		assert.Equal(t, wfv1.NodeRunning, node.Phase)
-	}
-}
-
-const testOnExitDAGStatusCompatibility = `
-apiVersion: argoproj.io/v1alpha1
-kind: Workflow
-metadata:
-  name: dag-diamond-8xw8l
-spec:
-  entrypoint: diamond
-  templates:
-  - dag:
-      tasks:
-      - name: A
-        onExit: echo
-        template: echo
-      - depends: A
-        name: B
-        template: echo
-    name: diamond
-  - container:
-      command:
-      - echo
-      - hi
-      image: alpine:3.7
-    name: echo
-status:
-  nodes:
-    dag-diamond-8xw8l:
-      children:
-      - dag-diamond-8xw8l-1488416551
-      displayName: dag-diamond-8xw8l
-      finishedAt: "2021-03-24T15:37:06Z"
-      id: dag-diamond-8xw8l
-      name: dag-diamond-8xw8l
-      outboundNodes:
-      - dag-diamond-8xw8l-1505194170
-      phase: Running
-      startedAt: "2021-03-24T15:36:47Z"
-      templateName: diamond
-      templateScope: local/dag-diamond-8xw8l
-      type: DAG
-    dag-diamond-8xw8l-1342580575:
-      boundaryID: dag-diamond-8xw8l
-      displayName: A.onExit
-      finishedAt: "2021-03-24T15:36:59Z"
-      hostNodeName: k3d-k3s-default-server-0
-      id: dag-diamond-8xw8l-1342580575
-      name: A.onExit
-      phase: Running
-      startedAt: "2021-03-24T15:36:54Z"
-      templateName: echo
-      templateScope: local/dag-diamond-8xw8l
-      type: Pod
-    dag-diamond-8xw8l-1488416551:
-      boundaryID: dag-diamond-8xw8l
-      children:
-      - dag-diamond-8xw8l-1342580575
-      displayName: A
-      finishedAt: "2021-03-24T15:36:53Z"
-      hostNodeName: k3d-k3s-default-server-0
-      id: dag-diamond-8xw8l-1488416551
-      name: dag-diamond-8xw8l.A
-      phase: Succeeded
-      startedAt: "2021-03-24T15:36:47Z"
-      templateName: echo
-      templateScope: local/dag-diamond-8xw8l
-      type: Pod
-  phase: Running
-  startedAt: "2021-03-24T15:36:47Z"
-`
-
-// Previously we used `parentNodeDisplayName` to generate all onExit node names. However, as these can be non-unique
-// we transitioned to using `parentNodeName` instead, which are guaranteed to be unique. In order to not disrupt
-// running workflows during upgrade time, we first check if there is an onExit node that currently exists with the
-// legacy name AND said node is a child of the parent node. If it does, we continue execution with the legacy name.
-// If it doesn't, we use the new (and unique) name for all operations henceforth.
-//
-// Here we test to see if this backwards compatibility works. This test workflow contains a running onExit node with the
-// old name. When we call operate on it, we should NOT create the subsequent DAG done ("B") until the onExit node name with
-// the old name finishes running.
-//
-// TODO: This test should be removed after a couple of "grace period" version upgrades to allow transitions. It was introduced in v3.0.0
-// See more: https://github.com/argoproj/argo-workflows/issues/5502
-func TestOnExitDAGStatusCompatibility(t *testing.T) {
-	wf := wfv1.MustUnmarshalWorkflow(testOnExitDAGStatusCompatibility)
-	cancel, controller := newController(wf)
-	defer cancel()
-
-	ctx := context.Background()
-	woc := newWorkflowOperationCtx(wf, controller)
-
-	createRunningPods(ctx, woc)
-
-	nodesBeforeOperation := len(woc.wf.Status.Nodes)
-	woc.operate(ctx)
-
-	assert.Equal(t, wfv1.WorkflowRunning, woc.wf.Status.Phase)
-	// Number of nodes should not change (no new name node was created)
-	assert.Equal(t, nodesBeforeOperation, len(woc.wf.Status.Nodes))
-	node := woc.wf.Status.Nodes.FindByDisplayName("A.onExit")
-	if assert.NotNil(t, node) {
-		assert.Equal(t, wfv1.NodeRunning, node.Phase)
-	}
-
-	nodeB := woc.wf.Status.Nodes.FindByDisplayName("B")
-	assert.Nil(t, nodeB)
-}
-
 const testGlobalParamSubstitute = `
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow

diff --git a/workflow/controller/steps.go b/workflow/controller/steps.go
@@ -260,7 +260,7 @@ func (woc *wfOperationCtx) executeStepGroup(ctx context.Context, stepGroup []wfv
 		if !childNode.Fulfilled() {
 			completed = false
 		} else if childNode.Completed() {
-			hasOnExitNode, onExitNode, err := woc.runOnExitNode(ctx, step.GetExitHook(woc.execWf.Spec.Arguments), step.Name, childNode.Name, stepsCtx.boundaryID, stepsCtx.tmplCtx, "steps."+step.Name, childNode.Outputs)
+			hasOnExitNode, onExitNode, err := woc.runOnExitNode(ctx, step.GetExitHook(woc.execWf.Spec.Arguments), childNode.Name, stepsCtx.boundaryID, stepsCtx.tmplCtx, "steps."+step.Name, childNode.Outputs)
 			if hasOnExitNode && (onExitNode == nil || !onExitNode.Fulfilled() || err != nil) {
 				// The onExit node is either not complete or has errored out, return.
 				completed = false