errdefs: implement "IsXXX" utilities using containerd/errdefs #49378
Draft
thaJeztah wants to merge 2 commits intomoby:masterfrom
Draft
errdefs: implement "IsXXX" utilities using containerd/errdefs #49378thaJeztah wants to merge 2 commits intomoby:masterfrom
thaJeztah wants to merge 2 commits intomoby:masterfrom
Conversation
thaJeztah
added
status/2-code-review
kind/refactor
Member
Author
|
I have a branch to also update uses of containerd's errdefs package, but pushing this one first to verify compatibility. |
Member
Author
|
Hm... something is not working right here; That looks to be this error that it's not matching; moby/volume/drivers/extpoint.go Lines 63 to 69 in f88304a |
Member
Author
|
Yup, that's it; it fails without type driverNotFoundError string
func (e driverNotFoundError) Error() string {
return "volume driver not found: " + string(e)
}
func (driverNotFoundError) NotFound() {}
// OpErr is the error type returned by functions in the store package. It describes
// the operation, volume name, and error.
type OpErr struct {
// Err is the error that occurred during the operation.
Err error
// Op is the operation which caused the error, such as "create", or "list".
Op string
// Name is the name of the resource being requested for this op, typically the volume name or the driver name.
Name string
// Refs is the list of references associated with the resource.
Refs []string
}
// Error satisfies the built-in error interface type.
func (e *OpErr) Error() string {
if e == nil {
return "<nil>"
}
s := e.Op
if e.Name != "" {
s = s + " " + e.Name
}
s = s + ": " + e.Err.Error()
if len(e.Refs) > 0 {
s = s + " - " + "[" + strings.Join(e.Refs, ", ") + "]"
}
return s
}
// Cause returns the error the caused this error
func (e *OpErr) Cause() error {
return e.Err
}
// Unwrap returns the error the caused this error
func (e *OpErr) Unwrap() error {
return e.Err
}
func TestDriverNotFound(t *testing.T) {
err := driverNotFoundError("test")
if !IsNotFound(err) {
t.Fatalf("expected driver not found, got %T", err)
}
err2 := &OpErr{Op: "create", Name: "notexist", Err: err}
if !IsNotFound(err2) {
t.Fatalf("expected driver not found, got %T", err2)
}
}moby/vendor/github.com/containerd/errdefs/errors.go Lines 401 to 424 in f88304a |
Member
Author
|
Integration tests: Unit tests: This one is probably flaky; |
This was referenced Feb 4, 2025
thaJeztah
force-pushed
the
c8d_errdefs
branch
from
04f9c41 to
44fffaa
Compare
thaJeztah
changed the title
Member
Author
|
This one is still failing (on Windows at least; Linux is still running) That one would be hitting this code-path, but I see that any error from failing to find the container would be wrapped in an InvalidParameter; Lines 101 to 110 in e7da72a The only other thing in that code that looks possibly incorrect is that the errors returning from the volume driver are returned as-is, which could mean that it's producing a 404 if a volume isn't found; Lines 125 to 131 in e7da72a |
Member
Author
|
These still fail on Linux as well. I'm starting to wonder if the containerd errdefs's IsXXX functions do a "deep" search for errors, so continues unwrapping (therefore |
thaJeztah
force-pushed
the
c8d_errdefs
branch
from
44fffaa to
3a6c94f
Compare
thaJeztah
commented
Feb 4, 2025
Comment on lines
+288
to
+302
| func TestIntentionallyShadowed(t *testing.T) { | ||
| var ( | ||
| rootErr = errors.New("root error") | ||
|
|
||
| // Some code resulted in a "not found" error | ||
| notFoundErr = NotFound(rootErr) | ||
|
|
||
| // But a "not found" error should be considered a "invalid parameter", | ||
| // so it's intentionally wrapped. | ||
| actualErr = InvalidParameter(notFoundErr) | ||
| ) | ||
|
|
||
| if !IsInvalidParameter(actualErr) { | ||
| t.Errorf("expected invalid parameter error, got %T", actualErr) | ||
| } | ||
| if IsNotFound(actualErr) { | ||
| t.Errorf("not found error should've been masked by an invalid parameter error, but got %T", actualErr) | ||
| } | ||
| } |
Member
Author
There was a problem hiding this comment.
Confirmed; this test passes with our errdefs, but fails with containerd's implementation 🤔
Signed-off-by: Sebastiaan van Stijn <[email protected]>
This makes sure that errors originating from containerd are detected as errdefs types without converting them. Signed-off-by: Sebastiaan van Stijn <[email protected]>
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
1 participant
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.
This makes sure that errors originating from containerd are detected as
errdefs types without converting them.
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)