snapshot: fix error on proxy driver when switching different snapshotter #593

ChengyuZhu6 · 2024-04-28T08:10:29Z

Fixes: #592

ChengyuZhu6 · 2024-04-28T09:04:00Z

cc @imeoer

codecov · 2024-04-28T09:07:13Z

Codecov Report

Attention: Patch coverage is 0% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 34.36%. Comparing base (b8ffddf) to head (8b98d6a).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
- Coverage   34.64%   34.36%   -0.29%     
==========================================
  Files          65       65              
  Lines        6552     6606      +54     
==========================================
  Hits         2270     2270              
- Misses       3967     4021      +54     
  Partials      315      315

Files	Coverage Δ
snapshot/process.go	`0.00% <0.00%> (ø)`
snapshot/snapshot.go	`0.00% <0.00%> (ø)`

imeoer · 2024-04-28T13:18:03Z

@ChengyuZhu6 Thanks! I'll try to reproduce the bug and test the PR.

fidencio · 2024-05-10T08:43:43Z

This does work around the problem we've encountered when using guest-pull with Confidential Containers. More than working around the issue, it gives us enough time to work on containerd (which has a pace of release and adoption that can be slow to reach all the CSPs) fix without worrying much about which version of containerd will be used with Confidential Containers.

Huge thumbs up for having this one in!

fidencio · 2024-05-10T08:44:45Z

@ChengyuZhu6, I'd love to see, as part of the commit message, as much information as you provided in the issue.
This will help us, later on, to figure out why those changes were made without having to come back to GitHub to do so.

stevenhorsman · 2024-05-10T10:33:28Z

FYI - I've also testing this code and quay.io/chengyu_zhu/nydus-snapshotter@sha256:4b5e333ecd27d7b630cbe42fe686f8b3c38df7727c99f4b5cef724be6f3926fa with peer pods and it seemed to work without the workaround of cleaning up the images and content on the node. Amazing work!

imeoer · 2024-05-14T06:10:18Z

snapshot/process.go

@@ -57,6 +57,12 @@ func chooseProcessor(ctx context.Context, logger *logrus.Entry,
 		}
 	}

