return prune data when context canceled by allencloud · Pull Request #33979 · moby/moby

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

Signed-off-by: allencloud allen.sun@daocloud.io

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

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

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

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

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

This sentence doesn't look complete.

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

daemon/prune.go

This sentence doesn't look complete.

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

daemon/prune.go

This sentence doesn't look complete.

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

builder/fscache/fscache.go

Can be a debug

I see others are warnings as well...

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

daemon/prune.go

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

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

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

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

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

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 <allen.sun@daocloud.io>

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 <allen.sun@daocloud.io>

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

Conversation

allencloud commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure left a comment

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!

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!

allencloud commented Jul 9, 2017

Uh oh!

thaJeztah left a comment

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!

allencloud Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

thaJeztah commented Jul 10, 2017

Uh oh!

allencloud commented Jul 11, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

allencloud commented Jul 6, 2017 •

edited

Loading

allencloud Jul 10, 2017 •

edited

Loading