Write tar fixes by hanwen-flow · Pull Request #2261 · containers/storage

hanwen-flow · 2025-02-18T13:19:07Z

No description provided.

giuseppe · 2025-02-19T09:05:49Z

there are some tests failures that must be taken care of:

[+1128s] --- FAIL: TestTarWithMaliciousSymlinks (0.26s)
[+1128s]     archive_unix_test.go:84: /tmp/TestTarWithMaliciousSymlinks3287058553/001
[+1128s]     --- FAIL: TestTarWithMaliciousSymlinks//tmp/TestTarWithMaliciousSymlinks3287058553/001/root/safe/_host-file (0.05s)
[+1128s]         archive_unix_test.go:138: assertion failed: error is not nil: processing tar file(time="2025-02-18T13:40:42Z" level=error msg="Can't create tar archive: lstat /safe/: no such file or directory"
[+1128s]             lstat /safe/: no such file or directory): exit status 1
[+1128s]     --- FAIL: TestTarWithMaliciousSymlinks//tmp/TestTarWithMaliciousSymlinks3287058553/001/root/safe/_ (0.04s)
[+1128s]         archive_unix_test.go:138: assertion failed: error is not nil: processing tar file(time="2025-02-18T13:40:42Z" level=error msg="Can't create tar archive: lstat /safe/: no such file or directory"
[+1128s]             lstat /safe/: no such file or directory): exit status 1

hanwen-flow · 2025-02-19T09:18:06Z

there are some tests failures that must be taken care of:

The failure itself is not the problem, but rather what the right behavior should be. It seems that some errors should be ignored, but other errors maybe not. Do you have guidance?

A cop-out would be to add an TarOptions.AbortOnError and push the complexity onto the caller.

hanwen-flow · 2025-02-19T09:29:25Z

@giuseppe - let me propose the error handling commit in a separate PR, because it evidently needs more discussion.

pkg/archive/archive.go

giuseppe

LGTM

hanwen-flow · 2025-02-27T12:37:57Z

LGTM

thanks; this needs another LGTM, I think. Do you know someone who could provide this?

rhatdan · 2025-02-27T16:31:21Z

@mtrmac @nalind PTAL

mtrmac

Do we need to make TarWithOptionsTo public? c/storage attempts to have a stable API, every new function is a commitment. See also NewPatternMatcher.
This does not actually substantially improve error handling, because errors in the goroutine are only logged and don’t cause a failure in the consumer.

mtrmac · 2025-02-27T21:59:52Z

pkg/archive/archive.go

