errdefs: add support for typed errors #1454

tonistiigi · 2020-04-21T05:00:45Z

Add support for typed errors that are maintained through components and rpc calls.

As a first example, error from the client side also contains stack traces from the daemon side components.

Users shouldn't care about the grpc errors handling. Just use regular errors.Wrap and errors.Is/As checks that should match supported error types from errdefs package automatically.

@AkihiroSuda @hinshun

solver/errdefs/stack.go

hinshun · 2020-04-21T19:36:49Z

solver/errdefs/errdefs.proto

+    repeated string Cmdline = 2;
+    string Checksum = 3;
+    int32 Pid = 4;
+    // go commit, frontend digest


Is the intention here to eventually support propagating errors.WithStack from within frontends?

Yes. You can look at https://gist.github.com/tonistiigi/3f3cfcf274f5dde5b7cb69f44f44ed84 for an example. That trace contains stacks from buildctl, buildkitd and two dockerfile containers (one that does the build and other that forwards #syntax). Bit verbose because of pkg/errors#75 but we're not going to show this part to users by default anyway.

Awesome, this is really useful.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi · 2020-04-22T06:00:42Z

Vertex and source support added in addition to stack support. I'll continue adding more but this should be enough to review for the first PR.

tonistiigi · 2020-04-22T06:03:25Z

Probably need to change some code around in follow-up. Next step would be to add this to fsutil transfer errors, but that would require a backlinked dependency.

tonistiigi · 2020-04-22T06:03:55Z

frontend/dockerfile/instructions/parse.go

@@ -130,7 +132,7 @@ func (e *parseError) Error() string {
 	return fmt.Sprintf("dockerfile parse error line %d: %v", e.node.StartLine, e.inner.Error())
 }

-func (e *parseError) Unwarp() error {


hinshun · 2020-04-22T06:13:13Z

frontend/dockerfile/builder/build.go

+		Filename:  filename,
+		Local:     local,
+		Locations: make([]*errdefs.Range, 0, len(locations)),
+	}


This is... source mapping? Wow!

hinshun

PR currently failing with:

#13 5.205 The result of "go generate" differs
#13 5.205 
#13 5.205  M solver/errdefs/errdefs.pb.go
#13 5.205 
#13 5.205 Please update with "make generated-files"

hinshun · 2020-04-22T06:17:26Z

solver/jobs.go

