Add `--filter until=<timestamp>` for `docker container/image prune` by yongtang · Pull Request #29226 · moby/moby

yongtang · 2016-12-07T22:24:03Z

- What I did

This fix is a follow up for comment
#28535 (comment)

This fix provides --filter until=<timestamp> for docker container/image prune.

- How I did it

This fix adds --filter until=<timestamp> to docker container/image prune so that it is possible to specify a timestamp and prune those containers/images that are earlier than the timestamp.

Related docs has been updated.

- How to verify it

Several integration tests have been added to cover changes.

- Description for the changelog

Add --filter until=<timestamp> for docker container/image prune

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

This fix fixes #28497.

This fix is related to #28535.

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

yongtang · 2016-12-07T22:25:29Z

/cc @AkihiroSuda @mlaventure @thaJeztah @vdemeester

AkihiroSuda · 2016-12-08T01:12:24Z

docs/reference/commandline/container_prune.md

Can you clarify the format of <timestamp> in docs?

Thanks @AkihiroSuda. The docs has been updated.

AkihiroSuda · 2016-12-08T01:14:09Z

integration-cli/docker_cli_prune_unix_test.go

Can we have a test for --until=5s? (IMO we should allow some suffix like "ago". i. e. support --until="5s ago" as well)

See also: https://github.com/docker/docker/blob/master/api/types/time/timestamp_test.go

Added a test for --filter until=3s.

I am thinking for adding ago, this could be a separate PR because --filter until=<timestamp> always use the same format as --since and --until. Changing the format will have an impact on other parts of the code base as well.

AkihiroSuda · 2016-12-08T01:15:25Z

cli/command/system/prune.go

Can you support volumes and networks as well?

Thanks. networks has been added. Currently volumes does not have a timestamp so it is not implemented.

AkihiroSuda · 2016-12-08T01:16:10Z

docs/reference/commandline/system_prune.md

images -> images/containers ?

AkihiroSuda · 2016-12-08T01:16:51Z

docs/reference/commandline/system_prune.md

Please clarify that filter is not supported for volumes and networks? (If you are facing a difficulty in supporting them)

Networks have been added. Volumes needs a timestamp which is not there yet.

AkihiroSuda · 2016-12-08T01:18:27Z

daemon/prune.go

The code seems copy-pasted from ContainersPrune(). Let's dedup it.

yongtang · 2016-12-08T05:01:04Z

Thanks @AkihiroSuda for the review. The PR has been updated. Please take a look.

AkihiroSuda · 2016-12-12T01:14:47Z

Thank you for updating this PR, almost looks good, however I still wonder some people feel --filter until=3s is hard to understand..
#29226 (review)

PTAL @thaJeztah @mlaventure @vdemeester @cpuguy83

mlaventure

Only a nit regarding the code.

I haven't yet read the doc part.

mlaventure · 2016-12-12T15:47:03Z

cli/command/system/prune.go

Should probable be e.g. instead of i.e. since there's going to be other filters included eventually (would prevent forgetting changing the text here)

The same applies for the other prune commands

yongtang · 2016-12-12T17:14:43Z

Thanks @mlaventure. The help output in code and docs has been updated. Please take a look.

mlaventure · 2016-12-12T17:19:17Z

LGTM

I agree that --until="5s ago" would be easier to understand, but given that would require changes to other command for consistency, delaying to a separate PR makes sense.

If that works for @AkihiroSuda, I think we can move this to doc-reviews

vdemeester

Wouldn't it make more sense to have :

# all images/containers created before 10m ago
--filter until=10m
# all images/container created before that date
--filter before=2017-01-01 # <- a valid timestamp

/cc @thaJeztah @dnephin

vdemeester · 2016-12-12T18:04:12Z

integration-cli/docker_cli_prune_unix_test.go

If you rebase, you should be able to do s.d.StartWithBusybox(c) without the Assert.

vdemeester · 2016-12-12T18:04:27Z

integration-cli/docker_cli_prune_unix_test.go

mlaventure · 2016-12-13T15:27:14Z

moving back to design review given it seems we still haven't agreed on the input format 😅

thaJeztah · 2016-12-15T19:39:42Z

We should have a look if changes are needed; I noticed that docker logs --since accepts both a full date/time or relative time (docker logs --since 10m also works) see #29449

If we have the same for this option, that would be consistent, and easy to use

thaJeztah · 2016-12-15T19:42:08Z

Actually, I see it was described in this PR 😄 So I think @vdemeester's comment is addressed?

vdemeester · 2017-01-04T09:17:07Z

