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.

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 podman run --timeout option #10119

Merged
merged 1 commit into from
Apr 27, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Apr 22, 2021

This option allows users to specify the maximum amount of time to run
before conmon sends the kill signal to the container.

Fixes: #6412

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2021
@@ -651,7 +651,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *ContainerCLIOpts) {
createFlags.UintVar(
&cf.StopTimeout,
stopTimeoutFlagName, containerConfig.Engine.StopTimeout,
"Timeout (in seconds) to stop a container. Default is 10",
"Timeout (in seconds) to stop a container, when calling Podman stop. Default is 10",
Copy link
Member

Choose a reason for hiding this comment

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

Timeout (in seconds) that containers stopped by user command have to exit. If exceeded, the container will be forcibly stopped via SIGKILL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

createFlags.UintVar(
&cf.Timeout,
timeoutFlagName, 0,
"Timeout (in seconds) to kill a container if it does not complete, by default container will not be stopped",
Copy link
Member

Choose a reason for hiding this comment

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

Instead of "by default..." - "A timeout of 0 means the container will not be automatically stopped"

Copy link
Member

Choose a reason for hiding this comment

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

Maybe should reword the first bit, too - "Maximum length of time a container can run. They will be stopped automatically afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -529,6 +533,10 @@ type InspectContainerHostConfig struct {
IOMaximumBandwidth uint64 `json:"IOMaximumBandwidth"`
// CgroupConf is the configuration for cgroup v2.
CgroupConf map[string]string `json:"CgroupConf"`
// Timeout is time before container is killed by conmon
Timeout uint `json:"Timeout"`
Copy link
Member

Choose a reason for hiding this comment

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

These should be in config or hostconfig, not both

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops from a previous version.

@@ -758,6 +758,20 @@ func WithStopTimeout(timeout uint) CtrCreateOption {
}
}

// WithTimeout sets the time to after initial stop signal is sent to the
Copy link
Member

Choose a reason for hiding this comment

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

I think this is wrong - "sets the maximum time a container is allowed to run"

@@ -83,6 +83,11 @@ type ContainerBasicConfig struct {
// instead.
// Optional.
StopTimeout *uint `json:"stop_timeout,omitempty"`
// Timeout is a maximum time in seconds the container will run before
// main process is sent SIGKILL.
Copy link
Member

Choose a reason for hiding this comment

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

So is this immediate SIGKILL, or does it use the normal stop logic (SIGTERM -> timeout -> SIGKILL)?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are using the conmon --timeout flag, which is defined as

   -T, --timeout Kill container after specified timeout in seconds.

Which to me means the KILL signal gets sent. I don't think we should make it more complicated then this. If users want more complicated they should do it via bash.

@rhatdan rhatdan force-pushed the timeout branch 2 times, most recently from 5322fdf to 2b2ed73 Compare April 23, 2021 12:39
This option allows users to specify the maximum amount of time to run
before conmon sends the kill signal to the container.

Fixes: containers#6412

Signed-off-by: Daniel J Walsh <[email protected]>
Maximimum time a container is allowed to run before conmon sends it the kill
signal. By default containers will run until they exit or are stopped by
`podman stop`.

Copy link
Member

Choose a reason for hiding this comment

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

We point at conmon here and a few other places in this and the other man pages, but we don't explain it's relationship with Podman anywhere. Not for this PR, but we should probably do so.

In the shorter term, the common(8) man page now exists. Can you add a a ptr to that and to the other manpages in here please?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rhatdan
Copy link
Member Author

rhatdan commented Apr 26, 2021

@containers/podman-maintainers PTAL

@umohnani8
Copy link
Member

LGTM

Copy link
Member

@ashley-cui ashley-cui 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-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2021
@openshift-merge-robot openshift-merge-robot merged commit f613a2a into containers:master Apr 27, 2021
@kblin
Copy link

kblin commented May 3, 2021

Cool, thanks a lot!

@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 Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Container Timeout
8 participants