New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `label` filter for `docker system prune` #30740

Merged
merged 1 commit into from Apr 10, 2017

Conversation

@yongtang
Member

yongtang commented Feb 4, 2017

- What I did

This fix tries to address the issue raised in #29999 where it was not possible to mask these items (like important non-removable stuff) from docker system prune.

- How I did it

This fix adds label and label! field for --filter in system prune, so that it is possible to selectively prune items like:

$ docker container prune --filter label=foo

$ docker container prune --filter label!=bar

- How to verify it

Additional unit tests and integration tests have been added.

- Description for the changelog

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

kittens27

This fix fixes
This fix is an improvement for #29999.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang
Member

yongtang commented Feb 4, 2017

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 6, 2017

Contributor

I wouldn't really call this fixing it since it's a pretty bad interface, and not really any better than manually doing docker <object> rm $(docker <object> list --filter label=foo).

One potential solution to this would be to have the prune command read default filters from the cli config, but even then I'm not sure it's a particularly great interface... though definitely an improvement.

Contributor

cpuguy83 commented Feb 6, 2017

I wouldn't really call this fixing it since it's a pretty bad interface, and not really any better than manually doing docker <object> rm $(docker <object> list --filter label=foo).

One potential solution to this would be to have the prune command read default filters from the cli config, but even then I'm not sure it's a particularly great interface... though definitely an improvement.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Feb 7, 2017

Member

Yeah I agree with @cpuguy83, improvement but not fixing it 👼. Maybe we could also have a daemon configuration that defines some labels that should be protected by default ?

Member

vdemeester commented Feb 7, 2017

Yeah I agree with @cpuguy83, improvement but not fixing it 👼. Maybe we could also have a daemon configuration that defines some labels that should be protected by default ?

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 8, 2017

Member

Thanks @cpuguy83 @vdemeester for the review. I updated the PR and now it is possible to specify pruneFilters in config.json. That should allow pre-configuring the labels for prune to include or exclude. Please take a look.

The docs will be updated if we are ok with the interface.

Member

yongtang commented Feb 8, 2017

Thanks @cpuguy83 @vdemeester for the review. I updated the PR and now it is possible to specify pruneFilters in config.json. That should allow pre-configuring the labels for prune to include or exclude. Please take a look.

The docs will be updated if we are ok with the interface.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 8, 2017

Member

The PR description has also been updated to remove the fixes.

Member

yongtang commented Feb 8, 2017

The PR description has also been updated to remove the fixes.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 7, 2017

Contributor

Design OK, will need a rebase.

ping @vdemeester design ok for you?

Contributor

cpuguy83 commented Mar 7, 2017

Design OK, will need a rebase.

ping @vdemeester design ok for you?

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Mar 7, 2017

Contributor

How does this behave if I have label!=ignore in the config file but I specify label=ignore in the cli? :)

Contributor

mlaventure commented Mar 7, 2017

How does this behave if I have label!=ignore in the config file but I specify label=ignore in the cli? :)

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 7, 2017

Member

The PR has been rebased.

Member

yongtang commented Mar 7, 2017

The PR has been rebased.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 7, 2017

Member

@mlaventure In case of conflicting filtering, the current behavior is to ignore (not prune). I added a test case to show the behavior.

Not sure the best way to handle this scenario so let me know if we want to have special handling (like throw out an error or warning?).

Member

yongtang commented Mar 7, 2017

@mlaventure In case of conflicting filtering, the current behavior is to ignore (not prune). I added a test case to show the behavior.

Not sure the best way to handle this scenario so let me know if we want to have special handling (like throw out an error or warning?).

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Mar 7, 2017

Contributor

Hum, that's a bit annoying a behavior IMHO.

I think we usually have the cli flags supersede the values from the config.

If it doesn't in that case, I'd have to either update temporarily my config file or resort to cascading docker commands.

@cpuguy83 @vdemeester WDYGT?

Contributor

mlaventure commented Mar 7, 2017

Hum, that's a bit annoying a behavior IMHO.

I think we usually have the cli flags supersede the values from the config.

If it doesn't in that case, I'd have to either update temporarily my config file or resort to cascading docker commands.

@cpuguy83 @vdemeester WDYGT?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 7, 2017

Contributor

Agree it should supersede.

Once "--dry-run" it will help in such cases as well to actually see what would happen.

Contributor

cpuguy83 commented Mar 7, 2017

Agree it should supersede.

Once "--dry-run" it will help in such cases as well to actually see what would happen.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 8, 2017

Contributor

*is implemented.

Contributor

cpuguy83 commented Mar 8, 2017

*is implemented.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 8, 2017

Member

Agree with @cpuguy83 @mlaventure, the cli argument should superseed the config 👼

Member

vdemeester commented Mar 8, 2017

Agree with @cpuguy83 @mlaventure, the cli argument should superseed the config 👼

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 8, 2017

Member

Thanks all for the review. The PR has been updated and now CLI filter will supersede the config.json filter.

Member

yongtang commented Mar 8, 2017

Thanks all for the review. The PR has been updated and now CLI filter will supersede the config.json filter.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 8, 2017

