Add podman machine restart subcommand#28687
Conversation
| **[podman(1)](podman.1.md)**, **[podman-machine(1)](podman-machine.1.md)** | ||
|
|
||
| ## HISTORY | ||
| March 2021, Originally compiled by Ashley Cui <[email protected]> |
| The default machine name is `podman-machine-default`. If a machine name is not specified as an argument, | ||
| then `podman-machine-default` will be restarted. | ||
|
|
||
| Stopping an already stopped vm is not considered an error so running restart on a stopped vm just |
There was a problem hiding this comment.
| Stopping an already stopped vm is not considered an error so running restart on a stopped vm just | |
| Stopping an already stopped virtual machine is not considered an error so running restart on a stopped virtual machine just |
I realize this was probably a cut/paste form elsewhere, but we say "virtual machine" everywhere else in this doc, so we should be consistant.
There was a problem hiding this comment.
yes straight from a code comment i read :) fixed
| | ssh | [podman-machine-ssh(1)](podman-machine-ssh.1.md) | SSH into a virtual machine | | ||
| | start | [podman-machine-start(1)](podman-machine-start.1.md) | Start a virtual machine | | ||
| | stop | [podman-machine-stop(1)](podman-machine-stop.1.md) | Stop a virtual machine | | ||
| | restart | [podman-machine-restart(1)](podman-machine-restart.1.md) | Restart a virtual machine | |
There was a problem hiding this comment.
This needs to be in alpha order between reset and rm
|
@jaitjacob TYVM for the PR! It looks great overall. You should add a test or two if possible, and there are a few tweaks to do to the documenation. By far the best description I've seen on a PR in a very long time! |
Honny1
left a comment
There was a problem hiding this comment.
Thanks! At first glance, the code looks good. However, the PR is missing tests. Please implement e2e tests for machine. Here is an example you can use as a reference: https://github.com/containers/podman/blob/main/pkg/machine/e2e/start_test.go. You can add a restart_test.go file for this.
|
One of the reasons I never wanted this sub-command is because there were cases were if the Podman binary did not exit, then there could be collision or zombied processes of helper functions or the various sockets and pipes being used. Are you certain that this cannot happen? |
|
in addition to tests, we might need a basic test added to podman-machine-os as well? |
| | stop | [podman-machine-stop(1)](podman-machine-stop.1.md) | Stop a virtual machine | | ||
| | restart | [podman-machine-restart(1)](podman-machine-restart.1.md) | Restart a virtual machine | | ||
|
|
||
| ## SEE ALSO |
There was a problem hiding this comment.
might want to add a reference here as well
There was a problem hiding this comment.
added ref to the command under 'See Also'
| fmt.Printf("Machine %q stopped successfully\n", vmName) | ||
| newMachineEvent(events.Stop, events.Event{Name: vmName}) | ||
|
|
||
| if err := shim.Start(mc, vmProvider, machine.StartOptions{}, nil); err != nil { |
There was a problem hiding this comment.
As I understand this, passing nil here would trigger an update system connection prompt, which I'm not sure is ideal during a restart.
There was a problem hiding this comment.
passing false for updateSystemConn bool instead.
| if err := shim.Start(mc, vmProvider, machine.StartOptions{}, nil); err != nil { | ||
| return err | ||
| } | ||
| fmt.Printf("Machine %q started successfully\n", vmName) |
There was a problem hiding this comment.
| fmt.Printf("Machine %q started successfully\n", vmName) | |
| fmt.Printf("Machine %q restarted successfully\n", vmName) |
| return err | ||
| } | ||
|
|
||
| fmt.Printf("Machine %q stopped successfully\n", vmName) |
There was a problem hiding this comment.
How about dropping this altogether and simply printing a "restarted successfully" msg?
There was a problem hiding this comment.
Shall I also extend this to emit a single newMachineEvent(events.Restart) instead of the current newMachineEvent(events.Stop) followed by newMachineEvent(events.Start)?
There was a problem hiding this comment.
i dont believe a "restart" event is valid. it's still a stop and a start. i would agree, line 48 is not needed.
There was a problem hiding this comment.
I saw a Restart enum defined in events/config.go which I assumed I could use here.
I've removed line 48.
5fb7439 to
e76d4e0
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
e76d4e0 to
1ec2c98
Compare
| return err | ||
| } | ||
|
|
||
| newMachineEvent(events.Stop, events.Event{Name: vmName}) |
There was a problem hiding this comment.
What happens when podman machine restart emits a stop event, and in parallel, I run podman machine start? I believe this is the issue @baude wanted to point out. Maybe this could be solved with a new function shim.StopThenStart.
There was a problem hiding this comment.
Thank you for the review, Jan. I do not have enough understanding to create baude's concern as a test condition. I did ask AI for help but I wasn't convinced.
podman machine restart emiting a stop event implies shim.Stop was run successfully. So the machine is now definitely in Stopped state. Before restart.go can get to shim.Start a different podman binary could send start which would then result podman machine restart to error out saying,
"Starting machine "podman-machine-default" Error: unable to start "podman-machine-default": already running.
With a shim.StopThenStart are we expecting to hold a lock on the machine until we complete the restart?
| } | ||
|
|
||
| func init() { | ||
| registry.Commands = append(registry.Commands, registry.CliCommand{ |
There was a problem hiding this comment.
I think the restart command cloud has some flags that are related to stopping and starting the machine, for example, --quiet.
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?
Fixes #28366
This PR implements
podman machine restartsubcommand.Here's how I tested it,
podmanbinary usingmake BUILDTAGS="selinux seccomp" PREFIX=/usrcd bin/./podman machine restart --help& verify help text displays correctly../podman machine list./podman machine startuptime./podman machine restartuptimeagainI also built the docs locally and viewed in browser,
