solver: mark history and graph concistency errors as internal

[tonistiigi]

Error during creating history or failure in graph concistency checks are signs of either bugs or system configuration issue. This makes sure that gRPC error code in the API error based on these cases has correct value to signify it.

This doesn't of course cover all the possible cases where error return from BuildKit could be considered internal, but can be used as a proof of concept.

[tonistiigi]

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 IsInternal() function can be used instead in the caller side for equivalent behavior.

[tonistiigi]

The separate function is mostly to avoid direct grpcerrors dependency in packages that don't interact with grpc. Otherwise WrapCode() is the generic version where any code can be passed directly.

That's also why this isn't part of solver/errdefs for example.

[stevvooe]

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.

[tonistiigi]

Did not fully understand the question, but this error type will return correctly in the current containerd IsInternal function.

Iiuc there is a issue that this customMessage https://github.com/containerd/errdefs/blob/124d0dc9f0cd27578004a3f14e01ef8a3456d147/errors.go#L396 does not have a Unwrap method but otherwise the custom IsInterface there looks equivalent to errors.As() in here.

[stevvooe]

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.

[tonistiigi]

but classically, errdefs represent the end of the line for the error chain.

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, but lstat(boltdb) is internal.

[stevvooe]

Yes, agreed. Most internally errors will be unhandled errors that bubble up.

[stevvooe]

Could this just be errors.Is? Also, does system need to implement err for this to work?

[tonistiigi]

No, Is would do a direct comparison of any error in chain.

If you mean adding func (e internalErr) Is(target) bool { ... interface check } and then calling errors.Is(err, InternalErr{}) instead, then this will only work if some error in the chain is type internalErr and not when there is any error that just satisfies system interface as otherwise the custom Is() method would not get called.

Also, does system need to implement err for this to work?

No. Err needs to be assignable to system. System does not need to be assignable to error.

[stevvooe]

Ok, I see.

Typically, I would expect the following to be true for an internal error:

err := fallableWithInternalError()
errors.Is(err, errdefs.ErrInternal)

We'd likely need to have Unwrap() []error to make this work with a causal chain but that is why I was asking about having the pointer in the internalErr type above.

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.

[tonistiigi]

The main difference is still that for errors.Is(err, ErrInternal) this would only work if errdefs.Internal() (or some form of Wrap(errdefs.ErrInternal) is called on the error and not if the system interface is implemented or not. So would not automatically pick up containerd and moby system errors.

[stevvooe]

LGTM

A few nits but this seems like a reasonable approach.