Use Regexp in volume ls --filter name #14597

boaz0 · 2022-06-15T09:50:22Z

Does this PR introduce a user-facing change?

Added Regexp support to filtering volumes by name

rhatdan · 2022-06-15T10:34:34Z

This needs glob support not regex support. This also needs tests added.

You can use filepath.Match I believe.

boaz0 · 2022-06-15T10:37:42Z

@rhatdan
Oh, sorry. I will fix this and almost done with the testing.

rhatdan · 2022-06-15T10:38:49Z

Great thanks.

Luap99 · 2022-06-15T11:24:55Z

The man page says regex and the other commands use regex so this is fine.

rhatdan · 2022-06-15T12:19:26Z

Docker supports GLOB and we were looking for compatibility. We could perhaps support both, but I believe GLOB is always much more easily understood by the average computer user the regex. And working with shell makes the most sense.

Luap99 · 2022-06-15T12:41:20Z

I am pretty sure that docker uses regex not glob.

rhatdan · 2022-06-15T12:47:03Z

$ docker volume list --filter name=testvolume_*
DRIVER VOLUME NAME
local testvolume_1
local testvolume_2

A Regex would have only matched testvolume testvolume_ testvolume__ ...
IT should not match 1 and 2.

Luap99 · 2022-06-15T12:50:45Z

No, your regex matches everything which contains testvolume_ or testvolume__. What you want is this regex: ^testvolume_*$.
Just try out testvolume_[12], it will work because it uses regex not glob

rhatdan · 2022-06-15T13:01:29Z

Ok as long as that test case works then I am fine with regex.

rhatdan · 2022-06-15T13:22:32Z

My understanding of * in regex matches this description from Wikipedia But I am missing something.

* | The asterisk indicates zero or more occurrences of the preceding element. For example, ab*c matches "ac", "abc", "abbc", "abbbc", and so on.

edsantiago · 2022-06-15T15:55:14Z

@boaz0 please rebase on latest main (08f35da) to fix CI breakage

flouthoc

@boaz0 Could you please squash the commit.

openshift-ci · 2022-06-16T10:25:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: boaz0, flouthoc

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:

