From 68979f6e3dab8225765e166d346502e7e66b0c77 Mon Sep 17 00:00:00 2001 From: Simon Behar Date: Fri, 12 Feb 2021 10:35:19 -0800 Subject: [PATCH] fix: Do not create pods under shutdown strategy (#5055) Signed-off-by: Simon Behar --- test/e2e/functional/stop-terminate-2.yaml | 24 +++++++++++++++++ test/e2e/functional/stop-terminate.yaml | 6 ++--- test/e2e/signals_test.go | 32 +++++++++++++++++++--- workflow/controller/operator_test.go | 33 +++++++++++++++++++++++ workflow/controller/workflowpod.go | 6 +++++ 5 files changed, 94 insertions(+), 7 deletions(-) create mode 100644 test/e2e/functional/stop-terminate-2.yaml diff --git a/test/e2e/functional/stop-terminate-2.yaml b/test/e2e/functional/stop-terminate-2.yaml new file mode 100644 index 000000000000..c6792000e326 --- /dev/null +++ b/test/e2e/functional/stop-terminate-2.yaml @@ -0,0 +1,24 @@ +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: stop-terminate- + labels: + argo-e2e: true +spec: + entrypoint: main + templates: + - name: main + steps: + - - name: A + template: sleep + - - name: B + template: pass + + - name: sleep + container: + image: argoproj/argosay:v1 + args: [ sleep, "999"] + + - name: pass + container: + image: argoproj/argosay:v1 diff --git a/test/e2e/functional/stop-terminate.yaml b/test/e2e/functional/stop-terminate.yaml index 8d3da2e5d5b4..e0ab37e0e5a4 100644 --- a/test/e2e/functional/stop-terminate.yaml +++ b/test/e2e/functional/stop-terminate.yaml @@ -17,9 +17,9 @@ spec: - name: echo container: - image: argoproj/argosay:v2 - args: [ sleep, "999" ] + image: argoproj/argosay:v1 + args: [ sleep, "999"] - name: exit container: - image: argoproj/argosay:v2 \ No newline at end of file + image: argoproj/argosay:v1 diff --git a/test/e2e/signals_test.go b/test/e2e/signals_test.go index 4898a2d2a768..e8add2f5b54e 100644 --- a/test/e2e/signals_test.go +++ b/test/e2e/signals_test.go @@ -34,7 +34,7 @@ func (s *SignalsSuite) TestStopBehavior() { assert.NoError(t, err) assert.Regexp(t, "workflow stop-terminate-.* stopped", output) }). - WaitForWorkflow(). + WaitForWorkflow(1 * time.Minute). Then(). ExpectWorkflow(func(t *testing.T, m *metav1.ObjectMeta, status *wfv1.WorkflowStatus) { assert.Equal(t, wfv1.WorkflowFailed, status.Phase) @@ -63,7 +63,7 @@ func (s *SignalsSuite) TestTerminateBehavior() { assert.NoError(t, err) assert.Regexp(t, "workflow stop-terminate-.* terminated", output) }). - WaitForWorkflow(). + WaitForWorkflow(1 * time.Minute). Then(). ExpectWorkflow(func(t *testing.T, m *metav1.ObjectMeta, status *wfv1.WorkflowStatus) { assert.Equal(t, wfv1.WorkflowFailed, status.Phase) @@ -78,6 +78,30 @@ func (s *SignalsSuite) TestTerminateBehavior() { }) } +// Tests that new pods are never created once a stop shutdown strategy has been added +func (s *SignalsSuite) TestDoNotCreatePodsUnderStopBehavior() { + s.Given(). + Workflow("@functional/stop-terminate-2.yaml"). + When(). + SubmitWorkflow(). + WaitForWorkflow(fixtures.ToStart, "to start"). + RunCli([]string{"stop", "@latest"}, func(t *testing.T, output string, err error) { + assert.NoError(t, err) + assert.Regexp(t, "workflow stop-terminate-.* stopped", output) + }). + WaitForWorkflow(1 * time.Minute). + Then(). + ExpectWorkflow(func(t *testing.T, m *metav1.ObjectMeta, status *wfv1.WorkflowStatus) { + assert.Equal(t, wfv1.WorkflowFailed, status.Phase) + nodeStatus := status.Nodes.FindByDisplayName("A") + if assert.NotNil(t, nodeStatus) { + assert.Equal(t, wfv1.NodeFailed, nodeStatus.Phase) + } + nodeStatus = status.Nodes.FindByDisplayName("B") + assert.Nil(t, nodeStatus) + }) +} + func (s *SignalsSuite) TestPropagateMaxDuration() { s.T().Skip("too hard to get working") s.Given(). @@ -106,7 +130,7 @@ spec: `). When(). SubmitWorkflow(). - WaitForWorkflow(45 * time.Second). + WaitForWorkflow(1 * time.Minute). Then(). ExpectWorkflow(func(t *testing.T, _ *metav1.ObjectMeta, status *wfv1.WorkflowStatus) { assert.Equal(t, wfv1.WorkflowFailed, status.Phase) @@ -123,7 +147,7 @@ func (s *SignalsSuite) TestSidecars() { Workflow("@testdata/sidecar-workflow.yaml"). When(). SubmitWorkflow(). - WaitForWorkflow(). + WaitForWorkflow(1 * time.Minute). Then(). ExpectWorkflow(func(t *testing.T, _ *metav1.ObjectMeta, status *wfv1.WorkflowStatus) { assert.Equal(t, wfv1.WorkflowSucceeded, status.Phase) diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go index aaf2107f48d5..cd03158280bd 100644 --- a/workflow/controller/operator_test.go +++ b/workflow/controller/operator_test.go @@ -5770,6 +5770,7 @@ func TestParamAggregation(t *testing.T) { } } } + func TestRetryOnDiffHost(t *testing.T) { cancel, controller := newController() defer cancel() @@ -5836,3 +5837,35 @@ func TestRetryOnDiffHost(t *testing.T) { } assert.Equal(t, sourceNodeSelectorRequirement, targetNodeSelectorRequirement) } + +var noPodsWhenShutdown = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + name: hello-world +spec: + entrypoint: whalesay + shutdown: "Stop" + templates: + - name: whalesay + container: + image: docker/whalesay:latest + command: [cowsay] + args: ["hello world"] +` + +func TestNoPodsWhenShutdown(t *testing.T) { + wf := unmarshalWF(noPodsWhenShutdown) + cancel, controller := newController(wf) + defer cancel() + + ctx := context.Background() + woc := newWorkflowOperationCtx(wf, controller) + woc.operate(ctx) + + node := woc.wf.Status.Nodes.FindByDisplayName("hello-world") + if assert.NotNil(t, node) { + assert.Equal(t, wfv1.NodeSkipped, node.Phase) + assert.Contains(t, node.Message, "workflow shutdown with strategy: Stop") + } +} diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index 18abcd9a827a..728e98d86c14 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -147,6 +147,12 @@ func (woc *wfOperationCtx) createWorkflowPod(ctx context.Context, nodeName strin } } + if !woc.execWf.Spec.Shutdown.ShouldExecute(opts.onExitPod) { + // Do not create pods if we are shutting down + woc.markNodePhase(nodeName, wfv1.NodeSkipped, fmt.Sprintf("workflow shutdown with strategy: %s", woc.execWf.Spec.Shutdown)) + return nil, nil + } + tmpl = tmpl.DeepCopy() wfSpec := woc.execWf.Spec.DeepCopy()