@@ -53,6 +54,7 @@ type state struct {

 	vtx          Vertex
 	clientVertex client.Vertex
+	origDigest   digest.Digest // original LLB digest. TODO: probably better to use string ID so this isn't needed


TODO will be done in another PR?

Yes, this is a solver internals change that I don't want to do atm.

tonistiigi · 2020-04-22T06:38:00Z

api/services/control/errors.go

+	grpc "google.golang.org/grpc"
+)
+
+func WrapServerErrors(srv ControlServer) ControlServer {


maybe I should do grpc.StreamInterceptor grpc.UnaryInterceptor instead. Not much cleaner and need to merge with existing ones. But no changes required on new API endpoints.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

This reverts commit 3f77f04. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tiborvass · 2020-04-23T01:40:50Z

util/grpcerrors/grpcerrors.go

+	}
+	st, ok := status.FromError(err)
+	if !ok {
+		return err


when would you do FromGRPC(notAGRPCError{})? Maybe do (error, bool) like status.FromError.

this doesn't return any extra info/type so I don't think bool is needed and just complicated the caller side

tiborvass · 2020-04-23T01:50:46Z

util/stack/stack.go

+	switch verb {
+	case 'v':
+		if s.Flag('+') {
+			fmt.Fprintf(s, "%+v\n", w.Error())


w.Error() always returns a string, so no need for %+v afaict, could be %s which is less confusing (I thought there would be some kind of recursion due to %+v like it is the case in pkg/errors. If there is any recursion it will be in the Error() method itself.

I think this basically should be io.WriteString(s, w.Error()) like when it woudl fallthrough if not +

tiborvass · 2020-04-23T01:55:57Z

util/stack/stack.go

+				for _, f := range stack.Frames {
+					fmt.Fprintf(s, "%s\n\t%s:%d\n", f.Name, f.File, f.Line)
+				}
+				fmt.Fprintf(s, "\n")


tiborvass · 2020-04-23T01:58:49Z

util/stack/stack.go

+func convertStack(s errors.StackTrace) *Stack {
+	var out Stack
+	for _, f := range s {
+		dt, err := f.MarshalText()


Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

hinshun · 2020-04-24T19:25:22Z

frontend/dockerfile/parser/errors.go

 type Range struct {
 	Start Position
 	End   Position
 }

+// Position is a point in source code
 type Position struct {
 	Line      int
 	Character int
 }


This maps directly to LSP: https://github.com/sourcegraph/go-lsp/blob/4631ffd93a18dc7dd940f937be13589f6ff79826/structures.go#L5-L40

I think at some point we can build a frontend from an IDE and see diagonistics there directly via LSP.

Yes, this is where I took it so it would be compatible. Now, who is up to rewriting the Dockerfile parser to LSP? :)

I think a better solution is to write a generic frontend integration for LSP error diagnosis. I think they call it "code actions"? In order to run frontends, diagnose errors, or even a LLB debugger (That's shareable between frontends). You can supplement that with frontend-specific implementations like jump definition, autocomplete, semantic highlighting for Dockerfile.

Yes, the errors come from LLB level and using VertexError (and probably typed error for every op type in future) here can be mapped to source but to be useful for the user they need to be mapped to original source code.

hinshun reviewed Apr 21, 2020

View reviewed changes

tonistiigi force-pushed the errdefs branch from e4f717e to c05334f Compare April 21, 2020 21:06

errdefs: add support for typed errors

02fff48

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the errdefs branch from c05334f to cf90f83 Compare April 22, 2020 05:57

tonistiigi marked this pull request as ready for review April 22, 2020 05:58

tonistiigi commented Apr 22, 2020

View reviewed changes

hinshun reviewed Apr 22, 2020

View reviewed changes

tonistiigi force-pushed the errdefs branch from cf90f83 to 2c859b0 Compare April 22, 2020 06:28

tonistiigi commented Apr 22, 2020

View reviewed changes

tonistiigi added 6 commits April 21, 2020 23:46

grpc: add error handling wrappers to client/server

3f77f04

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

gateway: fix error details passing through gateway

e4cc086

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

errdefs: report component version in stack

ae3b75d

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

solver: attach causing vertex in an error

cce301b

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

errdefs: attach source to an error

abbda4e

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

dockerfile: add more source information to errors

4b2636a

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the errdefs branch from 2c859b0 to 4b2636a Compare April 22, 2020 06:46

tonistiigi added 3 commits April 22, 2020 15:58

util: add grpcerrors and stack package

c511cdb

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

Revert "grpc: add error handling wrappers to client/server"

2fc9aee

This reverts commit 3f77f04. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

grpc interceptors for errors

725f5e1

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the errdefs branch from 1951422 to b8295a9 Compare April 23, 2020 01:31

tiborvass reviewed Apr 23, 2020

View reviewed changes

tonistiigi force-pushed the errdefs branch from b8295a9 to b33d73a Compare April 23, 2020 01:41

tiborvass reviewed Apr 23, 2020

View reviewed changes

tonistiigi force-pushed the errdefs branch 2 times, most recently from e0ceb12 to 704f544 Compare April 23, 2020 02:20

tonistiigi force-pushed the errdefs branch from 704f544 to 235ec48 Compare April 23, 2020 04:29

tonistiigi added 2 commits April 24, 2020 11:25

errdefs: update to new packages

90288ab

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

remove last instances of errors cause

c78e875

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the errdefs branch from 235ec48 to c78e875 Compare April 24, 2020 18:26

tonistiigi requested review from hinshun and AkihiroSuda April 24, 2020 18:26

hinshun approved these changes Apr 24, 2020

View reviewed changes

tiborvass approved these changes Apr 27, 2020

View reviewed changes

tiborvass merged commit 1088921 into moby:master Apr 27, 2020

tonistiigi mentioned this pull request May 2, 2020

Improved debugging support #1472

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errdefs: add support for typed errors #1454

errdefs: add support for typed errors #1454

tonistiigi commented Apr 21, 2020 •

edited

Loading

hinshun Apr 21, 2020

tonistiigi Apr 21, 2020

hinshun Apr 21, 2020

tonistiigi commented Apr 22, 2020

tonistiigi commented Apr 22, 2020

tonistiigi Apr 22, 2020

hinshun Apr 22, 2020

hinshun left a comment

hinshun Apr 22, 2020

tonistiigi Apr 22, 2020

tonistiigi Apr 22, 2020

tiborvass Apr 23, 2020

tonistiigi Apr 23, 2020

tiborvass Apr 23, 2020 •

edited

Loading

tiborvass Apr 23, 2020

tiborvass Apr 23, 2020

tiborvass Apr 23, 2020

hinshun Apr 24, 2020

tonistiigi Apr 24, 2020

hinshun Apr 24, 2020 •

edited

Loading

tonistiigi Apr 24, 2020

errdefs: add support for typed errors #1454

errdefs: add support for typed errors #1454

Conversation

tonistiigi commented Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Apr 22, 2020

tonistiigi commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hinshun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hinshun Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Apr 21, 2020 •

edited

Loading

tiborvass Apr 23, 2020 •

edited

Loading

hinshun Apr 24, 2020 •

edited

Loading