Moving to docs-review, /cc @thaJeztah 👼

thaJeztah

left some suggestions, and a question 😄

thaJeztah · 2017-01-04T11:00:26Z

api/swagger.yaml

Perhaps we should use <timestamp> here as well, and explain the format? Does the API take the Go format, or is this a numeric (UNIX) timestamp?

Thanks @thaJeztah. The swagger.yaml has been updated. The API takes the same format (Go, timestamp) as the cli.

thaJeztah · 2017-01-04T11:00:38Z

api/swagger.yaml

Thanks. Done. Also updated the part for /networks/prune (missed that one before).

thaJeztah · 2017-01-04T11:04:25Z

docs/reference/commandline/container_prune.md

I think images should be containers here?

Also, instead of "filter", describe what the result is; "only remove containers created before given timestamp"

thaJeztah · 2017-01-04T11:17:35Z

docs/reference/commandline/container_prune.md

I think we can use one or two examples here;

an example showing a duration string "The following example prunes all stopped containers that were created more than 30 minutes ago"

an example showing an absolute date/time

we may have to emphasize that it's looking at the creation time, not the time that the container was stopped / exited (thinking of this, I think that would actually be more useful; i.e., as a user, I may want to keep containers around that were created 10 months ago, but exited 10 minutes ago. When cleaning up containers, it's more likely I'm no longer interested in a container that exited 2 weeks ago). <-- @vdemeester WDYT?

hum good point, this is trickier than I though at first… I also think FinishedTime (the time the contanier stopped) makes sense, but I think prune could also be used to clean running containers right ?

Don't think it does;

Usage: docker container prune [OPTIONS] Remove all stopped containers Options: -f, --force Do not prompt for confirmation --help Print usage

@thaJeztah @vdemeester The docs have been updated with several examples. Please take a look.

On a separate note, I agree it probably would be more useful for containers to use "stopped/exited" time.

The only downside is that, the stopped/exited time will only be appropriate for containers. For images or networks, we will not keep records for "deleted images" or "deleted networks". So there might be some inconsistency if we use stopped/exited time for containers while at the same time use created time for images or networks.

For that I think it might make sense to keep until as created time across the board (containers, images, networks), and have another filter for stopped/exited time on containers.

Maybe something like finished_before or running_until?

I can created a separate PR if this is preferable.

@tiborvass WDYT?

@thaJeztah @vdemeester The docs have been updated with several examples. Please take a look.

On a separate note, I agree it probably would be more useful for containers to use "stopped/exited" time.

The only downside is that, the stopped/exited time will only be appropriate for containers. For images or networks, we will not keep records for "deleted images" or "deleted networks". So there might be some inconsistency if we use stopped/exited time for containers while at the same time use created time for images or networks.

For that I think it might make sense to keep until as created time across the board (containers, images, networks), and have another filter for stopped/exited time on containers.

Maybe something like finished_before or running_until?

last_use?
This could be defined for images, containers and networks.

container: last time the containers was running (i.e. last time the container stopped).

image: last time a container based on this image was running.

network: last time it had a running container in it.

I can created a separate PR if this is preferable.

Did you ever create this PR?

There's this tracking issue to keep track when an image was last "used"; #4237

thaJeztah · 2017-01-04T11:18:08Z

docs/reference/commandline/image_prune.md

Can you add one or two simple examples here as well (and explain what they do?)

thaJeztah · 2017-01-04T11:18:47Z

docs/reference/commandline/network_prune.md

Can you add some simple examples here as well?

thaJeztah · 2017-01-04T11:20:28Z

docs/reference/commandline/image_prune.md

Perhaps, instead of "filter", describe what the result is; "only remove images created before given timestamp"

thaJeztah · 2017-01-04T11:21:16Z

docs/reference/commandline/network_prune.md

Same here ("only remove ..")

thaJeztah · 2017-01-04T11:22:05Z

docs/reference/commandline/system_prune.md

"only remove containers, images, and networks created before given timestamp"

Done. Thanks.

This fix is a follow up for comment moby#28535 (comment) This fix provides `--filter until=<timestamp>` for `docker container/image prune`. This fix adds `--filter until=<timestamp>` to `docker container/image prune` so that it is possible to specify a timestamp and prune those containers/images that are earlier than the timestamp. Related docs has been updated Several integration tests have been added to cover changes. This fix fixes moby#28497. This fix is related to moby#28535. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

thaJeztah

LGTM

vdemeester

LGTM 🐯

thaJeztah · 2017-01-09T16:57:58Z

