machine config: make write atomic

[vrothberg]

As indicated in #21849, loading the machine config can flake/fail with an EOF JSON error indicating an incomplete file. Address the issue by atomically writing the config. This way, it is not possible to load an incomplete or partially written file. The lock can be acquired later on to sync state.

[NO NEW TESTS NEEDED] as it's a hard-to-hit race.

Fixes: #21849

Does this PR introduce a user-facing change?

Fix a race condition in podman machine to avoid loading an incomplete config file.

[packit-as-a-service]

Cockpit tests failed for commit f8abd7f. @martinpitt, @jelly, @mvollmer please check.

[giuseppe]

LGTM

[Luap99]

LGTM
This patch is correct and useful to prevent writing corrupted files in case a process gets killed mid write.
However the reason we see this flake is that there are fundamental locking issues in the code that still need to get fixed.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99, 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,giuseppe,vrothberg]

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

[vrothberg]

However the reason we see this flake is that there are fundamental locking issues in the code that still need to get fixed.

Can you elaborate further? What I noticed is that loading is done without having the lock. Whether that's an issue or not depends on the consistency model which I am not totally sure of.

[Luap99]

However the reason we see this flake is that there are fundamental locking issues in the code that still need to get fixed.

Can you elaborate further? What I noticed is that loading is done without having the lock. Whether that's an issue or not depends on the consistency model which I am not totally sure of.

Reading without the lock held is always broken. How can we know the data is still accurate if we do not hold the lock?
The logic currently reads the file (unlocked) then at some point later takes the lock then acts on data that was read before taking the lock, how do we know the VM is still in the state we think it is? When we take a lock we have to re-read the config file otherwise we may try to do the wrong thing. If someone runs start in parallel we may end up trying to start the VM twice which would be very bad.

[vrothberg]

Reading without the lock held is always broken.

That depends on the consistency model. As long as writes are atomic, it is valid to read without holding a lock as long as the data is not considered to be accurate.

How can we know the data is still accurate if we do not hold the lock?
The logic currently reads the file (unlocked) then at some point later takes the lock then acts on data that was read before taking the lock, how do we know the VM is still in the state we think it is?

For querying the state, QEMU does a Refresh() which will re-read the file while holding a lock.

[Luap99]

Reading without the lock held is always broken.

That depends on the consistency model. As long as writes are atomic, it is valid to read without holding a lock as long as the data is not considered to be accurate.

There is no consistency model here, yes you can read the file sure. But the data you have must be consider incorrect.

How can we know the data is still accurate if we do not hold the lock?
The logic currently reads the file (unlocked) then at some point later takes the lock then acts on data that was read before taking the lock, how do we know the VM is still in the state we think it is?

For querying the state, QEMU does a Refresh() which will re-read the file while holding a lock.

Only QEMU does that and Refresh is unlocked (unless a caller locked it) which is the reason we the the flake in the first place because the ls quires the State() which is also unlocked. For something like podman machine ls that is fine I agree and this patch will fix that.

However podman machine start is broken, it can easily start two VMS if you call it at the same time.

Also the only provider using Refresh() at all is qemu which means the other ones are even worse.

Anyhow no point in arguing this here, #21854 should fix most of the issues I think.

[vrothberg]

Good to merge?

[Luap99]

/lgtm