return prune data when context canceled #33979

allencloud · 2017-07-06T08:54:27Z

Signed-off-by: allencloud [email protected]

This PR takes into user's cancel request when prune data into consideration. When context is canceled, docker daemon still returns the data which is already pruned.

Prune Object includes: container, network, image, volume, and build cache.

fixes #33957

- What I did

make engine return prune data even if context is canceled.

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

/cc @mlaventure

mlaventure · 2017-07-06T09:22:31Z

daemon/prune.go

@@ -215,7 +221,8 @@ func (daemon *Daemon) ImagesPrune(ctx context.Context, pruneFilters filters.Args
 	for id, img := range allImages {
 		select {
 		case <-ctx.Done():
-			return nil, ctx.Err()
+			// when the context is cancelled, only return the report struct as presently
+			return nil, nil


This one can stay as before as nothing was altered yet.

Oh, thanks. Updated now.

mlaventure

LGTM

cpuguy83 · 2017-07-06T16:19:51Z

daemon/prune.go

@@ -154,6 +156,10 @@ func (daemon *Daemon) VolumesPrune(ctx context.Context, pruneFilters filters.Arg

 	err = daemon.traverseLocalVolumes(pruneVols)

+	if err == errPruneCancelled {


Do we need this new error?
Why not if err == context.Cancelled?

This could also check context.DeadlineExceeded

Thanks for your feedback. @cpuguy83
Currently I have updated the PR and only checked err == context.Cancelled. And if we also check context.DeadlineExceeded, what should we return, a report with nil, or a report with error?

cpuguy83 · 2017-07-06T16:20:31Z

daemon/prune.go

@@ -26,7 +26,8 @@ import (
 var (
 	// errPruneRunning is returned when a prune request is received while
 	// one is in progress
-	errPruneRunning = fmt.Errorf("a prune operation is already running")
+	errPruneRunning   = fmt.Errorf("a prune operation is already running")
+	errPruneCancelled = fmt.Errorf("a prune operation is cancelled")


I don't think think we need to add this new error here, but in any case the wording is a bit funky.
Maybe the prune operation was cancelled

cpuguy83 · 2017-07-06T16:21:26Z

daemon/prune.go

@@ -122,7 +124,7 @@ func (daemon *Daemon) VolumesPrune(ctx context.Context, pruneFilters filters.Arg
 		select {
 		case <-ctx.Done():
 			logrus.Warnf("VolumesPrune operation cancelled: %#v", *rep)
-			return ctx.Err()
+			return errPruneCancelled


I don't think it's right to return a cancelled error here since cancellation is just one reason why the context would be done.

cpuguy83 · 2017-07-06T16:22:30Z

daemon/prune.go

@@ -320,6 +325,7 @@ func (daemon *Daemon) localNetworksPrune(ctx context.Context, pruneFilters filte
 	l := func(nw libnetwork.Network) bool {
 		select {
 		case <-ctx.Done():
+			// when the context is cancelled, only return the report struct as presently


This sentence doesn't look complete.

cpuguy83 · 2017-07-06T16:22:34Z

daemon/prune.go

@@ -370,7 +376,8 @@ func (daemon *Daemon) clusterNetworksPrune(ctx context.Context, pruneFilters fil
 	for _, nw := range networks {
 		select {
 		case <-ctx.Done():
-			return rep, ctx.Err()
+			// when the context is cancelled, only return the report struct as presently


This sentence doesn't look complete.

cpuguy83 · 2017-07-06T16:22:39Z

daemon/prune.go

@@ -428,7 +435,8 @@ func (daemon *Daemon) NetworksPrune(ctx context.Context, pruneFilters filters.Ar
 	select {
 	case <-ctx.Done():
 		logrus.Warnf("NetworksPrune operation cancelled: %#v", *rep)
-		return nil, ctx.Err()
+		// when the context is cancelled, only return the report struct as presently


This sentence doesn't look complete.

cpuguy83 · 2017-07-07T20:50:19Z

builder/fscache/fscache.go

 	s.mu.Lock()
 	defer s.mu.Unlock()
 	var size uint64

 	for id, snap := range s.sources {
+		select {
+		case <-ctx.Done():
+			logrus.Warnf("Cache prune operation cancelled, pruned size: %d", size)


Can be a debug

I see others are warnings as well...

cpuguy83 · 2017-07-07T20:57:43Z

daemon/prune.go

@@ -428,7 +432,8 @@ func (daemon *Daemon) NetworksPrune(ctx context.Context, pruneFilters filters.Ar
 	select {
 	case <-ctx.Done():
 		logrus.Warnf("NetworksPrune operation cancelled: %#v", *rep)
-		return nil, ctx.Err()
+		// when the context is cancelled, return current report struct and nil


This is obvious from the return line, I don't think we need the comment explaining

cpuguy83 · 2017-07-07T20:57:47Z

daemon/prune.go

@@ -370,7 +373,8 @@ func (daemon *Daemon) clusterNetworksPrune(ctx context.Context, pruneFilters fil
 	for _, nw := range networks {
 		select {
 		case <-ctx.Done():
-			return rep, ctx.Err()
+			// when the context is cancelled, return current report struct and nil


This is obvious from the return line, I don't think we need the comment explaining

allencloud · 2017-07-09T01:24:12Z

Thanks for your review again. @cpuguy83
I have updated that, PTAL.

thaJeztah

Left some questions / comments, thanks!

thaJeztah · 2017-07-10T00:33:25Z

daemon/prune.go

-	err = daemon.traverseLocalVolumes(pruneVols)
+	if err := daemon.traverseLocalVolumes(pruneVols); err == context.Canceled {
+		return rep, nil
+	}

 	return rep, err


this is now returning a different error than before, because the err = daemon.traverseLocalVolumes() is now moved inside the if ...

thaJeztah · 2017-07-10T00:34:33Z

daemon/prune.go

@@ -152,7 +152,9 @@ func (daemon *Daemon) VolumesPrune(ctx context.Context, pruneFilters filters.Arg
 		return nil
 	}

-	err = daemon.traverseLocalVolumes(pruneVols)
+	if err := daemon.traverseLocalVolumes(pruneVols); err == context.Canceled {
+		return rep, nil


related to my comment two lines below; this doesn't return if err == nil, correct?

What you mentioned is right, I have updated this, Thanks a lot. @thaJeztah
PTAL

thaJeztah · 2017-07-10T00:36:56Z

daemon/prune.go

@@ -320,6 +321,7 @@ func (daemon *Daemon) localNetworksPrune(ctx context.Context, pruneFilters filte
 	l := func(nw libnetwork.Network) bool {
 		select {
 		case <-ctx.Done():
+			// when the context is cancelled, return true to stop the networks traverse


nit; "when" should probably be "if"

would it be enough to just mention "return true to stop network traverse"? or is "context cancelled" a special case?

thaJeztah · 2017-07-10T00:39:17Z

daemon/prune.go

@@ -427,8 +429,8 @@ func (daemon *Daemon) NetworksPrune(ctx context.Context, pruneFilters filters.Ar

 	select {
 	case <-ctx.Done():
-		logrus.Warnf("NetworksPrune operation cancelled: %#v", *rep)
-		return nil, ctx.Err()
+		logrus.Debugf("NetworksPrune operation cancelled: %#v", *rep)


I see @cpuguy83 suggested "debug"; just wondering if changing from "warn" in the current code to "debug" is too big a change, and if "info" would be a better fit.

Not a show stopper, just thinking aloud

I think debug is fine. Not sure if that message being in the log by default has any benefits

Signed-off-by: allencloud <[email protected]>

thaJeztah · 2017-07-10T21:09:50Z

hm, flaky? https://jenkins.dockerproject.org/job/Docker-PRs/44175/console

03:33:45 ----------------------------------------------------------------------
03:33:45 FAIL: docker_cli_swarm_test.go:1146: DockerSwarmSuite.TestSwarmLockUnlockCluster
03:33:45 
03:33:45 [d6209096216f9] waiting for daemon to start
03:33:45 [d6209096216f9] daemon started
03:33:45 
03:33:45 [df75e109fdee3] waiting for daemon to start
03:33:45 [df75e109fdee3] daemon started
03:33:45 
03:33:45 [ded4392fb23e8] waiting for daemon to start
03:33:45 [ded4392fb23e8] daemon started
03:33:45 
03:33:45 [df75e109fdee3] exiting daemon
03:33:45 [df75e109fdee3] waiting for daemon to start
03:33:45 [df75e109fdee3] daemon started
03:33:45 
03:33:45 [df75e109fdee3] exiting daemon
03:33:45 [d6209096216f9] exiting daemon
03:33:45 [d6209096216f9] waiting for daemon to start
03:33:45 [d6209096216f9] daemon started
03:33:45 
03:33:45 [ded4392fb23e8] exiting daemon
03:33:45 [ded4392fb23e8] waiting for daemon to start
03:33:45 [ded4392fb23e8] daemon started
03:33:45 
03:33:45 [df75e109fdee3] waiting for daemon to start
03:33:45 [df75e109fdee3] daemon started
03:33:45 
03:33:45 docker_cli_swarm_test.go:1192:
03:33:45     // d2 is now set to lock
03:33:45     checkSwarmUnlockedToLocked(c, d2)
03:33:45 docker_utils_test.go:471:
03:33:45     c.Assert(v, checker, args...)
03:33:45 ... obtained bool = false
03:33:45 ... expected bool = true
03:33:45 
03:33:45 [d6209096216f9] exiting daemon
03:33:45 [df75e109fdee3] exiting daemon
03:33:45 [ded4392fb23e8] exiting daemon
03:34:06

allencloud · 2017-07-11T01:09:25Z

Great to see all green. ☀️ 😄

thaJeztah

LGTM

@cpuguy83 PTAL

cpuguy83

LGTM

GordonTheTurtle added the status/0-triage label Jul 6, 2017

allencloud force-pushed the return-prune-data-when-context-canceled branch from 00888cf to 47c13aa Compare July 6, 2017 09:07

mlaventure suggested changes Jul 6, 2017

View reviewed changes

mlaventure added the rebuild/* label Jul 6, 2017

GordonTheTurtle removed the rebuild/* label Jul 6, 2017

allencloud force-pushed the return-prune-data-when-context-canceled branch 2 times, most recently from d842e14 to 5b54eb3 Compare July 6, 2017 09:53

mlaventure approved these changes Jul 6, 2017

View reviewed changes

cpuguy83 requested changes Jul 6, 2017

View reviewed changes

LK4D4 added status/2-code-review and removed status/0-triage labels Jul 6, 2017

allencloud force-pushed the return-prune-data-when-context-canceled branch 4 times, most recently from c69a42b to 6f12a2b Compare July 7, 2017 02:55

cpuguy83 requested changes Jul 7, 2017

View reviewed changes

allencloud force-pushed the return-prune-data-when-context-canceled branch 2 times, most recently from 4431087 to 88a3eeb Compare July 8, 2017 01:54

thaJeztah requested changes Jul 10, 2017

View reviewed changes

return prune data when context canceled

87b4dc2

Signed-off-by: allencloud <[email protected]>

allencloud force-pushed the return-prune-data-when-context-canceled branch from 88a3eeb to 87b4dc2 Compare July 10, 2017 02:06

thaJeztah added the rebuild/janky label Jul 10, 2017

GordonTheTurtle removed the rebuild/janky label Jul 10, 2017

thaJeztah approved these changes Jul 11, 2017

View reviewed changes

cpuguy83 approved these changes Jul 11, 2017

View reviewed changes

cpuguy83 merged commit 73e8f56 into moby:master Jul 11, 2017

allencloud deleted the return-prune-data-when-context-canceled branch July 11, 2017 01:19

thaJeztah added the impact/changelog label Jul 11, 2017

thaJeztah mentioned this pull request Jul 11, 2017

Flaky test: DockerSwarmSuite.TestSwarmLockUnlockCluster #34051

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return prune data when context canceled #33979

return prune data when context canceled #33979

allencloud commented Jul 6, 2017 •

edited

Loading

mlaventure Jul 6, 2017

allencloud Jul 6, 2017

mlaventure left a comment

cpuguy83 Jul 6, 2017

cpuguy83 Jul 6, 2017

allencloud Jul 7, 2017

cpuguy83 Jul 6, 2017

cpuguy83 Jul 6, 2017

cpuguy83 Jul 6, 2017

cpuguy83 Jul 6, 2017

cpuguy83 Jul 6, 2017

cpuguy83 Jul 7, 2017

cpuguy83 Jul 7, 2017

cpuguy83 Jul 7, 2017

cpuguy83 Jul 7, 2017

allencloud commented Jul 9, 2017

thaJeztah left a comment

thaJeztah Jul 10, 2017

thaJeztah Jul 10, 2017

allencloud Jul 10, 2017 •

edited

Loading

thaJeztah Jul 10, 2017

thaJeztah Jul 10, 2017

mlaventure Jul 10, 2017

thaJeztah commented Jul 10, 2017

allencloud commented Jul 11, 2017

thaJeztah left a comment

cpuguy83 left a comment

		@@ -154,6 +156,10 @@ func (daemon *Daemon) VolumesPrune(ctx context.Context, pruneFilters filters.Arg

		err = daemon.traverseLocalVolumes(pruneVols)

		if err == errPruneCancelled {

return prune data when context canceled #33979

return prune data when context canceled #33979

Conversation

allencloud commented Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlaventure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Jul 9, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jul 10, 2017

allencloud commented Jul 11, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

allencloud commented Jul 6, 2017 •

edited

Loading

allencloud Jul 10, 2017 •

edited

Loading