+// TarWithOptions creates an archive from the directory at `srcPath`, only including files whose relative
+// paths are included in `options.IncludeFiles` (if non-nil) or not in `options.ExcludePatterns`.
+func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) {
+	if _, err := fileutils.NewPatternMatcher(options.ExcludePatterns); err != nil {


NewPatternMatcher is not quite free; doing that twice is awkward.

If TarWithOptionsTo were private, we could easily only do it once, and pass the value into the goroutine.

Alternatively, if the pipe writer were modified to use CloseWithError (which, I, think, should definitely happen), we don’t need to specifically check here: we can just start the pipe writer goroutine, and receive the pattern matcher error through the pipe.

it's somewhat incompatible, though: the error will only be propagated once reading on the stream starts. If the caller does something like

tarStream, err := Tar(...) if err != nil { return err } dest := os.Create("file.tar") io.Copy(dest, tarStream)

then your proposed behavior would now leave around "file.tar" in case of a PatternMAtcher failure.

I implemented this. PTAL.

the error will only be propagated once reading on the stream starts

That’s a fair point. There are several ways to structure that in a private function; for the eventual public API, I don’t know.

as an aside, the pattern matcher could be O(1) in the number of patterns by rewriting

NewPatternMatcher("a", "b")

to generate the regexp '(a|b)'

pkg/archive/archive.go

mtrmac · 2025-02-27T22:09:32Z

pkg/archive/archive.go

-				logrus.Errorf("%s", err)
-				return
-			}
+	compressWriter, err := CompressStream(pipeWriter, options.Compression)


AFAICS compressWriter is ~entirely local to the pipe-writing goroutine; the reading goroutine doesn’t need to know it exists.

You mean: the writing routine (which becomes TarWithOptionsTo in this PR) should also do compression?

It is already doing the compression: compressWriter is a layer of indirection that does the compression within its Write implementation, and compressWriter.Write is ultimately called within the producer goroutine.

(After all, compressWriter is writing to pipeWriter; the caller of TarWithOptions is, most of the time, blocked on reading from pipeReader, and it has no opportunity to write to pipeWriter in the meantime.)

I'm not sure what your original point was. Can you rephrase it?

You mean: the writing routine (which becomes TarWithOptionsTo in this PR) should also do compression?

Yes. TarWithOptionsTo( options: { Compression: … } ) can be set, and it seems simpler to implement it than to specifically refuse the option (or, worse, to silently ignore it while TarWithOptions implements it).

OTOH that is similar to the PatternMatcher situation, where moving the initialization into the writing goroutine would delay error reporting.

… and would the compression interfere with your plans to use reflinks?

and would the compression interfere with your plans to use reflinks?

Compression, either in the file system, or on the tar stream obviously breaks reflink copying.

Unfortunately, the CompressStream function does more than compression; it also introduces buffering for the Uncompressed case by means of the NewWriteCloserWrapper. AFAICT, the wrapper will hide the ReadFrom method of the underlying writer.

I'm not sure why the buffering is there. The tarWriter has its own buffer.

moby/moby#7542 claims a fairly significant performance improvement, although that’s not directly attributed to exactly these lines.

I don’t see archive.Tar doing all that much buffering.

the moby PR has a bunch of anecdotal data, along with a TarUntar benchmark. The latter looks rather unrepresentative, creating 100 4 byte files in /tmp, and then repeatedly tarring them.

I made a quick test that tars a dir on a real file system. The buffering in CompressStream makes a ~10% impact when going from ext4 to ext4,

$ TARDEST=$HOME/tardest.tar TARDIR=$HOME/vc/linux go test -run TarLinux -bench TarLinux 2025/03/11 13:41:38 tarring "/home/hanwen/vc/linux" 2025/03/11 13:41:45 7.221876687s 2025/03/11 13:41:55 9.783580435s 2025/03/11 13:42:03 8.324693106s 2025/03/11 13:42:13 9.535414505s 2025/03/11 13:42:21 8.34720174s 2025/03/11 13:42:31 9.622281549s 2025/03/11 13:42:39 8.726733474s 2025/03/11 13:42:48 9.115060653s 2025/03/11 13:42:57 8.690927914s 2025/03/11 13:43:06 9.37279054s 2025/03/11 13:43:06 With 8.34720174s, without 9.535414505s PASS ok github.com/containers/storage/pkg/archive 88.748s

suprprisingly, with my local hack for zero-transport copy, the buffering (which disables zero-transport) actually makes things go faster:

$ TARDEST=$HOME/.cache/tardest.tar TARDIR=$HOME/.cache/linux go test -run TarLinux -bench TarLinux 2025/03/11 13:47:06 tarring "/home/hanwen/.cache/linux" 2025/03/11 13:47:15 9.059872899s 2025/03/11 13:47:30 14.882930582s 2025/03/11 13:47:37 6.968169459s 2025/03/11 13:47:53 16.237515011s 2025/03/11 13:48:00 7.07192259s 2025/03/11 13:48:15 15.151443769s 2025/03/11 13:48:23 7.171133439s 2025/03/11 13:48:38 15.060790794s 2025/03/11 13:48:46 8.301557066s 2025/03/11 13:49:01 14.920596331s 2025/03/11 13:49:01 With 7.171133439s, without 15.060790794s PASS ok github.com/containers/storage/pkg/archive 114.837s

for tarring larger files, zero copy does bring benefits,

$ TARDEST=$HOME/.cache/tardest.tar TARDIR=$HOME/.cache/lib64 go test -run TarLinux -bench TarLinux 2025/03/11 13:54:09 tarring "/home/hanwen/.cache/lib64" 2025/03/11 13:54:12 2.802090087s 2025/03/11 13:54:14 1.640192869s 2025/03/11 13:54:16 2.12680278s 2025/03/11 13:54:18 1.61872927s 2025/03/11 13:54:20 2.151480276s 2025/03/11 13:54:21 1.619020427s 2025/03/11 13:54:24 2.168449473s 2025/03/11 13:54:25 1.584594405s 2025/03/11 13:54:27 2.132707088s 2025/03/11 13:54:29 1.617399592s 2025/03/11 13:54:29 With 2.151480276s, without 1.61872927s

it seems I'll need a more sophisticated strategy here (using zero-copy only for large files), and right now, there is no good argument for dropping the buffering from CompressStream.

https://github.com/hanwen-flow/containers-storage/tree/benchmark

mtrmac · 2025-02-27T22:12:24Z

pkg/archive/archive.go

+// `dest`, only including files whose relative paths are included in
+// `options.IncludeFiles` (if non-nil) or not in
+// `options.ExcludePatterns`.
+func TarWithOptionsTo(dest io.WriteCloser, srcPath string, options *TarOptions) error {


Given how this function silently ignores many errors (and, it seems, the plan is to change that), I’m very uncomfortable with making it a public API at this point.

That's a fair point.

I am primarily interested in creating tar files (in the Podman Checkpoint command) without the intermediate buffering introduced by io.Pipe. How should we proceed then? IIUC, there are couple of uncontested parts:

passing errors in the writer through to the reader using CloseWithError

avoiding writes to TarOptions

and a more hairy change, which is to propagate all or some errors from tarWriter.addFile(). IIUC, you'd rather see us decide what this looks like before making a new API.

FWIW, I think it would be better to export something like archive.tarWriter. Then, some of the functionality of TarOptions could be expressed in code by callers, rather than having 16 options that interact with each other in unpredictable ways.

I think the “uncontested parts” can be merged ~immediately.

I didn’t look at the failures triggered by adding error handling ~at all yet. I understand that they matter to you (don’t they?), so they block the overall effort anyway. It think that’s the next step.

And making the API public would follow, or be a part of the “add error handling” PR.

I think it would be better to export something like archive.tarWriter.

I have ~no opinion on that in principle, I’d defer to actual c/storage maintainers.

At a high level, I’m a bit unclear on what is the point of a granular API for external consumers. archive.Tar, sure, a very simple one-shot call. archive.TarWithOptions, fine, for various tunables (although many would be pretty specific to needs of c/storage callers).

For the individual tarWriter.addFile… I’m tempted to question whether there are callers specialized enough that using just archive.TarWithOptions is insufficient, but not so much that they would want to use the standard-library archive/tar.Writer, and whether the intersection of these conditions is enough to worry about.

Then, some of the functionality of TarOptions could be expressed in code by callers

I don’t know what this means. Do you mean some other parameter-passing convention (e.g. “functional options”?) Using ordinary function parameters instead of a struct? I’d be rather opposed to the latter.

I am primarily interested in creating tar files (in the Podman Checkpoint command) without the intermediate buffering introduced by io.Pipe. I’m curious, does that have a measurable performance impact?

I understand that they matter to you (don’t they?),

The failures in the test suite (caused by ignoring errors) don't matter to me. What matters to me is that in case of any error, the tar creation fails.

they would want to use the standard-library archive/tar.Writer, and whether the intersection of these conditions is enough to worry about.

The archive/tar package falls short because it doesn't connect the file system to the Writer/Reader, handling among others

extended attributes

file types (mkdir vs mknod vs creat)

hard-links

Note that the std library archive/tar.Writer is structured as a type with methods, offering greater flexibility to callers, rather than a function with a zillion options.

I’m curious, does that have a measurable performance impact?

See containers/podman@40e49cc and containers/podman#24826 (comment)

for writing large files to BTRFS, there is an effective speedup of up to 1000x.

pkg/archive/archive.go

mtrmac

So, I think we are now at

some refactoring towards a better design — good
some, but not all, error paths now returning errors

That’s a good bit of progress.

@giuseppe PTanotherL.

pkg/archive/archive.go

mtrmac · 2025-03-10T17:38:47Z

pkg/archive/archive.go

-				logrus.Errorf("%s", err)
-				return
-			}
+	compressWriter, err := CompressStream(pipeWriter, options.Compression)


moby/moby#7542 claims a fairly significant performance improvement, although that’s not directly attributed to exactly these lines.

I don’t see archive.Tar doing all that much buffering.

mtrmac

LGTM.

hanwen-flow · 2025-03-17T13:11:09Z

ping. @giuseppe

giuseppe

/lgtm

A nil and empty slice are equivalent, except resulting JSON serialization. There is JSON serialization in applyLayerHandler(), but it's internal to the storage package and therefore not subject to compatibility promises. Signed-off-by: Han-Wen Nienhuys <hanwen@engflow.com>

Before, this was breaking the convention that options passed by reference are not mutated. In particular, TestOverlayTarUntar passes the same object to both TarWithOptions and Untar, resulting in a race detector failure. Signed-off-by: Han-Wen Nienhuys <hanwen@engflow.com>

Gives a name to the anonymous goroutine populating the archive, and makes parameter dependencies explicit, in preparation to moving it out. No functional changes. Signed-off-by: Han-Wen Nienhuys <hanwen@engflow.com>

hanwen-flow · 2025-03-17T14:35:36Z

@mtrmac , @giuseppe - PTAL, had to rebase. (I used the GH UI to rebase)

giuseppe

/lgtm

openshift-ci · 2025-03-17T14:38:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, hanwen-flow

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

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

hanwen-flow force-pushed the write-tar-fixes branch 2 times, most recently from 87b1551 to 89d00b2 Compare February 19, 2025 10:26

giuseppe reviewed Feb 19, 2025

View reviewed changes

pkg/archive/archive.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label Feb 25, 2025

hanwen-flow force-pushed the write-tar-fixes branch from 89d00b2 to f7ff038 Compare February 26, 2025 08:47

openshift-merge-robot removed the needs-rebase label Feb 26, 2025

giuseppe approved these changes Feb 26, 2025

View reviewed changes

openshift-ci bot added the approved label Feb 26, 2025

mtrmac reviewed Feb 27, 2025

View reviewed changes

hanwen-flow force-pushed the write-tar-fixes branch from f7ff038 to ff06e02 Compare March 4, 2025 09:20

mtrmac reviewed Mar 10, 2025

View reviewed changes

hanwen-flow force-pushed the write-tar-fixes branch from ff06e02 to 6e03dd5 Compare March 11, 2025 16:19

mtrmac reviewed Mar 11, 2025

View reviewed changes

giuseppe approved these changes Mar 17, 2025

View reviewed changes

openshift-ci bot assigned giuseppe Mar 17, 2025

openshift-ci bot added the lgtm label Mar 17, 2025

hanwen-flow added 3 commits March 17, 2025 14:29

archive: introduce tarWithOptionsTo goroutine

d39dba2

Gives a name to the anonymous goroutine populating the archive, and makes parameter dependencies explicit, in preparation to moving it out. No functional changes. Signed-off-by: Han-Wen Nienhuys <hanwen@engflow.com>

hanwen-flow force-pushed the write-tar-fixes branch from 6e03dd5 to d39dba2 Compare March 17, 2025 13:29

openshift-ci bot removed the lgtm label Mar 17, 2025

giuseppe approved these changes Mar 17, 2025

View reviewed changes

openshift-ci bot added the lgtm label Mar 17, 2025

openshift-merge-bot bot merged commit 11e3fbc into containers:main Mar 17, 2025
20 checks passed

mtrmac mentioned this pull request Mar 19, 2025

Improve error handling in archive creation #2289

Merged

Conversation

hanwen-flow commented Feb 18, 2025

Uh oh!

giuseppe commented Feb 19, 2025

Uh oh!

hanwen-flow commented Feb 19, 2025

Uh oh!

hanwen-flow commented Feb 19, 2025

Uh oh!

Uh oh!

giuseppe left a comment

Choose a reason for hiding this comment

Uh oh!

hanwen-flow commented Feb 27, 2025

Uh oh!

rhatdan commented Feb 27, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

hanwen-flow commented Mar 17, 2025

Uh oh!

giuseppe left a comment

Choose a reason for hiding this comment

Uh oh!