-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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 --filter until=<timestamp>
for docker container/image prune
#29226
Conversation
|
||
The currently supported filters are: | ||
|
||
* until (`<timestamp>`) - filter images created before given timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify the format of <timestamp>
in docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AkihiroSuda. The docs has been updated.
@@ -89,3 +90,51 @@ func (s *DockerDaemonSuite) TestPruneImageDangling(c *check.C) { | |||
c.Assert(err, checker.IsNil) | |||
c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), id) | |||
} | |||
|
|||
func (s *DockerSuite) TestPruneContainerUntil(c *check.C) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
for _, pruneFn := range []func(dockerCli *command.DockerCli) (uint64, string, error){ | ||
prune.RunContainerPrune, | ||
prune.RunVolumePrune, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you support volumes and networks as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. networks has been added. Currently volumes does not have a timestamp so it is not implemented.
|
||
The currently supported filters are: | ||
|
||
* until (`<timestamp>`) - filter images created before given timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
images
-> images/containers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
The filtering flag (`-f` or `--filter`) format is of "key=value". If there is more | ||
than one filter, then pass multiple flags (e.g., `--filter "foo=bar" --filter "bif=baz"`) | ||
|
||
The currently supported filters are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please clarify that filter
is not supported for volumes and networks? (If you are facing a difficulty in supporting them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Networks have been added. Volumes needs a timestamp which is not there yet.
@@ -84,6 +106,23 @@ func (daemon *Daemon) ImagesPrune(pruneFilters filters.Args) (*types.ImagesPrune | |||
} | |||
} | |||
|
|||
until := time.Time{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code seems copy-pasted from ContainersPrune()
. Let's dedup it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
8df8f9f
to
5a28853
Compare
Thanks @AkihiroSuda for the review. The PR has been updated. Please take a look. |
Thank you for updating this PR, almost looks good, however I still wonder some people feel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a nit regarding the code.
I haven't yet read the doc part.
@@ -32,6 +34,7 @@ func NewPruneCommand(dockerCli *command.DockerCli) *cobra.Command { | |||
flags := cmd.Flags() | |||
flags.BoolVarP(&opts.force, "force", "f", false, "Do not prompt for confirmation") | |||
flags.BoolVarP(&opts.all, "all", "a", false, "Remove all unused images not just dangling ones") | |||
flags.Var(&opts.filter, "filter", "Provide filter values (i.e. 'until=<timestamp>')") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
5a28853
to
9fc49f3
Compare
Thanks @mlaventure. The help output in code and docs has been updated. Please take a look. |
LGTM I agree that If that works for @AkihiroSuda, I think we can move this to doc-reviews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
|
||
func (s *DockerDaemonSuite) TestPruneImageUntil(c *check.C) { | ||
c.Assert(s.d.StartWithBusybox(), checker.IsNil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you rebase, you should be able to do s.d.StartWithBusybox(c)
without the Assert
.
} | ||
|
||
func (s *DockerDaemonSuite) TestPruneNetworkUntil(c *check.C) { | ||
c.Assert(s.d.StartWithBusybox(), checker.IsNil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
9fc49f3
to
806ad6f
Compare
moving back to design review given it seems we still haven't agreed on the input format 😅 |
We should have a look if changes are needed; I noticed that If we have the same for this option, that would be consistent, and easy to use |
Actually, I see it was described in this PR 😄 So I think @vdemeester's comment is addressed? |
Moving to docs-review, /cc @thaJeztah 👼 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some suggestions, and a question 😄
@@ -4226,6 +4226,7 @@ paths: | |||
Filters to process on the prune list, encoded as JSON (a `map[string][]string`). | |||
|
|||
Available filters: | |||
- `until=<string>` Prune containers created until this timestamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @thaJeztah. The swagger.yaml has been updated. The API takes the same format (Go, timestamp) as the cli.
@@ -4901,6 +4902,7 @@ paths: | |||
- `dangling=<boolean>` When set to `true` (or `1`), prune only | |||
unused *and* untagged images. When set to `false` | |||
(or `0`), all unused images are pruned. | |||
- `until=<string>` Prune images created until this timestamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Done. Also updated the part for /networks/prune
(missed that one before).
|
||
The currently supported filters are: | ||
|
||
* until (`<timestamp>`) - filter images created before given timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think images
should be containers
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, instead of "filter", describe what the result is; "only remove containers created before given timestamp"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
seconds (aka Unix epoch or Unix time), and the optional .nanoseconds field is a | ||
fraction of a second no more than nine digits long. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tiborvass WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 usestopped/exited
time for containers while at the same time usecreated
time for images or networks.For that I think it might make sense to keep
until
ascreated
time across the board (containers, images, networks), and have another filter forstopped/exited
time on containers.Maybe something like
finished_before
orrunning_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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's this tracking issue to keep track when an image was last "used"; #4237
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
that have elapsed since January 1, 1970 (midnight UTC/GMT), not counting leap | ||
seconds (aka Unix epoch or Unix time), and the optional .nanoseconds field is a | ||
fraction of a second no more than nine digits long. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add one or two simple examples here as well (and explain what they do?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
seconds (aka Unix epoch or Unix time), and the optional .nanoseconds field is a | ||
fraction of a second no more than nine digits long. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some simple examples here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
The currently supported filters are: | ||
|
||
* until (`<timestamp>`) - filter images created before given timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, instead of "filter", describe what the result is; "only remove images created before given timestamp"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
The currently supported filters are: | ||
|
||
* until (`<timestamp>`) - filter networks created before given timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here ("only remove ..")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
The currently supported filters are: | ||
|
||
* until (`<timestamp>`) - filter containers/images/networks created before given timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"only remove containers, images, and networks created before given timestamp"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
619aaa6
to
58738cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐯
all green |
Add `--filter until=<timestamp>` for `docker container/image prune`
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
- What I did
This fix is a follow up for comment
#28535 (comment)
This fix provides
--filter until=<timestamp>
fordocker container/image prune
.- How I did it
This fix adds
--filter until=<timestamp>
todocker 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>
fordocker 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