Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 15, 2024

Conversation

ChengyuZhu6
Copy link
Contributor

Fixes: #592

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

cc @imeoer

Copy link

codecov bot commented Apr 28, 2024

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

Impacted file tree graph

@@            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
Copy link
Collaborator

imeoer commented Apr 28, 2024

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

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

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
Copy link
Contributor

@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
Copy link

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!

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a necessary log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infof -> Warnf ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

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

proxyhandler := func() (bool, []mount.Mount, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better be proxyhandler -> proxyHandler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

"github.com/containerd/nydus-snapshotter/config"
"github.com/containerd/nydus-snapshotter/config/daemonconfig"
rafs "github.com/containerd/nydus-snapshotter/pkg/rafs"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is an alias necessary?

@@ -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")
Copy link
Collaborator

@imeoer imeoer May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also print the detail reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

FsDriver: config.GetFsDriver(),
ImageID: "",
SnapshotID: "",
SnapshotDir: "",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ChengyuZhu6 ChengyuZhu6 force-pushed the fix-error branch 4 times, most recently from 38d0d9f to fa24178 Compare May 14, 2024 07:24
@ChengyuZhu6 ChengyuZhu6 force-pushed the fix-error branch 2 times, most recently from a27f916 to 6e06754 Compare May 14, 2024 07:49
@@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about change the return type to bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ChengyuZhu6 ChengyuZhu6 force-pushed the fix-error branch 3 times, most recently from b5b72c5 to b01f842 Compare May 15, 2024 02:10
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>
Copy link
Collaborator

@imeoer imeoer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix!

@imeoer imeoer merged commit e9d1bb7 into containerd:main May 15, 2024
16 checks passed
@ChengyuZhu6 ChengyuZhu6 deleted the fix-error branch May 15, 2024 02:28
stevenhorsman added a commit to stevenhorsman/cc-operator that referenced this pull request May 15, 2024
- 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>
fidencio pushed a commit to confidential-containers/operator that referenced this pull request May 15, 2024
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

snapshots: Fix error when switching different snapshotters
4 participants