diff --git a/workflow/controller/controller_test.go b/workflow/controller/controller_test.go index be053327ecf0..8036bf4f85bf 100644 --- 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 index a3d467013707..753e3bb89171 100644 --- 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 index 86099e6f4b78..9e093810f8d4 100644 --- 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 index 9482b211711c..6bbc80ed5747 100644 --- 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 index d783b5c4ddf4..f2e538d46fc3 100644 --- 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