Add podman volume create --ignore #16243
Conversation
cmd/podman/volumes/create.go
Outdated
| @@ -61,6 +66,17 @@ func create(cmd *cobra.Command, args []string) error { | |||
| ) | |||
| if len(args) > 0 { | |||
| createOpts.Name = args[0] | |||
|
|
|||
| if opts.Ignore { | |||
| existsResponse, err := registry.ContainerEngine().VolumeExists(context.Background(), createOpts.Name) | |||
There was a problem hiding this comment.
There is always a TOCTOU race in any call to "podman create volume", no? We can make it smaller, but nothing keeps the volume alive after registry.ContainerEngine().VolumeCreate() returns.
There was a problem hiding this comment.
I'd expect the name to be checked and assigned under a lock.
There was a problem hiding this comment.
Yes, but we return the name with the lock released so there are no guarantees that the volume of that name exists by the time podman prints the name.
There was a problem hiding this comment.
The correct way is to always create the volume and only if it fails with volume exists ignore that error.
There was a problem hiding this comment.
Agreed. And that should happen in the backend as mentioned above.
There was a problem hiding this comment.
I am changing the code to that, which will make things better. But fundamentally, if you have some process removing volumes without cooperation, there is no race-free way to use the "podman volume create" api (the cli or the rest api).
You are worrying that the ordering is:
Task A Task B
check if volume exist => yes
remove volume
return old volume name
use the returned volume name => ENOVOLUME
Here a lock around the check and create will solve this problem, but there is still this possible ordering:
Task A Task B
check if volume exist => no
create volume
return new volume name
remove volume
use the returned volume name => ENOVOLUME
And no code inside podman can ever fix that.
There was a problem hiding this comment.
That is true but what it will fix is two podman volume create --ignore name calls at the same time, both can pass the volume exists check before but only one will create it and thus the other one would fail since the error was not checked.
alexlarsson
force-pushed
the
volume-create-ignore
branch
from
75ebe92 to
d25cf50
Compare
github-actions
bot
added
the
kind/api-change
|
New version pushes check inside lock. |
There was a problem hiding this comment.
Thanks, @alexlarsson!
Can you also do the plumbing for the remote client?
- Update swagger docs plus the handler https://github.com/containers/podman/blob/main/pkg/api/server/register_volumes.go#L30
- Update the "tunnel" implementation plus the bindings behind https://github.com/containers/podman/blob/main/pkg/domain/infra/tunnel/volumes.go#L14
Once the binding type is updated you can run make generate-bindings to generate the setter.
This ignores the create request if the named volume already exists. It is very useful when scripting stuff. Signed-off-by: Alexander Larsson <[email protected]>
This way we don't have to use the `ExecCondition=podman volume exist`, which saves one process start. Signed-off-by: Alexander Larsson <[email protected]>
alexlarsson
force-pushed
the
volume-create-ignore
branch
from
d25cf50 to
b7f05ce
Compare
|
@vrothberg I added a "schema:" to the VolumeCreateOptions struct member addtiotion. But I'm not sure we actually need anything else? At least |
My bad. Most endpoints use parameters but the volume's one is sending the entire struct over the wire. |
|
Restarted the flaked test |
|
What is with this CI? In cirrus-ci it looks like all passed, but here its still waiting for one test. |
Haven't seen this specific issue but Cirrus does not seem to realize that the tests have finished and turned green. |
|
I reran the job. Almost there 🤞 |
|
Yay! all green |
|
@containers/podman-maintainers PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexlarsson, 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
This ignores the create request if the named volume already exists.
It is very useful when scripting stuff.