solver: mark history and graph concistency errors as internal #5163
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| package errdefs | ||
|
|
||
| import "errors" | ||
|
|
||
| type internalErr struct { | ||
| error | ||
| } | ||
|
|
||
| func (internalErr) System() {} | ||
|
|
||
| func (err internalErr) Unwrap() error { | ||
| return err.error | ||
| } | ||
|
|
||
| type system interface { | ||
| System() | ||
| } | ||
|
|
||
| var _ system = internalErr{} | ||
|
|
||
| func Internal(err error) error { | ||
|
There was a problem hiding this comment. The separate function is mostly to avoid direct That's also why this isn't part of solver/errdefs for example. |
||
| if err == nil { | ||
| return nil | ||
| } | ||
| return internalErr{err} | ||
| } | ||
|
|
||
| func IsInternal(err error) bool { | ||
|
There was a problem hiding this comment. This matches the interface definition and behavior of https://github.com/containerd/errdefs/blob/124d0dc9f0cd27578004a3f14e01ef8a3456d147/errors.go#L304 (unreleased, not part of the v1). In the future containerd |
||
| var s system | ||
| return errors.As(err, &s) | ||
|
There was a problem hiding this comment. Could this just be There was a problem hiding this comment. No, If you mean adding
No. Err needs to be assignable to system. System does not need to be assignable to error. There was a problem hiding this comment. Ok, I see. Typically, I would expect the following to be true for an internal error: We'd likely need to have I guess it would have to be something like this: type causal struct {
err error
cause error
}
func (c causal) Error() string {
return fmt.Sprintf("%v: %v", c.err, c.cause)
}
func (c causal) Unwrap() []error {
errs := [2]error{c.err, c.cause}
return errs[:]
}
type errInternal struct{}
var ErrInternal = errInternal{}
func (errInternal) Error() string { return "internal error" }
func WithCause(err, cause error) error {
return causal{
err: err,
cause: cause,
}
}
func (errInternal) System() {}
type system interface {
System()
Error() string
}
func main() {
var s system
err := WithCause(ErrInternal, fmt.Errorf("the disk is done or something"))
fmt.Println(errors.As(err, &s))
fmt.Println(errors.Is(err, ErrInternal))
}I don't think we need to block the PR on this. There was a problem hiding this comment. The main difference is still that for |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add a pointer in the error structs in the containerd package?
The other option would be have these as a sibling in some sort of causal wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not fully understand the question, but this error type will return correctly in the current containerd
IsInternalfunction.Iiuc there is a issue that this
customMessagehttps://github.com/containerd/errdefs/blob/124d0dc9f0cd27578004a3f14e01ef8a3456d147/errors.go#L396 does not have aUnwrapmethod but otherwise the customIsInterfacethere looks equivalent toerrors.As()in here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, all of the errors in errdefs don't have an internal pointer to a cause: https://github.com/containerd/errdefs/blob/124d0dc9f0cd27578004a3f14e01ef8a3456d147/errors.go#L40. I think we are good with the matching interface, but classically, errdefs represent the end of the line for the error chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's a common case for internal actually. End error is likely a syscall error/parse error etc. (or if containerd loses the syscall error then the containerd API error). If containerd, for example, says
blob not found,it could be a user error if the blob is wrong in manifest or wrong user input, but internal if it is during history addition, for example. Eg. in this PR, all the errors that come wrong writing history are automatically internal errors (they could be errors from blobstore, bolt, leases etc) because buildkit guarantees that it can save history on any build, no matter if the user asks for something broken. Same for syscall,lstat(user-copy-path)is user error, butlstat(boltdb)is internal.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed. Most internally errors will be unhandled errors that bubble up.