+	proxyhandler := func() (bool, []mount.Mount, error) {
+		logger.Infof("Nydus proxy handler")


Is this a necessary log?

imeoer · 2024-05-14T06:10:55Z

snapshot/process.go

@@ -118,6 +124,10 @@ func chooseProcessor(ctx context.Context, logger *logrus.Entry,
 		// It should not be committed during this Prepare() operation.

 		pID, pInfo, _, pErr := snapshot.GetSnapshotInfo(ctx, sn.ms, parent)
+		if checkErr := checkLabelsWithDriver(pInfo.Labels); checkErr != nil {
+			logger.Infof("not found proxy label: %v, err = %v", pInfo.Labels, checkErr)


Infof -> Warnf ?

imeoer · 2024-05-14T06:11:53Z

snapshot/process.go

@@ -57,6 +57,12 @@ func chooseProcessor(ctx context.Context, logger *logrus.Entry,
 		}
 	}

+	proxyhandler := func() (bool, []mount.Mount, error) {


Better be proxyhandler -> proxyHandler.

imeoer · 2024-05-14T06:12:36Z

snapshot/snapshot.go

 	"github.com/containerd/nydus-snapshotter/config"
 	"github.com/containerd/nydus-snapshotter/config/daemonconfig"
+	rafs "github.com/containerd/nydus-snapshotter/pkg/rafs"


Is an alias necessary?

imeoer · 2024-05-14T06:16:26Z

snapshot/snapshot.go

@@ -432,6 +432,11 @@ func (o *snapshotter) Mounts(ctx context.Context, key string) ([]mount.Mount, er
 		return nil, errors.Wrapf(err, "get snapshot %s", key)
 	}

+	if checkErr := checkLabelsWithDriver(info.Labels); checkErr != nil {
+		log.L.Debug("[Mounts] not found proxy label")


Maybe also print the detail reason.

imeoer · 2024-05-14T06:29:55Z

snapshot/snapshot.go

@@ -1011,3 +1063,18 @@ func (o *snapshotter) snapshotRoot() string {
 func (o *snapshotter) snapshotDir(id string) string {
 	return filepath.Join(o.snapshotRoot(), id)
 }
+
+func checkLabelsWithDriver(labels map[string]string) error {


How about func treatAsProxyDriver(labels map[string]string) bool ?

imeoer · 2024-05-14T06:36:32Z

snapshot/snapshot.go

+		FsDriver:    config.GetFsDriver(),
+		ImageID:     "",
+		SnapshotID:  "",
+		SnapshotDir: "",


It seems that FsDriver, ImageID, SnapshotID, SnapshotDir can be omitted.

I have removed ImageID, SnapshotID, SnapshotDir, but we still need FsDriver and annotations.

imeoer · 2024-05-14T09:01:07Z

snapshot/snapshot.go

@@ -1011,3 +1060,18 @@ func (o *snapshotter) snapshotRoot() string {
 func (o *snapshotter) snapshotDir(id string) string {
 	return filepath.Join(o.snapshotRoot(), id)
 }
+
+func treatAsProxyDriver(labels map[string]string) error {


How about change the return type to bool?

imeoer · 2024-05-14T09:05:42Z

snapshot/process.go

@@ -118,6 +123,10 @@ func chooseProcessor(ctx context.Context, logger *logrus.Entry,
 		// It should not be committed during this Prepare() operation.

 		pID, pInfo, _, pErr := snapshot.GetSnapshotInfo(ctx, sn.ms, parent)
+		if checkErr := treatAsProxyDriver(pInfo.Labels); checkErr != nil {
+			logger.Warnf("not found proxy label: %v, err = %v", pInfo.Labels, checkErr)


Maybe we can print a clarify message like treat as proxy mode for the prepared snapshot by other snapshotter possibly: id = %s, labels = %v.

imeoer · 2024-05-14T09:06:01Z

snapshot/snapshot.go

@@ -432,6 +432,11 @@ func (o *snapshotter) Mounts(ctx context.Context, key string) ([]mount.Mount, er
 		return nil, errors.Wrapf(err, "get snapshot %s", key)
 	}

+	if checkErr := treatAsProxyDriver(info.Labels); checkErr != nil {
+		log.L.Debugf("[Mounts] not found proxy label, err = %v", checkErr)


During image pull, the containerd client calls Prepare API with the label containerd.io/snapshot.ref. When an image is pulled by other snapshotter, containerd doesn't send the label "containerd.io/snapshot.ref" to nydus snapshotter to let snapshotter prepare ro layers. Consequently, the code logic in nydus snapshotter cannot find label "containerd.io/snapshot/nydus-proxy-mode" and the logic of guest pull (proxy) is skipped. This occurs while reading the label data of parent snapshots(ro layers) during the preparation of the active snapshot(rw layer). Thus, when the snapshotter driver is configured to proxy mode, nydus snapshotter is compelled to implement the proxy logic. Fixes: containerd#592 Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>

imeoer

LGTM, thanks for the fix!

- Bump the nydus snapshotter version to v0.13.13, in order to pick the nydus-snapshotter caching workaround containerd/nydus-snapshotter#593 Signed-off-by: stevenhorsman <steven@uk.ibm.com>

ChengyuZhu6 force-pushed the fix-error branch 3 times, most recently from b9cb808 to c1fbf56 Compare April 28, 2024 09:01

ChengyuZhu6 force-pushed the fix-error branch from c1fbf56 to 5d0c74e Compare April 28, 2024 09:09

ChengyuZhu6 force-pushed the fix-error branch 10 times, most recently from 76c8880 to 437002b Compare May 10, 2024 03:17

mkulke mentioned this pull request May 10, 2024

Switch to use kata-containers main branch confidential-containers/cloud-api-adaptor#1754

Merged

ChengyuZhu6 force-pushed the fix-error branch 4 times, most recently from e14a326 to 115a8ac Compare May 11, 2024 06:41

imeoer reviewed May 14, 2024

View reviewed changes

ChengyuZhu6 force-pushed the fix-error branch 4 times, most recently from 38d0d9f to fa24178 Compare May 14, 2024 07:24

ChengyuZhu6 force-pushed the fix-error branch 2 times, most recently from a27f916 to 6e06754 Compare May 14, 2024 07:49

imeoer reviewed May 14, 2024

View reviewed changes

ChengyuZhu6 force-pushed the fix-error branch 3 times, most recently from b5b72c5 to b01f842 Compare May 15, 2024 02:10

ChengyuZhu6 force-pushed the fix-error branch from b01f842 to 8b98d6a Compare May 15, 2024 02:13

imeoer approved these changes May 15, 2024

View reviewed changes

imeoer merged commit e9d1bb7 into containerd:main May 15, 2024
16 checks passed

ChengyuZhu6 deleted the fix-error branch May 15, 2024 02:28

stevenhorsman mentioned this pull request May 15, 2024

pre-reqs: Update nydus snapshotter version confidential-containers/operator#378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snapshot: fix error on proxy driver when switching different snapshotter #593

snapshot: fix error on proxy driver when switching different snapshotter #593

ChengyuZhu6 commented Apr 28, 2024

ChengyuZhu6 commented Apr 28, 2024

codecov bot commented Apr 28, 2024 •

edited

Loading

imeoer commented Apr 28, 2024

fidencio commented May 10, 2024

fidencio commented May 10, 2024

stevenhorsman commented May 10, 2024

imeoer May 14, 2024

ChengyuZhu6 May 14, 2024

imeoer May 14, 2024

ChengyuZhu6 May 14, 2024

imeoer May 14, 2024

ChengyuZhu6 May 14, 2024

imeoer May 14, 2024

imeoer May 14, 2024 •

edited

Loading

ChengyuZhu6 May 14, 2024

imeoer May 14, 2024

ChengyuZhu6 May 14, 2024

imeoer May 14, 2024

ChengyuZhu6 May 14, 2024

ChengyuZhu6 May 14, 2024

imeoer May 14, 2024

ChengyuZhu6 May 15, 2024

imeoer May 14, 2024

ChengyuZhu6 May 15, 2024

imeoer May 14, 2024

ChengyuZhu6 May 15, 2024

imeoer left a comment

snapshot: fix error on proxy driver when switching different snapshotter #593

snapshot: fix error on proxy driver when switching different snapshotter #593

Conversation

ChengyuZhu6 commented Apr 28, 2024

ChengyuZhu6 commented Apr 28, 2024

codecov bot commented Apr 28, 2024 • edited Loading

Codecov Report

imeoer commented Apr 28, 2024

fidencio commented May 10, 2024

fidencio commented May 10, 2024

stevenhorsman commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imeoer May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imeoer left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2024 •

edited

Loading

imeoer May 14, 2024 •

edited

Loading