-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 support for dangling filter to volumes #6756
Add support for dangling filter to volumes #6756
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
923bf90
to
39f37dc
Compare
pkg/domain/filters/volumes.go
Outdated
danglingVal := val | ||
invert := false | ||
switch strings.ToLower(danglingVal) { | ||
case "true": |
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 this be case "true", "1":
And the other be case "false", "0"
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.
Fixed
39f37dc
to
0b3cfbe
Compare
The dangling filter determine whether a volume is dangling - IE, it has no containers attached using it. Unlike our other filters, this one is a boolean - must be true or false, not arbitrary values. Signed-off-by: Matthew Heon <[email protected]>
0b3cfbe
to
d78e83f
Compare
default: | ||
return nil, errors.Errorf("%q is not a valid value for the \"dangling\" filter - must be true or false", danglingVal) | ||
} | ||
vf = append(vf, func(v *libpod.Volume) bool { |
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 docker API spec for this filter is a bit ambiguous IMO. Since it's a filter defined as an element of a map[string][]string
, it's sounds like it's possible to specify it repeatedly and the spec doesn't disallow that by mutually excluding the possible boolean values. This code does exactly that but the interesting consequence is that the way we build our filter set is exclusive, not inclusive; meaning that if we specify {"dangling": ["true", "false"]}
, we'd expect to get no results based on the code as it stands; but it's not clear if this is what a user might expect or what the API spec demands.
This actually has a more interesting consequence (that might be enlightening for if any structural changes need to be made) in the name
filter where the exclusive filtering causes us to not be able to match multiple volumes if we specify all of their names.
e.g.
sh-5.0$ podman volume ls
DRIVER VOLUME NAME
local bar
local foo
sh-5.0$ python -c 'import urllib.parse; import json; print(urllib.parse.quote(json.dumps({"name": ["foo", "bar"]})))'
%7B%22name%22%3A%20%5B%22foo%22%2C%20%22bar%22%5D%7D
sh-5.0$ curl --unix-socket /tmp/podman.sock -H "Content-Type: application/json" 'http://unixsocket/v1.40/volumes?filters=%7B%22name%22%3A%20%5B%22foo%22%2C%20%22bar%22%5D%7D' | jq
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 29 100 29 0 0 9666 0 --:--:-- --:--:-- --:--:-- 9666
{
"Volumes": [],
"Warnings": []
}
sh-5.0$ curl --unix-socket /tmp/podman.sock -H "Content-Type: application/json" 'http://unixsocket/v1.40/volumes?filters=%7B%22name%22%3A%20%5B%22foo%22%5D%7D' | jq
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 218 100 218 0 0 72666 0 --:--:-- --:--:-- --:--:-- 106k
{
"Volumes": [
{
"CreatedAt": "2020-06-25T10:29:50+10:00",
"Driver": "local",
"Labels": {},
"Mountpoint": "/home/user/.local/share/containers/storage/volumes/foo/_data",
"Name": "foo",
"Options": {},
"Scope": "local"
}
],
"Warnings": []
}
sh-5.0$ curl --unix-socket /tmp/podman.sock -H "Content-Type: application/json" 'http://unixsocket/v1.40/volumes?filters=%7B%22name%22%3A%20%5B%22bar%22%5D%7D' | jq
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 218 100 218 0 0 72666 0 --:--:-- --:--:-- --:--:-- 106k
{
"Volumes": [
{
"CreatedAt": "2020-06-25T10:29:52+10:00",
"Driver": "local",
"Labels": {},
"Mountpoint": "/home/user/.local/share/containers/storage/volumes/bar/_data",
"Name": "bar",
"Options": {},
"Scope": "local"
}
],
"Warnings": []
}
I'll make an issue to follow that up since it's not really specifically relevant to this change. In the meantime, perhaps we could amend this changeset to explode if we hit both sides of the case statement, even though the Docker API doesn't forbid that?
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 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.
We probably only want to generate one filter for each filter type, and within that filter check if the value given is any one of the user-provided inputs. Should not be a big change.
Agree that we should not allow both true and false filters to be passed; honestly, I'd expect to error if more than one dangling=
filter was passed.
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 briefly tested this against a docker on another machine and it looks like they support multiple filters and combine them inclusively. So you can (weirdly) provide dangling=["true", "false"]
to get all volumes.
LGTM |
LGTM |
/lgtm |
podman inspect is problematic because there can be naming clashes. Also, it only inspects a couple of types of objects and the docs for it didn't help discover that several more types could be inspected as well. To address both concerns, we deprecate `podman inspect` and update the docs to point to to the recommend alternatives. Issue: containers#6756 Signed-off-by: Mark Stosberg <[email protected]>
The dangling filter determine whether a volume is dangling - IE, it has no containers attached using it. Unlike our other filters, this one is a boolean - must be true or false, not arbitrary values.