Make restart parallel and add --all #1738
Conversation
cmd/podman/restart.go
Outdated
| Usage: "restart all non-running containers", | ||
| }, | ||
| cli.BoolFlag{ | ||
| Name: "running-only", |
There was a problem hiding this comment.
Change option to running. I have options with "-" in them.
|
What happens if I specify --running without --all? |
I spit an error out that says --running is only valid with all. I would be open to treat --running as if it implied --all. Meaning, you could do restart --running and all running containers would get a restart. And also honor restart --running --all in the same way. WDYT? I kind of like that idea. |
|
Yes I would guess --running would imply --all or conflict with --all. I don't think you should need to specify both. |
|
ok, lets do that, and I will make the option name change too! |
|
Options change SGTM |
docs/podman-restart.1.md
Outdated
| Instead of providing the container name or ID, use the last created container. If you use methods other than Podman | ||
| to run containers such as CRI-O, the last started container could be from either of those methods. | ||
|
|
||
| **--running-only** |
There was a problem hiding this comment.
This is just "--running" now me thinks.
docs/podman-restart.1.md
Outdated
| Restart all containers that are already in the *running* state. | ||
|
|
||
| **--timeout** | ||
| Timeout to wait before forcibly stopping the container |
There was a problem hiding this comment.
I'm not sure, if --timeout can be used with --all or --running, should we change to "stopping a container" as there might be multiples and I'm assuming the timeout would apply individually?
There was a problem hiding this comment.
OK last pick, "Timeout in seconds"
|
Couple man page nits, otherwise LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude 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-robot
added
the
approved
cmd/podman/restart.go
Outdated
| } | ||
| lastError = errors.Wrapf(err, "error restarting container %s", ctr.ID()) | ||
| f := func() error { | ||
| return con.RestartWithTimeout(context.TODO(), ctrTimeout) |
There was a problem hiding this comment.
This should probably be getContext() instead of context.TODO()
When attempting to restart many containers, we can benefit from making the restarts parallel. For convenience, two new options are added: --all attempts to restart all containers --run-only when used with --all will attempt to restart only running containers Signed-off-by: baude <bbaude@redhat.com>
|
LGTM. @rhatdan @TomSweeneyRedHat @umohnani8 PTAL |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
When attempting to restart many containers, we can benefit from making
the restarts parallel. For convenience, two new options are added:
--all attempts to restart all containers
--run-only when used with --all will attempt to restart only running containers
Signed-off-by: baude bbaude@redhat.com