Carry "Don't delete tasks until they're actually removed by the agent" by dperny · Pull Request #2461 · moby/swarmkit

dperny · 2017-11-28T00:51:39Z

Finishes up the work on #2446, because @nishanttotla is unavailable. Added one commit addressing a few nits. See #2446 for original discussion. I'd have just pushed to his branch but I don't have permissions.

/cc @aaronlehmann, @stevvooe, and @anshulpundir, who were involved in the discussion on that PR.

stevvooe · 2017-11-28T01:20:25Z

+		return nil
+	}
+
+	checkDo(ctx, t, task, ctlr, &api.TaskStatus{


Add one more case here that ensures a COMPLETED (or any terminal state) does not proceed to shutdown. It should only proceed to shutdown if the container is not already terminal.

@stevvooe do you mean like this?

// TestDesiredStateRemoveOnlyNonterminal checks that the agent will only stop // a container on REMOVE if it's not already in a terminal state. If the // container is already in a terminal state, (like COMPLETE) the agent should // take no action func TestDesiredStateRemoveOnlyNonterminal(t *testing.T) { // go through all terminal states, just for completeness' sake for _, state := range []api.TaskState{ api.TaskStateCompleted, api.TaskStateShutdown, api.TaskStateFailed, api.TaskStateRejected, api.TaskStateRemove, // no TaskStateOrphaned becaused that's not a state the task can be in // on the agent } { // capture state variable here to run in parallel state := state t.Run(state.String(), func(t *testing.T) { // go parallel to go faster t.Parallel() var ( // create a new task, actual state `state`, desired state // shutdown task = newTestTask(t, state, api.TaskStateShutdown) ctx, ctlr, finish = buildTestEnv(t, task) ) // make the shutdown function a noop ctlr.ShutdownFn = func(_ context.Context) error { return nil } // Note we check for error ErrTaskNoop, which will be raised // because nothing happens checkDo(ctx, t, task, ctlr, &api.TaskStatus{ State: state, }, ErrTaskNoop) defer func() { finish() // we should never have called shutdown assert.Equal(t, 0, ctlr.calls["Shutdown"]) }() }) } }

stevvooe · 2017-11-28T01:21:26Z

 			err := batch.Update(func(tx store.Tx) error {
-				if err := store.DeleteTask(tx, t.ID); err != nil {
-					log.G(ctx).WithError(err).Errorf("failed to delete task")
+				// time travel is not allowed. if the current desired state is


A bug should be filed to have a function that enforces this "math".

Albeit, "desired state" doesn't have the same constraints as the state field, so this is less important.

"DesiredState" is typically directly observable.

I'm unclear what you mean by this.

Sorry for the lack of clarity. I was just pointing out that we should probably always do this check when we change the state. I am not sure it should be done in this PR, as it may introduce a regression.

aaronlehmann · 2017-11-28T04:15:36Z

+  // TaskStateOrphaned is used to free up resources associated with service
+  // tasks on unresponsive nodes without having to delete those tasks. This
+  // state is directly assigned to the task by the orchestrator.
+  ORPHANED = 832 [(gogoproto.enumvalue_customname)="TaskStateOrphaned"];


Something went wrong with indentation here.

codecov · 2017-11-28T18:40:59Z

Codecov Report

Merging #2461 into master will decrease coverage by 3.21%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master    #2461      +/-   ##
==========================================
- Coverage   63.74%   60.53%   -3.22%     
==========================================
  Files          64      128      +64     
  Lines       11793    26433   +14640     
==========================================
+ Hits         7517    16000    +8483     
- Misses       3662     9031    +5369     
- Partials      614     1402     +788

stevvooe · 2017-11-28T19:00:57Z

LGTM after adding the extra test.

dperny · 2017-11-28T19:07:16Z

I'm kicking off a rebuild not because I'm trying to ram this through but because I want to see if the same error happens twice.

dperny · 2017-11-28T21:56:42Z

This should be ready to merge.

anshulpundir

I'd suggest squashing the commits into a single commit:

Set task desired state to REMOVE for service removal and scale down …
Add unit tests for Task state REMOVE behavior …
Fix some nits …

Set task desired state to REMOVE for service removal and scale down. Tasks in REMOVE state can be deleted after they are reported SHUTDOWN. Avoids a race conditions where network resources are freed before the running task has shut down fully. Add unit tests for Task state REMOVE behavior. Started by Nishant Totla <nishanttotla@gmail>, finished by Drew Erny <drew.erny@docker.com>. Signed-off-by: Nishant Totla <nishanttotla@gmail.com> Signed-off-by: Drew Erny <drew.erny@docker.com>

dperny · 2017-11-28T23:00:55Z

@anshulpundir rebased and squashed. Reworded commit message.

aaronlehmann · 2017-11-29T05:37:19Z


-	if len(tasks) > 0 {
-		for _, t := range tasks {
+	if len(orphanedTasks)+len(removeTasks) > 0 {


I think this if statement is superfluous.

Yeah but if I leave it in, when I come back to this code in several months and I'm like, "what doofus made this junk, who's responsible for this?" it will be a very humbling experience.

anshulpundir · 2017-11-29T05:52:47Z

BTW my suggestion was to squash the last three commits. Its prob OK to have the protobuf change as a separate commit. But I guess this is fine too @dperny

dperny · 2017-11-29T18:19:51Z

I think it's fine to have the commits squashed together. Putting the proto changes with the code changes helps put them into context. And we don't often go through the history anyway.

Set task desired state to REMOVE for service removal and scale down. Tasks in REMOVE state can be deleted after they are reported SHUTDOWN. Avoids a race conditions where network resources are freed before the running task has shut down fully. Add unit tests for Task state REMOVE behavior. Started by Nishant Totla <nishanttotla@gmail>, finished by Drew Erny <drew.erny@docker.com>. Signed-off-by: Nishant Totla <nishanttotla@gmail.com> Signed-off-by: Drew Erny <drew.erny@docker.com> Backports moby#2461 to 17.06. Cherry pick did not apply cleanly (cherry picked from commit b9e2c2f) Signed-off-by: Drew Erny <drew.erny@docker.com>

stevvooe reviewed Nov 28, 2017

View reviewed changes

aaronlehmann reviewed Nov 28, 2017

View reviewed changes

dperny force-pushed the task-management-improvements branch from 07682a0 to be6a62e Compare November 28, 2017 18:28

dperny force-pushed the task-management-improvements branch from be6a62e to f8f2a85 Compare November 28, 2017 18:50

anshulpundir suggested changes Nov 28, 2017

View reviewed changes

dperny force-pushed the task-management-improvements branch from f8f2a85 to b9e2c2f Compare November 28, 2017 23:00

aaronlehmann approved these changes Nov 29, 2017

View reviewed changes

anshulpundir approved these changes Nov 29, 2017

View reviewed changes

anshulpundir merged commit e7946de into moby:master Nov 29, 2017

dperny mentioned this pull request Nov 29, 2017

[v17.06] Cherry-pick "Adding new Task Status REMOVE" #2463

Merged

andrewhsu mentioned this pull request Dec 5, 2017

Vendor swarmkit to 4429c763 moby/moby#35698

Merged

friism mentioned this pull request Dec 13, 2017

[Proposal] Resolving the IP address contention issue #2407

Closed

nishanttotla mentioned this pull request Jan 16, 2018

Update design to describe desired state REMOVE behavior for tasks #2485

Merged

tiborvass mentioned this pull request Jan 30, 2018

Service rm/scaledown in an old-worker w/ new manager cluster #2494

Open

thaJeztah mentioned this pull request Feb 2, 2018

Add REMOVE and ORPHANED to TaskState moby/moby#36146

Merged

This was referenced Mar 12, 2018

Potential race condition in scheduler? #2554

Open

Task reaper does not consider some tasks for removal #2555

Open

nishanttotla mentioned this pull request May 4, 2018

[bump_v17.06] Adding Task State Remove and other fixes #2623

Merged

Conversation

dperny commented Nov 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stevvooe commented Nov 28, 2017

Uh oh!

dperny commented Nov 28, 2017

Uh oh!

dperny commented Nov 28, 2017

Uh oh!

anshulpundir left a comment

Choose a reason for hiding this comment

Uh oh!

dperny commented Nov 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anshulpundir commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dperny commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Nov 28, 2017 •

edited

Loading

anshulpundir commented Nov 29, 2017 •

edited

Loading