Skip to content
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

fix: report build failures inline for failed concurrent builds #6911

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

briandealwis
Copy link
Member

Fixes: #6126

Description
This PR causes Skaffold to report build successes and failures inline as part of the build logs, just as we report cancelled builds.

 ---> 40349a2425ef
Step 3/8 : RUN exit 1
 ---> Running in cf6bf19cb78e
Build [leeroy-web] failed: The command '/bin/sh -c exit 1' returned a non-zero code: 1
Building [leeroy-app]...

Previously such an error would be passed up and reported by the top-level command instance. This works for serial builds (concurrency = 1, the default with local builds) and single-artifact builds as the failed build would be the last build reported. But it makes diagnosing builds failures very difficult with concurrent builds as Skaffold buffers the logs of each build and reports them as each completes. When a build fails, the log is copied out and any other builds in progress are cancelled. These cancelled logs may appear to have been interrupted mid-stream. And the actual build failure is easily lost in the sea of logs.

This change does not report the actual error on serial or single-artifact builds to avoid a visual stutter as Skaffold also reports the build failure in its final output, and leads to output like:

Building [base]...
Sending build context to Docker daemon  2.048kB
Step 1/4 : FROM gcr.io/distroless/base
 ---> 64bbfbb81976
Step 2/4 : RUN exit 1
 ---> Running in b605bfb58ba0
Build [base] failed: unable to stream build output: OCI runtime create failed: container_linux.go:380: starting container process caused: exec: "/bin/sh": stat /bin/sh: no such file or directory: unknown. Please fix the Dockerfile and try again..
unable to stream build output: OCI runtime create failed: container_linux.go:380: starting container process caused: exec: "/bin/sh": stat /bin/sh: no such file or directory: unknown. Please fix the Dockerfile and try again..

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix tests and merge!

@codecov
Copy link

codecov bot commented Dec 10, 2021

Codecov Report

Merging #6911 (62ab9f0) into main (290280e) will decrease coverage by 1.65%.
The diff coverage is 56.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6911      +/-   ##
==========================================
- Coverage   70.48%   68.83%   -1.66%     
==========================================
  Files         515      551      +36     
  Lines       23150    25305    +2155     
==========================================
+ Hits        16317    17418    +1101     
- Misses       5776     6713     +937     
- Partials     1057     1174     +117     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/fix.go 68.85% <40.00%> (-7.62%) ⬇️
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/skaffold.go 76.19% <70.00%> (-8.43%) ⬇️
... and 170 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5933db1...62ab9f0. Read the comment docs.

@tejal29 tejal29 merged commit cb636a8 into GoogleContainerTools:main Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

skaffold render sometimes exits early
2 participants