New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Include output of failed exec in error message. #4161
Conversation
c7f4ce5
to
f614de1
Compare
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
stdoutFile, err := os.Open(stdoutPath) | ||
if err != nil && !os.IsNotExist(err) { | ||
panic(err) | ||
} | ||
_, err = io.Copy(os.Stdout, stdoutFile) | ||
if err != nil { | ||
panic(err) | ||
} | ||
stderrFile, err := os.Open(stderrPath) | ||
if err != nil && !os.IsNotExist(err) { | ||
panic(err) | ||
} | ||
_, err = io.Copy(os.Stderr, stderrFile) | ||
if err != nil { | ||
panic(err) | ||
} |
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.
If/when we do #3496 it might be a good idea to use the "combined" stream so the user sees how the output would have looked in a terminal. 🤔
var se *errdefs.SolveError | ||
if errors.As(returnErr, &se) { | ||
// Ensure we don't get blocked trying to return an error by enforcing a timeout | ||
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
defer cancel() | ||
|
||
op := se.Op | ||
if op == nil || op.Op == nil { | ||
return | ||
} | ||
execOp, ok := se.Op.Op.(*pb.Op_Exec) | ||
if !ok { | ||
return | ||
} |
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.
Are you a wizard?
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.
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.
🧙
Just want to say: 🤯 |
Signed-off-by: Erik Sipsma erik@sipsma.dev
Fixes #3025
I looked a bit into the upcoming build history API in buildkit v0.11 but realized the way its implemented makes it very hard to use for this case (it's all the progress for the whole build, not just one exec), so I think the approach in this pr, while inherently ugly, to be the only viable way.
We use a secret feature of buildkit that lets you obtain the mounts of a failed exec and use them in
NewContainer
calls. We then create a NewContainer with the meta mount of the failed exec and tell our shim to just output the contents of it.Example error message:
Important cleanups needed before merge: