Update layer store to sync transaction files before committing

[dmcgowan]

Fixes case where shutdown occurs before content is synced to disked
on layer creation. This case can leave the layer store in an bad
state and require manual recovery. This change ensures all files
are synced to disk before a layer is committed. Any shutdown that
occurs will only cause the layer to not show up but will allow it to
be repulled or recreated without error.

Added generic io logic to ioutils package to abstract it out of
the layer store package.

This is designed to address #23184 but further testing is needed before
that issue can be marked resolved. This change will also not fix
any layer store with existing empty files.

[thaJeztah]

/cc @aaronlehmann @tonistiigi ptal

[tonistiigi]

To be more consistent with existing AtomicFileWriter maybe these methods would be better:

NewAtomicWriteSet(dir string) (*AtomicWriteSet, error) // dir is target, tempdir automatic
func (ws *AtomicWriteSet) WriteFile(filename string, data []byte, perm os.FileMode) error // no change
func (ws *AtomicWriteSet) FileWriter(name string, perm os.FileMode) (*io.WriteCloser, error) // close calls fsync
func (ws *AtomicWriteSet) Commit() error 

[mlaventure]

Windows error seems real:

06:01:32 --- FAIL: TestAtomicWriteSetCommit (0.01s)
06:01:32    fswriters_test.go:85: Mode mismatched, expected 640, got 666

[dmcgowan]

@mlaventure: ugh yes, Window's lack of support for mode makes it annoying to test. In this case 0666 gets umasked to 0644, but in Windows it will always be 0666. I will find a better solution.

@tonistiigi: I had considered using io.WriteCloser, since underlying access is not needed I can try that out. As for the target and tempdir, this was designed specifically to handle the needs of the layer store. The target directory is not known when the transaction is created, we only know the root directory, and from there we can make an appropriate tmp root directory. Because of this, we also cannot choose the tmp directory since we don't know where the target will be to safely pick a tmp on the same device.

[aaronlehmann]

As an optimization, it may make sense to leave the files open and call Sync on all of the files at Commit time before the rename.

If multiple files are written sequentially, each Sync could be expensive. But by delaying the Syncs until commit time, the first one would probably flush all the buffers, and the remaining syncs would be no-ops.

[dmcgowan]

I do agree with that optimization, was trying to avoid until I knew what the impact would be. We could get away with leaving most the files open until then, except for the tar split file which gets explicitly closed after apply.

[dmcgowan]

Updated and ci is passing again

[aaronlehmann]

Any thoughts about adding the optimization?

[dmcgowan]

@aaronlehmann: I have, right now the only way I see doing it involves making the returned close a nop, add each syncFileCloser to a list, then on Commit sync close everything. My problem with this approach is leaving the files open. While I don't think it will be a problem in our case it feels messy to do. Note the files have to stay open because fsync requires the fd.

[tonistiigi]

@dmcgowan What about syncfs(2)

[aaronlehmann]

Yeah, this was what I had in mind. I don't see a problem with leaving the files open until Commit time. I think of it as the cost you pay for doing an atomic commit.

[dmcgowan]

@tonistiigi: syncfs syncs only the single filesystem associated with an FD that is provided, that is what we want but it is also not exposed in Golang's syscall package. sync is exposed, but flushes all filesystems, don't think we want that.

@aaronlehmann: I am less convinced of the value of this optimization. While fsync operates on a single file it does not guarantee other files on the same filesystem will gets synced, such as with syncfs. It doesn't seem too problematic to just pay the individual fsync penalty on each file close in the set, AtomicFileWriter already does this. We could certainly benchmark this but feels like early optimization at the cost of leaving files open potentially longer than expected.

[tonistiigi]

LGTM. Single fsync would be nice (unless benchmarks on machines with slow disks show it doesn't have an impact) but can be a follow-up.

[cpuguy83]

@aaronlehmann Are you good with this?

[aaronlehmann]

I still feel like coalescing the fsync would be a meaningful improvement, but as Tonis says that could be a followup.

[cpuguy83]

@aaronlehmann Does that mean you give this one your blessing?

[aaronlehmann]

Yeah

[aaronlehmann]

LGTM