Enable retry and recover for tarfs #538

jiangliu · 2023-09-19T01:32:43Z

Enhance tarfs implementation by:

retry downloading from registry if the background downloading task fails
recover snapshot information related to tarfs on startup
remount EROFS associated with tarfs when restart
avoid redundant merge operation if the image has only one layer
auto detach loop devices when they are not used anymore

codecov · 2023-09-19T03:28:02Z

Codecov Report

Attention: Patch coverage is 14.16838% with 418 lines in your changes missing coverage. Please review.

Project coverage is 33.97%. Comparing base (5009c52) to head (e0124e7).
Report is 138 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/tarfs/tarfs.go	20.97%	255 Missing and 5 partials ⚠️
pkg/tarfs/losetup_linux.go	0.00%	105 Missing ⚠️
snapshot/snapshot.go	0.00%	45 Missing ⚠️
pkg/manager/manager.go	0.00%	7 Missing ⚠️
snapshot/process.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #538      +/-   ##
==========================================
+ Coverage   33.63%   33.97%   +0.33%     
==========================================
  Files          65       68       +3     
  Lines        8259     9352    +1093     
==========================================
+ Hits         2778     3177     +399     
- Misses       5166     5817     +651     
- Partials      315      358      +43

Files with missing lines	Coverage Δ
snapshot/process.go	`0.00% <0.00%> (ø)`
pkg/manager/manager.go	`0.00% <0.00%> (ø)`
snapshot/snapshot.go	`0.00% <0.00%> (ø)`
pkg/tarfs/losetup_linux.go	`0.00% <0.00%> (ø)`
pkg/tarfs/tarfs.go	`28.23% <20.97%> (ø)`

... and 5 files with indirect coverage changes

sctb512 · 2023-09-22T11:01:32Z

pkg/tarfs/tarfs.go

-		if lock {
-			st.mutex.Lock()
-		}
+		st.mutex.Lock()


Why lock this by default, Can we add lock and release it after calling getSnapshotStatus?

Otherwise, change getSnapshotStatus to getSnapshotStatusWithLock?

It's ensure atomicity by acquiring the lock on snapshotStatus with the Manager.mutex" locked. So renamed getSnapshotStatustogetSnapshotStatusWithLock`.

pkg/tarfs/tarfs.go

sctb512 · 2023-09-22T11:36:12Z

pkg/tarfs/losetup_linux.go

