[0.15 backport] snapshot/containerd: fix wrong errdefs package import#5195
Merged
AkihiroSuda merged 1 commit intoJul 30, 2024
Merged
Conversation
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> (cherry picked from commit c6745c3) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
AkihiroSuda
approved these changes
Jul 30, 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
2 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;
(cherry picked from commit c6745c3)