all green

Add `--filter until=<timestamp>` for `docker container/image prune`

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> (cherry picked from commit bd33a99) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Conflicts: - daemon/prune.go: missing moby#29226, moby#29963

GordonTheTurtle added the status/0-triage label Dec 7, 2016

AkihiroSuda reviewed Dec 8, 2016

View reviewed changes

AkihiroSuda added status/1-design-review and removed status/0-triage labels Dec 8, 2016

AkihiroSuda added this to the 1.14.0 milestone Dec 8, 2016

yongtang force-pushed the 28535-prune-until-follow-up branch from 8df8f9f to 5a28853 Compare December 8, 2016 04:55

mlaventure added status/2-code-review and removed status/1-design-review labels Dec 12, 2016

mlaventure suggested changes Dec 12, 2016

View reviewed changes

yongtang force-pushed the 28535-prune-until-follow-up branch from 5a28853 to 9fc49f3 Compare December 12, 2016 17:12

mlaventure approved these changes Dec 12, 2016

View reviewed changes

vdemeester suggested changes Dec 12, 2016

View reviewed changes

yongtang force-pushed the 28535-prune-until-follow-up branch from 9fc49f3 to 806ad6f Compare December 13, 2016 13:26

mlaventure added status/1-design-review and removed status/2-code-review labels Dec 13, 2016

AkihiroSuda mentioned this pull request Dec 15, 2016

docker logs --sinceminutes <ago> #29449

Closed

vdemeester added status/3-docs-review and removed status/2-code-review labels Jan 4, 2017

thaJeztah requested changes Jan 4, 2017

View reviewed changes

yongtang force-pushed the 28535-prune-until-follow-up branch from 619aaa6 to 58738cd Compare January 4, 2017 22:17

yongtang mentioned this pull request Jan 4, 2017

Add .CreatedAt placeholder for docker network ls --format #29900

Merged

thaJeztah mentioned this pull request Jan 9, 2017

Need a way to mask objects from docker system prune #29999

Closed

thaJeztah approved these changes Jan 9, 2017

View reviewed changes

vdemeester approved these changes Jan 9, 2017

View reviewed changes

vdemeester added the rebuild/windowsRS1 label Jan 9, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jan 9, 2017

vdemeester added rebuild/windowsRS1 status/4-merge and removed status/3-docs-review labels Jan 9, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jan 9, 2017

thaJeztah merged commit f1fdbec into moby:master Jan 9, 2017

yongtang deleted the 28535-prune-until-follow-up branch January 10, 2017 07:01

albers mentioned this pull request Jan 16, 2017

Add bash completion for container|image|network prune --filter until #30181

Merged

sdurrheimer mentioned this pull request Jan 20, 2017

Add zsh completion for 'docker {container,image,network,system} prune --filter #30317

Merged

mkobit mentioned this pull request Feb 15, 2017

Check release version for system_prune reference topic docker/docs#1665

Closed

AkihiroSuda mentioned this pull request Feb 17, 2017

unknown flag: --filter for system prune command #31122

Closed

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

Merge pull request moby#29226 from yongtang/28535-prune-until-follow-up

39c5af4

Add `--filter until=<timestamp>` for `docker container/image prune`

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

Merge pull request moby#29226 from yongtang/28535-prune-until-follow-up

047b905

Add `--filter until=<timestamp>` for `docker container/image prune`

thaJeztah mentioned this pull request May 16, 2017

prune commands : make sure label filters are considered docker/cli#21

Merged

thaJeztah mentioned this pull request Jul 11, 2017

Error if "until" filter is combined with "--volumes" on system prune docker/cli#311

Merged

thaJeztah mentioned this pull request Nov 26, 2019

Allow 'until' filters to accept formats as described in docs #40254

Open

kolyshkin mentioned this pull request May 2, 2020

backport: No container locks on docker ps projectatomic/docker#375

Merged

Conversation

yongtang commented Dec 7, 2016

Uh oh!

yongtang commented Dec 7, 2016

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!

yongtang commented Dec 8, 2016

Uh oh!

AkihiroSuda commented Dec 12, 2016

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yongtang commented Dec 12, 2016

Uh oh!

mlaventure commented Dec 12, 2016

Uh oh!

vdemeester 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!

mlaventure commented Dec 13, 2016

Uh oh!

thaJeztah commented Dec 15, 2016

Uh oh!

thaJeztah commented Dec 15, 2016

Uh oh!

vdemeester commented Jan 4, 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!

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!

yongtang Jan 4, 2017 •

edited

Loading