Contributor

@mlaventure Good to move to code-review?

Contributor

cpuguy83 commented Mar 8, 2017

@mlaventure Good to move to code-review?

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Mar 8, 2017

Contributor

Yep!

Contributor

mlaventure commented Mar 8, 2017

Yep!

units "github.com/docker/go-units"
"github.com/spf13/cobra"
"golang.org/x/net/context"
)
type pruneOptions struct {
force bool
force bool
filter opts.FilterOpt

This comment has been minimized.

@cpuguy83

cpuguy83 Mar 8, 2017

Contributor

This is not properly initialized, getting a panic when trying to add filters.

@cpuguy83

cpuguy83 Mar 8, 2017

Contributor

This is not properly initialized, getting a panic when trying to add filters.

This comment has been minimized.

@yongtang

yongtang Mar 24, 2017

Member

Thanks @cpuguy83 for the review. The panic has been fixed.

@yongtang

yongtang Mar 24, 2017

Member

Thanks @cpuguy83 for the review. The panic has been fixed.

@@ -111,3 +114,61 @@ func (s *DockerSuite) TestPruneContainerUntil(c *check.C) {
c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), id1)
c.Assert(strings.TrimSpace(out), checker.Contains, id2)
}
func (s *DockerSuite) TestPruneContainerLabel(c *check.C) {

This comment has been minimized.

@cpuguy83

cpuguy83 Mar 8, 2017

Contributor

Looks like we need to test each of the subcommands

@cpuguy83

cpuguy83 Mar 8, 2017

Contributor

Looks like we need to test each of the subcommands

This comment has been minimized.

@yongtang

yongtang Mar 24, 2017

Member

Thanks. Additional tests have been added.

@yongtang

yongtang Mar 24, 2017

Member

Thanks. Additional tests have been added.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 21, 2017

Member

ping @yongtang I see @cpuguy83 left some review notes 😄

Member

thaJeztah commented Mar 21, 2017

ping @yongtang I see @cpuguy83 left some review notes 😄

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 24, 2017

Member

Thanks all for the review. The PR has been updated.

Member

yongtang commented Mar 24, 2017

Thanks all for the review. The PR has been updated.

@cpuguy83

LGTM

@mlaventure

nit on checker use, but LGTM

c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), id1)
c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), id2)
c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), id3)
c.Assert(strings.TrimSpace(out), checker.Contains, id4)

This comment has been minimized.

@mlaventure

mlaventure Apr 7, 2017

Contributor

checker.Equals on id4 would be enough in that case

@mlaventure

mlaventure Apr 7, 2017

Contributor

checker.Equals on id4 would be enough in that case

c.Assert(strings.TrimSpace(out), checker.Contains, id4)
out, _ = dockerCmd(c, "container", "prune", "--force", "--filter", "label=foo")
c.Assert(strings.TrimSpace(out), checker.Contains, id1)

This comment has been minimized.

@mlaventure

mlaventure Apr 7, 2017

Contributor

checker.Equals on id1 would be enough in that case (similar comment for the other subcommands)

@mlaventure

mlaventure Apr 7, 2017

Contributor

checker.Equals on id1 would be enough in that case (similar comment for the other subcommands)

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Apr 7, 2017

Member

Thanks @mlaventure for the review. Currently the output of prune also contains Deleted Images and Total reclaimed space: so it may not be possible to use Equals.

ubuntu@ubuntu:~/docker$ docker image prune -f
Deleted Images:
deleted: sha256:9485e5f3b535139c864b249fc8b135d1dc3e2907f4b0cfda6012b9cc045c8b53
deleted: sha256:483ef533be5120f014ec2d9298b0daa240ec7f0da3b2fa5d4e4bdcd36144491b
deleted: sha256:ca96556fd1b6ea07ac0e9f1f7d9b4d7440780752484e620199a53a91b36b1731
deleted: sha256:725617c4f06697290b288a91ebd884b1f2a1d9e6da3e7514d31c2bab09e65d73
deleted: sha256:85b88b89c53f89f7f4e93941882c751e2af8b25477ad0b23629663014bcbf084
deleted: sha256:b92809f737413ddbd4efecfbf570045f177376a3f4992aa4b9a7ef52e7085c69
deleted: sha256:80ec69f46395bfcbda5de6c2a84f6a5c5a32388b2f7fe59dcb3b1463a45b7779
deleted: sha256:4a389c79f1a7e8d2e5be44e2c2c84e5685a113b80f2a2deac3206c348eaacea4
deleted: sha256:cb9343a57b9a4745c75b1b37607dfce3e9152802e54ba22af955b387d42e7de7
deleted: sha256:8b85a29f5f6e4198db2d4cdd836da03e456197031528daacd8f7be0093836309
deleted: sha256:ceeb09215557367d4781c899034b912381c047bb2899ea3cb3e28fafeda82235
deleted: sha256:0c7f605ab25bd85021421084e557bae9047ba37733ee2a45e988525d18085944

Total reclaimed space: 195.1 MB

I am thinking it might make sense to add an additional flag of -q to image prune (and others) so that only the digests of images are printed?

Member

