snapshot/containerd: fix wrong errdefs package import#5194
Merged
Conversation
github-actions
Bot
added
area/dependencies
I noticed that we were importing the nydus errdefs package here, and looking at [f044e0a][1] (v0.12.0-rc1), which introduced this import, this very likely was meant to be containerd's errdefs package. The only function consumed from the package is `errdefs.IsNotFound` which at the time of the commit was not compatible with containerd's `errdefs.IsNotFound` as it was [checking for the nydus error specifically][2]. Nydus-snapshotter v0.8.0 fixed this incompatibility by aliasing the error to [containerd's `ErrNotFound`][3] and was updated through [483e877][4]. Ironically, the original commit [f044e0a][1] broke vendoring, because the Nydus errdefs was no longer vendored. This was fixed in [75dd88e][4], but failed to notice that the missing vendor was due to an incorrect import. So it looks like things were broken _twice_ in the chain of events (once because the wrong errdefs package did not match the expected error; once because the errdefs package was missing), but all of them landed in v0.12.0-rc1, so nothing broke in a release ':-) This PR; - fixes the wrong import - adds a depguard rule to prevent accidental importing of this package [1]: moby@f044e0a [2]: https://github.com/moby/buildkit/blob/f044e0a9468639559db93fe30ee826ce502ac481/vendor/github.com/containerd/nydus-snapshotter/pkg/errdefs/errors.go#L22-L33 [3]: https://github.com/moby/buildkit/blob/f044e0a9468639559db93fe30ee826ce502ac481/vendor/github.com/containerd/nydus-snapshotter/pkg/errdefs/errors.go#L22-L33 [4]: moby@483e877 [5]: moby@75dd88e Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
fix_wrong_errdefs
branch
from
734e5f5 to
c6745c3
Compare
thaJeztah
commented
Jul 27, 2024
Comment on lines
104
to
+106
| info, err := c.main.Info(ctx, dgst) | ||
| if err != nil { | ||
| if errdefs.IsNotFound(err) { | ||
| if cerrdefs.IsNotFound(err) { |
Member
Author
There was a problem hiding this comment.
I wonder if this alway works; some implementations appear to not be returning a typed error. For example;
buildkit/cache/remotecache/v1/chains.go
Lines 130 to 136 in 59a3ce5
buildkit/cache/remotecache/gha/gha.go
Lines 406 to 409 in 59a3ce5
Member
There was a problem hiding this comment.
I don't think these are the implementations for the nsFallbackStore
tonistiigi
approved these changes
Jul 27, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I noticed that we were importing the nydus errdefs package here, and looking at f044e0a9468639559db93fe30ee826ce502ac481 (v0.12.0-rc1), which introduced this import, this very likely was meant to be containerd's errdefs package.
The only function consumed from the package is
errdefs.IsNotFoundwhich at the time of the commit was not compatible with containerd'serrdefs.IsNotFoundas it was checking for the nydus error specifically.Nydus-snapshotter v0.8.0 fixed this incompatibility by aliasing the error to containerd's
ErrNotFoundand was updated through 483e87725e7fd99124e3e767143687cfd1d59a8e.Ironically, the original commit f044e0a9468639559db93fe30ee826ce502ac481 broke vendoring, because the Nydus errdefs was no longer vendored. This was fixed in 75dd88efb808edb8c91755565a4417f37b985143, but failed to notice that the missing vendor was due to an incorrect import.
So it looks like things were broken twice in the chain of events (once because the wrong errdefs package did not match the expected error; once because the errdefs package was missing), but all of them landed in v0.12.0-rc1, so nothing broke in a release ':-)
This PR;