-
Notifications
You must be signed in to change notification settings - Fork 202
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 volume plugin timeout to containers.conf #1129
Add volume plugin timeout to containers.conf #1129
Conversation
@containers/podman-maintainers PTAL |
pkg/config/default.go
Outdated
@@ -168,6 +168,8 @@ const ( | |||
SeccompOverridePath = _etcDir + "/containers/seccomp.json" | |||
// SeccompDefaultPath defines the default seccomp path. | |||
SeccompDefaultPath = _installPrefix + "/share/containers/seccomp.json" | |||
// DefaultVolumePluginTimeout is the default volume plugin timeout, in milliseconds. | |||
DefaultVolumePluginTimeout = 5000 |
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.
Why not make this Human? I don't know Microseconds, but I do know 5s
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 people might want to do sub-1s timeouts (250/500 milliseconds seems reasonable) and I didn't want to make this a float. Probably not a big deal to do seconds instead.
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.
does 250ms work?
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.
Changed to just do seconds... Thinking about it more, it shouldn't really matter that much.
1960b1d
to
355e985
Compare
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
355e985
to
a74cc41
Compare
Tests should be going green |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lsm5, 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 |
/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.
I miss tests. One checking for the default timer and another for checking a custom value.
@mheon, could you add them in another PR? Just to be extra sure.
[v0.49] Backport #1129
[v0.48] Backport #1129
Add a new config knob for setting a default timeout for volume plugins. Just adds a field to containers.conf, real work will be in Libpod to wire it in.