+}
+
+// AttachLoopDevice attaches a specified backing file to a loop device
+func AttachLoopDevice(backingFile string) (string, error) {


Is it enough if we use it by importing "github.com/containerd/containerd/mount"?

Some modification is needed, so coopied it here.

pkg/tarfs/tarfs.go

The field `blobTarFilePath` is only used once, and it's easy to build it on demand. So remove it from struct snapshotStatus. Signed-off-by: Jiang Liu <[email protected]>

`waitLayerReady()` alreayds ensures the layer is in ready state, so avoid redundant status check. Signed-off-by: Jiang Liu <[email protected]>

Refactor tarfs::blobProcess() for coming changes. Signed-off-by: Jiang Liu <[email protected]>

Tarfs downloads and converts layer content on background, there's no flexible way to report failure from background tasks to containerd. So retry downloading and converting tasks when preparing rw layer for container. If it still fails, failure will be reported to the rw layer preparation request. Signed-off-by: Jiang Liu <[email protected]>

Avoid redundant merge operation for images with only one layer. Signed-off-by: Jiang Liu <[email protected]>

When nydus snapshotter restarts, we need to recover information related to tarfs. Otherwise all tarfs instance will become unusable. Signed-off-by: Jiang Liu <[email protected]>

Import losetup.go from containerd, to replace go-losetup. Signed-off-by: Jiang Liu <[email protected]>

Replace go-losetup with the version from containerd, to support auto-clean unused loop devices. Signed-off-by: Jiang Liu <[email protected]>

On startup, we need to recover information for all tarfs related snapshots, and remount EROFS filesystems. Signed-off-by: Jiang Liu <[email protected]>

pkg/tarfs/tarfs_test.go

.github/workflows/ci.yml

Add unit test cases for tarfs. Signed-off-by: Jiang Liu <[email protected]>

Fix a data race condition WARNING: DATA RACE Write at 0x00c000178428 by goroutine 27: github.com/containerd/nydus-snapshotter/pkg/remote/remotes/docker.(*httpReadSeeker).Close() /home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/remote/remotes/docker/httpreadseeker.go:87 +0x57 github.com/containerd/nydus-snapshotter/pkg/tarfs.(*Manager).blobProcess.func2.1() /home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/tarfs/tarfs.go:339 +0x48 runtime.deferreturn() /opt/hostedtoolcache/go/1.20.1/x64/src/runtime/panic.go:476 +0x32 github.com/containerd/nydus-snapshotter/pkg/tarfs.(*Manager).blobProcess.func3() /home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/tarfs/tarfs.go:394 +0x71 Previous read at 0x00c000178428 by goroutine 40: github.com/containerd/nydus-snapshotter/pkg/remote/remotes/docker.(*httpReadSeeker).Read() /home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/remote/remotes/docker/httpreadseeker.go:48 +0x68 bufio.(*Reader).Read() /opt/hostedtoolcache/go/1.20.1/x64/src/bufio/bufio.go:223 +0x2c3 github.com/containerd/containerd/archive/compression.(*bufferedReader).Read() /home/runner/go/pkg/mod/github.com/containerd/[email protected]/archive/compression/compression.go:113 +0xa4 io.copyBuffer() /opt/hostedtoolcache/go/1.20.1/x64/src/io/io.go:427 +0x28d io.Copy() /opt/hostedtoolcache/go/1.20.1/x64/src/io/io.go:386 +0x88 os.genericReadFrom() /opt/hostedtoolcache/go/1.20.1/x64/src/os/file.go:161 +0x34 os.(*File).ReadFrom() /opt/hostedtoolcache/go/1.20.1/x64/src/os/file.go:155 +0x324 io.copyBuffer() /opt/hostedtoolcache/go/1.20.1/x64/src/io/io.go:413 +0x1c5 io.Copy() /opt/hostedtoolcache/go/1.20.1/x64/src/io/io.go:386 +0x84 os/exec.(*Cmd).childStdin.func1() /opt/hostedtoolcache/go/1.20.1/x64/src/os/exec/exec.go:511 +0x45 os/exec.(*Cmd).Start.func2() /opt/hostedtoolcache/go/1.20.1/x64/src/os/exec/exec.go:717 +0x42 os/exec.(*Cmd).Start.func3() /opt/hostedtoolcache/go/1.20.1/x64/src/os/exec/exec.go:729 +0x47 Goroutine 27 (running) created at: github.com/containerd/nydus-snapshotter/pkg/tarfs.(*Manager).blobProcess() /home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/tarfs/tarfs.go:393 +0x9dd github.com/containerd/nydus-snapshotter/pkg/tarfs.(*Manager).PrepareLayer() /home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/tarfs/tarfs.go:465 +0x444 github.com/containerd/nydus-snapshotter/pkg/tarfs.TestPrepareLayer() /home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/tarfs/tarfs_test.go:33 +0x188 testing.tRunner() /opt/hostedtoolcache/go/1.20.1/x64/src/testing/testing.go:1576 +0x216 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.20.1/x64/src/testing/testing.go:1629 +0x47 Goroutine 40 (finished) created at: os/exec.(*Cmd).Start() /opt/hostedtoolcache/go/1.20.1/x64/src/os/exec/exec.go:716 +0xf8e github.com/containerd/containerd/archive/compression.cmdStream() /home/runner/go/pkg/mod/github.com/containerd/[email protected]/archive/compression/compression.go:284 +0x36f github.com/containerd/containerd/archive/compression.gzipDecompress() /home/runner/go/pkg/mod/github.com/containerd/[email protected]/archive/compression/compression.go:272 +0x152 github.com/containerd/containerd/archive/compression.DecompressStream() /home/runner/go/pkg/mod/github.com/containerd/[email protected]/archive/compression/compression.go:203 +0x3e4 github.com/containerd/nydus-snapshotter/pkg/tarfs.(*Manager).blobProcess.func2() /home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/tarfs/tarfs.go:341 +0x1b1 github.com/containerd/nydus-snapshotter/pkg/tarfs.(*Manager).blobProcess.func3() /home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/tarfs/tarfs.go:394 +0x71 ================== testing.go:1446: race detected during execution of test Signed-off-by: Jiang Liu <[email protected]>

imeoer · 2023-12-11T02:03:40Z

.github/workflows/ci.yml

+      - name: Setup Nydus
+        run: |
+          # Download nydus components
+          NYDUS_VER=v$(curl --header 'authorization: Bearer ${{ secrets.GITHUB_TOKEN }}' -s "https://api.github.com/repos/dragonflyoss/nydus/releases/latest" | jq -r .tag_name | sed 's/^v//')


No auth is required when requesting the latest version.

imeoer · 2023-12-11T02:19:12Z

pkg/tarfs/tarfs.go

+		// Download and convert layer content in background.
+		// Will retry when the content is actually needed if the background process failed.
+		go func() {
+			_ = process(rc, remote)


It should also give a warning log if an error happens.

imeoer · 2023-12-11T02:25:36Z

Can we add some e2e test cases for tarfs in https://github.com/containerd/nydus-snapshotter/blob/main/integration/entrypoint.sh?

jiangliu force-pushed the tarfs-recover branch 4 times, most recently from c001e9e to d276eea Compare September 19, 2023 03:18

sctb512 reviewed Sep 22, 2023

View reviewed changes

jiangliu added 9 commits November 19, 2023 17:13

tarfs: remove blobTarFilePath from snapshotStatus

5700836

The field `blobTarFilePath` is only used once, and it's easy to build it on demand. So remove it from struct snapshotStatus. Signed-off-by: Jiang Liu <[email protected]>

tarfs: remove redundant status check

07b9a91

`waitLayerReady()` alreayds ensures the layer is in ready state, so avoid redundant status check. Signed-off-by: Jiang Liu <[email protected]>

tarfs: refactor blobProcess()

e20fb9d

Refactor tarfs::blobProcess() for coming changes. Signed-off-by: Jiang Liu <[email protected]>

tarfs: avoid redundant merge operation for images with only one layer

d5558af

Avoid redundant merge operation for images with only one layer. Signed-off-by: Jiang Liu <[email protected]>

tarfs: recover tarfs information on restart

4c198ea

When nydus snapshotter restarts, we need to recover information related to tarfs. Otherwise all tarfs instance will become unusable. Signed-off-by: Jiang Liu <[email protected]>

tarfs: import losetup.go from containerd

cbe04b3

Import losetup.go from containerd, to replace go-losetup. Signed-off-by: Jiang Liu <[email protected]>

tarfs: replace go-losetup with the version from containerd

32f44c3

Replace go-losetup with the version from containerd, to support auto-clean unused loop devices. Signed-off-by: Jiang Liu <[email protected]>

tarfs: remount EROFS for existing tarfs instances on startup

4060f01

On startup, we need to recover information for all tarfs related snapshots, and remount EROFS filesystems. Signed-off-by: Jiang Liu <[email protected]>

jiangliu force-pushed the tarfs-recover branch 10 times, most recently from 8835297 to bfc11e7 Compare November 20, 2023 01:50

ChengyuZhu6 reviewed Nov 20, 2023

View reviewed changes

pkg/tarfs/tarfs_test.go Outdated Show resolved Hide resolved

jiangliu force-pushed the tarfs-recover branch 4 times, most recently from ea0b54c to 8f31e8e Compare November 20, 2023 02:28

jiangliu force-pushed the tarfs-recover branch 24 times, most recently from 62c85f1 to cc2f9f6 Compare November 22, 2023 11:20

bergwolf reviewed Dec 5, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

jiangliu added 2 commits December 7, 2023 11:08

tarfs: add unit test cases for tarfs

54e2afb

Add unit test cases for tarfs. Signed-off-by: Jiang Liu <[email protected]>

jiangliu force-pushed the tarfs-recover branch from cc2f9f6 to e0124e7 Compare December 7, 2023 03:08

imeoer reviewed Dec 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable retry and recover for tarfs #538

Enable retry and recover for tarfs #538

jiangliu commented Sep 19, 2023

codecov bot commented Sep 19, 2023 •

edited

Loading

sctb512 Sep 22, 2023

jiangliu Nov 19, 2023

sctb512 Sep 22, 2023

jiangliu Nov 19, 2023

imeoer Dec 11, 2023

imeoer Dec 11, 2023

imeoer commented Dec 11, 2023 •

edited

Loading

Enable retry and recover for tarfs #538

Are you sure you want to change the base?

Enable retry and recover for tarfs #538

Conversation

jiangliu commented Sep 19, 2023

codecov bot commented Sep 19, 2023 • edited Loading

Codecov Report

sctb512 Sep 22, 2023

Choose a reason for hiding this comment

jiangliu Nov 19, 2023

Choose a reason for hiding this comment

sctb512 Sep 22, 2023

Choose a reason for hiding this comment

jiangliu Nov 19, 2023

Choose a reason for hiding this comment

imeoer Dec 11, 2023

Choose a reason for hiding this comment

imeoer Dec 11, 2023

Choose a reason for hiding this comment

imeoer commented Dec 11, 2023 • edited Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

imeoer commented Dec 11, 2023 •

edited

Loading