~~OWNERS~~ [flouthoc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

test/e2e/volume_ls_test.go

edsantiago

LGTM with a couple of nits. Nice work.

edsantiago · 2022-06-21T20:41:25Z

test/system/160-volumes.bats

+    is "$output" "${prefix}_1_${suffix}.*${prefix}_2_${suffix}$" "--filter name=${prefix}* shows ${prefix}_1 and ${prefix}_2"
+
+    for i in 1 2; do
+        run_podman volume rm ${prefix}_${i}


This squicks me out. It works, but (apparently) only because podman does some internal disambiguation. I would like this either to be changed to rm ${prefix}_${i}_${suffix} or for you to add a comment such as

# side effect: test podman volume disambiguation (unique prefix)

Woow! Good catch, I was too focus on playing with the containers names that I forgot to update this part as well! 😓
I will update this
+1 on explaining why it worked anyway.
Thanks.

edsantiago · 2022-06-21T20:44:41Z

test/system/160-volumes.bats

+    run_podman volume ls --filter name=${prefix}_1.+ --format "{{.Name}}"
+    is "$output" "${prefix}_1_${suffix}" "--filter name=${prefix}_1.+ shows only one volume"
+
+    run_podman volume ls --filter name=${prefix}* --format "{{.Name}}"


Since you're adding regex, not glob, I would like to see this changed to name=${prefix}_1* with a comment loudly stating something like "yes, the '1' is intentional, blah blah this is regex". That will hammer the point to a casual reader who might be expecting globs.

flouthoc · 2022-06-22T11:19:12Z

Failing test was a flake restarted.

edsantiago · 2022-06-22T12:49:24Z

test/system/160-volumes.bats

+
+    # The _1* is intentional as asterisk has different meaning in glob and regexp. Make sure this is regexp
+    run_podman volume ls --filter name=${prefix}_1* --format "{{.Name}}"
+    is "$output" "${prefix}_1_${suffix}" "--filter name=${prefix}_1* shows only one volume"


This is EXACTLY why I requested the change I requested: because regexps are counterintuitive to many people. _1* will (and should) match BOTH volumes, because 1* means "zero or more instances of the character 1`.

I agree, which is why I like Globs.

I don't have access to docker/moby any more, but maybe this is a good opportunity to triple-confirm that we are implementing this feature in a way that is compatible with them.

yes we are, docker uses regex and we also use regex for all other commands.
I have no idea where the glob idea comes from, anyway this is the filter line in moby: https://github.com/moby/moby/blob/c17a566b0dc5074de8ce25a88ba1d43e10667d19/api/types/filters/parse.go#L190

Yep, my bad. I accidentally copied the wrong line. I updated the expected results.

edsantiago · 2022-06-22T18:36:13Z

/lgtm
/hold

edsantiago · 2022-06-22T20:29:34Z

LGTM and CI is green; but I'd like one more set of eyeballs simply because anything using regexps is fraught with peril.

nalind · 2022-06-23T13:49:30Z

pkg/domain/filters/volumes.go

+					if err != nil {
+						return false
+					}
+					return match


I think it would be faster to use regex.Compile() to compile the expression into a new Regexp object outside of the new anonymous filter function. This would also give us the opportunity to flag any syntax errors while we're still in GenerateVolumeFilters().

The anonymous filter function that's created here would then call that Regexp object's MatchString() method instead of calling the regexp package's MatchString() function, which I expect has to compile the regex every time it's called.

It looks like libpod.Runtime.Volumes() will call each of these filter functions for every volume, and that's got to add up.

I thought about it, the problem I still need to pass the pattern which is available only inside the scope of GenerateVolumeFilters() - nameVal := val which means it will be Compiled everytime GenerateVolumeFilters() is called. Unless, I can "cache" this in a map.

Or maybe I am missing something. Lemme know. Thanks.

The anonymous function (I looked it up and it turns out I should have used the phrase "function literal") can access variables defined in GenerateVolumeFilters() so long as the function itself is within the scope where the variable exists, as described at https://go.dev/ref/spec#Function_literals.

Oh right! This will work because vfs holds anonymous functions. Yes, that'll work. Thanks for the suggestions.

nalind · 2022-06-23T13:53:19Z

test/system/160-volumes.bats

+    is "$output" "${prefix}_1_${suffix}" "--filter name=${prefix}_1.+ shows only one volume"
+
+    # The _1* is intentional as asterisk has different meaning in glob and regexp. Make sure this is regexp
+    run_podman volume ls --filter name=${prefix}_1* --format "{{.Name}}"


Should the "name=" argument be quoted here?

I'm not sure... 🤷‍♂️

//cc @edsantiago what do you think?

nalind

Two suggested changes that would let us call out invalid patterns instead of silently failing to match against them.

pkg/domain/filters/volumes.go

Signed-off-by: Boaz Shuster <[email protected]>

edsantiago · 2022-06-27T18:37:12Z

LGTM but there's a merge conflict; you need to rebase.

rhatdan · 2022-06-27T18:44:59Z

/lgtm
/hold

edsantiago · 2022-06-27T20:55:54Z

eeeek, timeouts. Right after #14685. @mheon how certain are you that you found all the deadlocks?

mheon · 2022-06-27T21:07:18Z

Could be related, but it's a very different presentation than what we saw in that bug. Have we seen this anywhere else?

edsantiago · 2022-06-27T21:34:56Z

This seems surprising, but I've double-checked a different way: cirrus-flake-grep "sending signal" finds no relevant hits.

mheon · 2022-06-27T22:29:33Z

I'm going to rerun and hope it goes away, but if we see this again I'll take a more detailed look.

edsantiago · 2022-06-28T00:30:11Z

/hold cancel

It passed on rerun, but I'm betting this is going to come back.

edsantiago · 2022-06-28T00:38:09Z

P.S. thank you @boaz for your initiative and perseverance

boaz0 requested a review from rhatdan June 15, 2022 09:50

openshift-ci bot added the release-note label Jun 15, 2022

boaz0 force-pushed the closes_14583 branch from 808b8a4 to 9d134ea Compare June 15, 2022 09:51

boaz0 force-pushed the closes_14583 branch from 9d134ea to 9e20cfd Compare June 16, 2022 07:40

flouthoc approved these changes Jun 16, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2022

boaz0 force-pushed the closes_14583 branch from 9e20cfd to 6f54fb8 Compare June 16, 2022 11:35

nalind reviewed Jun 21, 2022

View reviewed changes

test/e2e/volume_ls_test.go Show resolved Hide resolved

edsantiago reviewed Jun 21, 2022

View reviewed changes

boaz0 force-pushed the closes_14583 branch 2 times, most recently from be582bf to 5a66856 Compare June 22, 2022 10:11

boaz0 requested review from edsantiago and nalind June 22, 2022 10:11

edsantiago reviewed Jun 22, 2022

View reviewed changes

boaz0 force-pushed the closes_14583 branch from 5a66856 to ab4f091 Compare June 22, 2022 18:32

openshift-ci bot assigned edsantiago Jun 22, 2022

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 22, 2022

nalind reviewed Jun 23, 2022

View reviewed changes

pkg/domain/filters/volumes.go Outdated Show resolved Hide resolved

pkg/domain/filters/volumes.go Outdated Show resolved Hide resolved

boaz0 force-pushed the closes_14583 branch from ab4f091 to 8139637 Compare June 27, 2022 10:16

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2022

boaz0 force-pushed the closes_14583 branch from 8139637 to 3ac5f05 Compare June 27, 2022 10:25

boaz0 requested review from nalind, edsantiago and Luap99 June 27, 2022 18:30

Use Regexp in volume ls --filter name

3b10c1b

Signed-off-by: Boaz Shuster <[email protected]>

boaz0 force-pushed the closes_14583 branch from 3ac5f05 to 3b10c1b Compare June 27, 2022 18:38

openshift-ci bot assigned rhatdan Jun 27, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2022

openshift-ci bot merged commit d095053 into main Jun 28, 2022

vrothberg deleted the closes_14583 branch June 28, 2022 07:43

edsantiago mentioned this pull request Jun 28, 2022

podman wait: new timeout, possibly deadlock #14761

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023

Use Regexp in volume ls --filter name #14597

Use Regexp in volume ls --filter name #14597

Conversation

boaz0 commented Jun 15, 2022 • edited Loading

Does this PR introduce a user-facing change?

rhatdan commented Jun 15, 2022

boaz0 commented Jun 15, 2022

rhatdan commented Jun 15, 2022

Luap99 commented Jun 15, 2022 • edited Loading

rhatdan commented Jun 15, 2022

Luap99 commented Jun 15, 2022

rhatdan commented Jun 15, 2022

Luap99 commented Jun 15, 2022

rhatdan commented Jun 15, 2022

rhatdan commented Jun 15, 2022 • edited Loading

edsantiago commented Jun 15, 2022

flouthoc left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 16, 2022

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc commented Jun 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Jun 22, 2022

edsantiago commented Jun 22, 2022

Choose a reason for hiding this comment

boaz0 Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

nalind left a comment

Choose a reason for hiding this comment

edsantiago commented Jun 27, 2022

rhatdan commented Jun 27, 2022

edsantiago commented Jun 27, 2022

mheon commented Jun 27, 2022

edsantiago commented Jun 27, 2022

mheon commented Jun 27, 2022

edsantiago commented Jun 28, 2022

edsantiago commented Jun 28, 2022

boaz0 commented Jun 15, 2022 •

edited

Loading

Luap99 commented Jun 15, 2022 •

edited

Loading

rhatdan commented Jun 15, 2022 •

edited

Loading

boaz0 Jun 22, 2022 •

edited

Loading

boaz0 Jun 23, 2022 •

edited

Loading

boaz0 Jun 23, 2022 •

edited

Loading