chunked: use temporary file for tar-split data #2312

giuseppe · 2025-04-09T10:57:18Z

Replace the in-memory buffer with a O_TMPFILE file. This reduces the memory requirements for a partial pull since the tar-split data can be written to disk.

c/image needs the following patch:

diff --git a/storage/storage_dest.go b/storage/storage_dest.go
index d1c5acef..e66e0ccc 100644
--- a/storage/storage_dest.go
+++ b/storage/storage_dest.go
@@ -425,6 +425,7 @@ func (s *storageImageDestination) PutBlobPartial(ctx context.Context, chunkAcces
 	if err != nil {
 		return private.UploadedBlob{}, err
 	}
+	defer differ.Close()
 
 	out, err := s.imageRef.transport.store.PrepareStagedLayer(nil, differ)
 	if err != nil {

openshift-ci · 2025-04-09T10:57:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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~~ [giuseppe]

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

pkg/chunked/compression_linux.go

Copilot

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

pkg/chunked/compression_linux_test.go

pkg/chunked/compression_linux.go

liamstask

found this PR while I happened to be following a few threads related to this, and noticed that using buffered i/o would likely be worthwhile - feel free to disregard, of course!

pkg/chunked/compression_linux.go

drivers/driver.go

giuseppe · 2025-04-23T07:32:37Z

@Luap99 @mtrmac the PR is ready, PTAL

pkg/chunked/compression_linux.go

extract the blob checksum validation logic from decodeAndValidateBlob into a separate function. Signed-off-by: Giuseppe Scrivano <[email protected]>

Signed-off-by: Giuseppe Scrivano <[email protected]>

pkg/chunked/compression_linux.go

pkg/chunked/compression_linux_test.go

Replace the in-memory buffer with a O_TMPFILE file. This reduces the memory requirements for a partial pull since the tar-split data can be written to disk. Signed-off-by: Giuseppe Scrivano <[email protected]>

Replace the direct call to unix.Open with the O_TMPFILE flag with the dedicated openTmpFile helper function. Signed-off-by: Giuseppe Scrivano <[email protected]>

Luap99

LGTM

rhatdan · 2025-04-29T13:14:11Z

/lgtm

mtrmac

I’m afraid I’m way behind, just a quick note.

I’m also generally confused about the lifetime of DriverWithDifferOutput.TarSplit.

pkg/chunked/storage_linux.go

mtrmac

Super late — I’ll file a PR for the nits.

I’d love a discussion about placing the files in /run, though.

mtrmac · 2025-05-06T19:46:33Z

layers.go

@@ -2550,10 +2550,14 @@ func (r *layerStore) applyDiffFromStagingDirectory(id string, diffOutput *driver
 		if err != nil {
 			compressor = pgzip.NewWriter(&tsdata)
 		}
+		if _, err := diffOutput.TarSplit.Seek(0, 0); err != nil {


io.SeekStart, throughout

mtrmac · 2025-05-06T19:46:47Z

pkg/chunked/compression_linux.go

+// tmpDir is a directory where the tar-split temporary file is written to.  The file is opened with
+// O_TMPFILE so that it is automatically removed when it is closed.
+// Returns (manifest blob, parsed manifest, tar-split file or nil, manifest offset).  The opened tar-split file
+// points to the end of the file (equivalent to Seek(0, 2)).


io.SeekEnd

#2328 removes that instead.

mtrmac · 2025-05-06T19:52:48Z

pkg/chunked/compression_linux.go

+		return err
+	}
+
+	decoder, err := zstd.NewReader(bytes.NewReader(blob)) //nolint:contextcheck


Does the lint suppression match anything?

mtrmac · 2025-05-06T19:56:01Z

pkg/chunked/compression_linux.go

@@ -157,10 +159,33 @@ func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64,
 	return manifestUncompressed, tocOffset, nil
 }

+func openTmpFile(tmpDir string) (*os.File, error) {
+	file, err := os.OpenFile(tmpDir, unix.O_TMPFILE|unix.O_RDWR|unix.O_CLOEXEC|unix.O_EXCL, 0o600)


(Contra #2312 (comment) ) this seems to work, but AFAIK nothing promises that passing syscall.O_… flags to os.OpenFile is supported — let alone flags from a “third-party” x/sys/unix.O_….

mtrmac · 2025-05-06T20:01:15Z

pkg/chunked/storage_linux.go

@@ -333,13 +339,16 @@ func makeConvertFromRawDiffer(store storage.Store, blobDigest digest.Digest, blo
 // makeZstdChunkedDiffer sets up a chunkedDiffer for a zstd:chunked layer.
 // It may return an error matching ErrFallbackToOrdinaryLayerDownload / errFallbackCanConvert.
 func makeZstdChunkedDiffer(store storage.Store, blobSize int64, tocDigest digest.Digest, annotations map[string]string, iss ImageSourceSeekable, pullOptions pullOptions) (*chunkedDiffer, error) {
-	manifest, toc, tarSplit, tocOffset, err := readZstdChunkedManifest(iss, tocDigest, annotations)
+	manifest, toc, tarSplit, tocOffset, err := readZstdChunkedManifest(store.RunRoot(), iss, tocDigest, annotations)


RunRoot is, by default, in /run … so the net total on this code path is that it moves the uncompressed tar-split from process memory (which can be swapped out, and is limited by swap size) to a tmpfs (also limited by swap size).

It’s still a bit of an improvement, because we have a bit of a more direct control over allocation / uses / lifetime of the memory — but it does it substantially change the actual limit?

Fix nits from #2312

openshift-ci bot added the approved label Apr 9, 2025

giuseppe force-pushed the chunked-read-tarsplit-sequentially branch 2 times, most recently from cebc9ff to 13cbaef Compare April 9, 2025 12:07

Luap99 reviewed Apr 15, 2025

View reviewed changes

pkg/chunked/compression_linux.go Outdated Show resolved Hide resolved

pkg/chunked/compression_linux.go Show resolved Hide resolved

giuseppe force-pushed the chunked-read-tarsplit-sequentially branch 2 times, most recently from 94a8a23 to b0fd638 Compare April 15, 2025 12:44

flouthoc requested a review from Copilot April 15, 2025 13:57

Copilot AI reviewed Apr 15, 2025

View reviewed changes

pkg/chunked/compression_linux_test.go Show resolved Hide resolved

Luap99 reviewed Apr 15, 2025

View reviewed changes

pkg/chunked/compression_linux.go Outdated Show resolved Hide resolved

giuseppe force-pushed the chunked-read-tarsplit-sequentially branch 2 times, most recently from 4f0de4e to 6342175 Compare April 15, 2025 21:38

cgwalters reviewed Apr 15, 2025

View reviewed changes

pkg/chunked/compression_linux.go Outdated Show resolved Hide resolved

giuseppe force-pushed the chunked-read-tarsplit-sequentially branch 3 times, most recently from f04c803 to 187c56b Compare April 16, 2025 10:31

liamstask reviewed Apr 23, 2025

View reviewed changes

pkg/chunked/compression_linux.go Outdated Show resolved Hide resolved

drivers/driver.go Show resolved Hide resolved

giuseppe force-pushed the chunked-read-tarsplit-sequentially branch from 187c56b to d70254a Compare April 23, 2025 07:30

Luap99 reviewed Apr 28, 2025

View reviewed changes

pkg/chunked/compression_linux.go Show resolved Hide resolved

kolyshkin reviewed Apr 28, 2025

View reviewed changes

pkg/chunked/compression_linux.go Outdated Show resolved Hide resolved

kolyshkin reviewed Apr 28, 2025

View reviewed changes

pkg/chunked/compression_linux.go Show resolved Hide resolved

giuseppe added 2 commits April 28, 2025 22:07

chunked: extract blob validation into standalone function

8ebe960

extract the blob checksum validation logic from decodeAndValidateBlob into a separate function. Signed-off-by: Giuseppe Scrivano <[email protected]>

driver: add Close() method to Differ

3724a5a

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the chunked-read-tarsplit-sequentially branch from d70254a to 0084bc1 Compare April 28, 2025 20:40

kolyshkin reviewed Apr 28, 2025

View reviewed changes

pkg/chunked/compression_linux.go Show resolved Hide resolved

pkg/chunked/compression_linux_test.go Outdated Show resolved Hide resolved

pkg/chunked/compression_linux_test.go Outdated Show resolved Hide resolved

giuseppe force-pushed the chunked-read-tarsplit-sequentially branch 2 times, most recently from 6472d74 to dfd505c Compare April 29, 2025 10:13

giuseppe added 2 commits April 29, 2025 12:26

chunked: use temporary file for tar-split data

f5bdfdc

Replace the in-memory buffer with a O_TMPFILE file. This reduces the memory requirements for a partial pull since the tar-split data can be written to disk. Signed-off-by: Giuseppe Scrivano <[email protected]>

chunked: Refactor temporary file creation

729821c

Replace the direct call to unix.Open with the O_TMPFILE flag with the dedicated openTmpFile helper function. Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the chunked-read-tarsplit-sequentially branch from dfd505c to 729821c Compare April 29, 2025 10:27

Luap99 reviewed Apr 29, 2025

View reviewed changes

openshift-ci bot assigned rhatdan Apr 29, 2025

openshift-ci bot added the lgtm label Apr 29, 2025

openshift-merge-bot bot merged commit ff9e524 into containers:main Apr 29, 2025
20 checks passed

giuseppe mentioned this pull request Apr 29, 2025

vendor: update c/storage containers/image#2844

Merged

mtrmac reviewed Apr 29, 2025

View reviewed changes

pkg/chunked/storage_linux.go Show resolved Hide resolved

giuseppe mentioned this pull request Apr 29, 2025

fix tarsplit file leak #2323

Merged

mtrmac reviewed May 6, 2025

View reviewed changes

mtrmac mentioned this pull request May 6, 2025

Fix nits from #2312 #2328

Merged

openshift-merge-bot bot added a commit that referenced this pull request May 15, 2025

Merge pull request #2328 from mtrmac/DifferOutput-nits

78f4258

Fix nits from #2312

chunked: use temporary file for tar-split data #2312

chunked: use temporary file for tar-split data #2312

Uh oh!

Conversation

giuseppe commented Apr 9, 2025

Uh oh!

openshift-ci bot commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liamstask left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

giuseppe commented Apr 23, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Apr 29, 2025

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!