yongtang commented Apr 7, 2017

Thanks @mlaventure for the review. Currently the output of prune also contains Deleted Images and Total reclaimed space: so it may not be possible to use Equals.

ubuntu@ubuntu:~/docker$ docker image prune -f
Deleted Images:
deleted: sha256:9485e5f3b535139c864b249fc8b135d1dc3e2907f4b0cfda6012b9cc045c8b53
deleted: sha256:483ef533be5120f014ec2d9298b0daa240ec7f0da3b2fa5d4e4bdcd36144491b
deleted: sha256:ca96556fd1b6ea07ac0e9f1f7d9b4d7440780752484e620199a53a91b36b1731
deleted: sha256:725617c4f06697290b288a91ebd884b1f2a1d9e6da3e7514d31c2bab09e65d73
deleted: sha256:85b88b89c53f89f7f4e93941882c751e2af8b25477ad0b23629663014bcbf084
deleted: sha256:b92809f737413ddbd4efecfbf570045f177376a3f4992aa4b9a7ef52e7085c69
deleted: sha256:80ec69f46395bfcbda5de6c2a84f6a5c5a32388b2f7fe59dcb3b1463a45b7779
deleted: sha256:4a389c79f1a7e8d2e5be44e2c2c84e5685a113b80f2a2deac3206c348eaacea4
deleted: sha256:cb9343a57b9a4745c75b1b37607dfce3e9152802e54ba22af955b387d42e7de7
deleted: sha256:8b85a29f5f6e4198db2d4cdd836da03e456197031528daacd8f7be0093836309
deleted: sha256:ceeb09215557367d4781c899034b912381c047bb2899ea3cb3e28fafeda82235
deleted: sha256:0c7f605ab25bd85021421084e557bae9047ba37733ee2a45e988525d18085944

Total reclaimed space: 195.1 MB

I am thinking it might make sense to add an additional flag of -q to image prune (and others) so that only the digests of images are printed?

@vdemeester

LGTM 🐸
The -q flags can/should be handled in a follow-up PR 😉
Moving to docs-review, cc @thaJeztah

@thaJeztah
@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Apr 10, 2017

Member

I am all for doing a follow-up for docs, let's get this merge.
@yongtang ping me if you want us to do the follow-up 😉

Member

vdemeester commented Apr 10, 2017

I am all for doing a follow-up for docs, let's get this merge.
@yongtang ping me if you want us to do the follow-up 😉

@vdemeester vdemeester merged commit 4460312 into moby:master Apr 10, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32615 has succeeded
Details
janky Jenkins build Docker-PRs 41224 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1404 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12342 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1238 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 10, 2017

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Apr 10, 2017

Member

Thanks @vdemeester. I am currently updating the docs. Will try to create a PR either late today or early tomorrow. Hopefully I could have the doc PR ready shortly today.

Member

yongtang commented Apr 10, 2017

Thanks @vdemeester. I am currently updating the docs. Will try to create a PR either late today or early tomorrow. Hopefully I could have the doc PR ready shortly today.

yongtang added a commit to yongtang/docker that referenced this pull request Apr 10, 2017

Update docs of `label` filter for `docker system prune`
This fix updates docs of `label` filter for `docker system prune`.

This fix is related to moby#30740 and moby#29999, and specifically to comment
moby#30740 (comment).

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

@yongtang yongtang deleted the yongtang:29999-prune-filter-label branch Apr 10, 2017

yongtang added a commit to yongtang/docker that referenced this pull request Apr 10, 2017

Update docs of `label` filter for `docker system prune`
This fix updates docs of `label` filter for `docker system prune`.

This fix is related to moby#30740 and moby#29999, and specifically to comment
moby#30740 (comment).

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Apr 11, 2017

Update docs of `label` filter for `docker system prune`
This fix updates docs of `label` filter for `docker system prune`.

This fix is related to moby#30740 and moby#29999, and specifically to comment
moby#30740 (comment).

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Apr 11, 2017

Update docs of `label` filter for `docker system prune`
This fix updates docs of `label` filter for `docker system prune`.

This fix is related to moby#30740 and moby#29999, and specifically to comment
moby#30740 (comment).

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30740 from yongtang/29999-prune-filter-label
Add `label` filter for `docker system prune`

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30740 from yongtang/29999-prune-filter-label
Add `label` filter for `docker system prune`

@thaJeztah thaJeztah removed the docs/revisit label May 16, 2017

andrewhsu pushed a commit to docker/docker-ce that referenced this pull request Jun 5, 2017

Update docs of `label` filter for `docker system prune`
This fix updates docs of `label` filter for `docker system prune`.

This fix is related to #30740 and #29999, and specifically to comment
moby/moby#30740 (comment).

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Upstream-commit: 40bb4a3b0e68f32b7141a8c5ddd72b7f98cc38e2
Component: cli

dnephin pushed a commit to dnephin/cli that referenced this pull request Jun 14, 2017

Update docs of `label` filter for `docker system prune`
This fix updates docs of `label` filter for `docker system prune`.

This fix is related to #30740 and #29999, and specifically to comment
moby/moby#30740 (comment).

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment