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

Use an atomic config write strategy #1397

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

n1hility
Copy link
Member

@n1hility n1hility commented Apr 4, 2023

Part of a set of changes to resolve containers/podman#18011

Uses updated AtomicFileWriter for atomic config file writing on Linux, Mac, and Windows

Updates to containers/storage@main to pull in containers/storage#1554

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

@Luap99
Copy link
Member

Luap99 commented Apr 4, 2023

Can we use https://github.com/containers/storage/blob/main/pkg/ioutils/fswriters.go#L33 instead?

Yes lets us that please. Also I think it is good to switch the linux version over to the atomic approach as well. If we have a power loss while writing connections it can corrupt the file as well on linux.

@n1hility
Copy link
Member Author

n1hility commented Apr 4, 2023

@vrothberg @Luap99 just took a look, it needs a behavioral patch for Mac and Windows but happy to switch to it (didn't see this existed , thanks!) The issue is that on both you need syncing to occur after metadata operations (with the exception fo the rename). But it could be a patch to storage to add that.

@Luap99
Copy link
Member

Luap99 commented Apr 4, 2023

@n1hility Patching c/storage sounds good, then we just need to make sure that we use this package in the relevant places.

@n1hility
Copy link
Member Author

n1hility commented Apr 5, 2023

/hold

Updates to containers/storage@main
(contains a required fswriters PR: containers/storage#1554)

[NO NEW TESTS NEEDED]

Signed-off-by: Jason T. Greene <[email protected]>
@n1hility
Copy link
Member Author

n1hility commented Apr 6, 2023

/remove-hold

PR is now revised as discussed above

PTAL @Luap99 @vrothberg @rhatdan

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, great work, @n1hility !

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n1hility, 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:

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

@openshift-ci openshift-ci bot added the approved label Apr 7, 2023
@baude
Copy link
Member

baude commented Apr 7, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 7, 2023
@openshift-merge-robot openshift-merge-robot merged commit 6f9f3f7 into containers:main Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman-machine-default.json gets corrupted on improper Windows shutdown
5 participants