Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for passing container stop timeout as -1 (infinite) #19425

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Add support for passing container stop timeout as -1 (infinite) #19425

merged 1 commit into from
Aug 8, 2023

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 29, 2023 •
edited
Loading

Add support for podman stop --time -1
Add support for podman restart --time -1
Add support for podman rm --time -1
Add support for podman pod stop --time -1
Add support for podman pod rm --time -1
Add support for podman volume rm --time -1
Add support for podman network rm --time -1

Fixes: #17542

Does this PR introduce a user-facing change?

Compatibility API now accepts timeout=-1 for stopping containers.

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 29, 2023
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jul 29, 2023
vrothberg
vrothberg approved these changes Jul 31, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rhatdan
Copy link
Member Author

rhatdan commented Jul 31, 2023

@containers/podman-maintainers PTAL

Luap99
Luap99 reviewed Jul 31, 2023
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only work for the compat API, however I think it would make more sense to correctly support this in libpod. For example podman stop -t -1 is still not supported after that.

pkg/api/handlers/compat/containers_stop.go Outdated
@@ -43,7 +43,8 @@ func StopContainer(w http.ResponseWriter, r *http.Request) {
}
} else {
if _, found := r.URL.Query()["t"]; found {
options.Timeout = &query.DockerTimeout
timeout := uint(query.DockerTimeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this technically only cause a overflow?
I mean wait for 18446744073709551615 seems more than good enough for wait unlimited but a comment would help to make clear that we only set the timeout to some high value

@rhatdan
Copy link
Member Author

rhatdan commented Jul 31, 2023

Can not support the remote bindings in libpod without a breaking change.

@rhatdan rhatdan force-pushed the service branch 4 times, most recently from 039c549 to 997bc49 Compare August 2, 2023 13:45
@vrothberg
Copy link
Member

Can not support the remote bindings in libpod without a breaking change.

We're always free to add a new parameter to avoid a breaking change. Not ideal for purists but a pragmatic means to add features without breaking.

@rhatdan rhatdan force-pushed the service branch 2 times, most recently from 6c79d55 to 6331aa5 Compare August 3, 2023 09:28
@rhatdan rhatdan changed the title Compat api for containers/stop should take -1 value Add support for passing container stop timeout as -1 (infinite) Aug 3, 2023
@rhatdan
Add support for passing container stop timeout as -1 (infinite)
1e54539 
Compat api for containers/stop should take -1 value

Add support for `podman stop --time -1`
Add support for `podman restart --time -1`
Add support for `podman rm --time -1`
Add support for `podman pod stop --time -1`
Add support for `podman pod rm --time -1`
Add support for `podman volume rm --time -1`
Add support for `podman network rm --time -1`

Fixes: containers#17542

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Member Author

rhatdan commented Aug 7, 2023

@Luap99 @mheon @vrothberg @giuseppe @flouthoc @baude @lsm5 PTAL

flouthoc
flouthoc approved these changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Luap99
Luap99 approved these changes Aug 8, 2023
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, Luap99, rhatdan, vrothberg

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 [Luap99,flouthoc,rhatdan,vrothberg]

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

@openshift-merge-robot openshift-merge-robot merged commit 2f50d8e into containers:main Aug 8, 2023
This was referenced Aug 23, 2023
Merged
Closed
@github-actions 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 Nov 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg approved these changes

@Luap99 Luap99 Luap99 approved these changes

@flouthoc flouthoc flouthoc approved these changes

Assignees

@Luap99 Luap99

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Docker compatible /containers/<id or name>/stop does not support negative timeouts
5 participants
@rhatdan @vrothberg @flouthoc @Luap99 @openshift-merge-robot