Make podman machine stop wait for qemu to exit

[shanesmith]

After podman machine stop successfully exits the qemu process may still be running, which can cause issues, especially if an attempt is made to start the machine again.

In my trials I found the qemu process normally takes about 1s to exit after podman machine stop, which means podman machine stop && podman machine start can create problems, for example. I've also found that the qemu process could hang from time to time after an attempted stop. I'm able to reproduce this most reliably by stopping the machine while a container is running, it then takes about 90s for the qemu process to exit. This last example should be an issue on its own, but all this to say podman machine stop should only exit after qemu does.

In this PR we use the -pidfile flag when running qemu and store its path in the MachineVM config. The podman machine stop command now waits for the qemu command to exit before itself exiting. Two commits are included for a bit of separation.

A couple notes:

• The MachineVM update requires a migration. I've simply followed what was already done, but do wonder if there might be a better way.

• The podman machine stop does not have a timeout, it'll wait for qemu to exit indefinitely. Perhaps it should?

• The user could interrupt (ie: CTRL-C) a podman machine stop that's taking a while, but the qemu process will still be alive.

• We could potentially send a kill signal to the qemu process after a timeout and/or a user interrupt.

• The pidfile can be used elsewhere in the future. For example, it could inform the state of a VM, or at least prevent a podman machine start.

Fixes #14148

Does this PR introduce a user-facing change?

The `podman machine stop` command on macOS now waits for the machine to be completely stopped.

[Luap99]

I don't think you should use migration for that. Migration is used when the json unmarshal would break due breaking changes to the VM format. I don't think this is necessary here. Why not just add the new field to the existing structure?

[shanesmith]

Are you suggesting leaving the proxy pidfile path in PidFilePath and just creating a new VMPidFilePath? Definitely possible and would indeed avoid migration, but I thought the naming would be a little confusing in the future.

[Luap99]

Are you suggesting leaving the proxy pidfile path in PidFilePath and just creating a new VMPidFilePath? Definitely possible and would indeed avoid migration, but I thought the naming would be a little confusing in the future.

Yes something like this, the naming might be confusing but this can be explained in the comment.
The complex format migration logic is hard to review/test making maintenance much harder in the long run.

[shanesmith]

Sure, that makes sense. =)

One last possibility, just to throw it out there, would be to keep PidFiles (or at least a new name for the proxy one), but not do the migration and essentially use proxyPidFile = vm.PidFiles.Proxy || vm.PidFilePath. That way we get better naming for the future while still supporting older VM configurations.

[benoitf]

I'm also facing this issue as well (trying to start the machine and it says it's still running while stop command says 'stops successfully'

[shanesmith]

@Luap99 Updated the PR to remove the need for migrating the config. Notes from the new commit:

• A new MachineVM struct is not needed since the PidFiles field is no
  longer used, instead PidFilePath is kept for the proxy and a new
  VMPidFilePath is added for the VM

• Migration is no longer needed since we're reusing the existing
  MachineVM struct

• Machines created before this PR won't have the VM PID file configured,
  stopping these VMs will revert back to waiting on the state to change
  away from Running, plus an added 2s sleep to give time for the VM to
  exit and to avoid potential issues

• Machines created after this PR will have a VM PID file configured and
  stopping the machine will wait indefinitely for the VM to exit

[vrothberg]

@ashley-cui @baude PTAL

[baude]

LGTM

[baude]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, shanesmith

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 [baude]

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

[ashley-cui]

Could you squash your commits? Other than that, LGTM, thanks @shanesmith !

[rhatdan]

Remove blank line

[shanesmith]

done!

[shanesmith]

Could you squash your commits? Other than that, LGTM, thanks @shanesmith !

Squashed! =)

[ssbarnea]

I wonder if this patch also fixed the bug described on #12815 - basically I am still trying to find a way to prevent gtar from failing after I stop the machine:

/usr/local/bin/gtar: ../../../.local/share/containers/podman/machine/qemu/podman-machine-default_fedora-coreos-36.20220618.2.0-qemu.x86_64.qcow2: file changed as we read it

[rhatdan]

/lgtm
/hold
Thanks @shanesmith

[vrothberg]

/hold cancel

[shanesmith]

I wonder if this patch also fixed the bug described on #12815

@ssbarnea I would assume it should =)