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.

Sign up for GitHub

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

Jump to bottom

add docker build distinct error codes #5059

Merged
merged 5 commits into from
Dec 1, 2020
Merged

add docker build distinct error codes #5059

merged 5 commits into from
Dec 1, 2020

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Nov 24, 2020 •
edited
Loading

should merge after #5060
Related to #4692

In this PR,

  1. First add a pkg/skaffold/error.ErrDef helper error for builders, deployers etc to create a error with errorcode and suggestion code.

Then,

Add distinct error codes for docker build.
Docker exposes two types of error

  1. Internal system error
    All these error codes are from https://github.com/moby/moby/blob/master/errdefs/defs.go
    These errors can happen for cases like,
  • docker context tar is empty, - probably issue with getDependencies
  • docker build is accessing a volume mount which can't be created or is deleted.
  • a conflict occurs when scheduling a build container etc.
  • docker build/dockerfile is using features which are not supported.
  1. Docker build error due to user error or incorrect dockerfile.
    In this case of errors executing the steps in dockerfile, a jsonmessage.JsonError is thrown.

@google-cla google-cla bot added the cla: yes label Nov 24, 2020
@codecov
Copy link

codecov bot commented Nov 24, 2020 •
edited
Loading

Codecov Report

Merging #5059 (14c2e36) into master (5b19e57) will increase coverage by 0.02%.
The diff coverage is 58.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5059      +/-   ##
==========================================
+ Coverage   72.35%   72.38%   +0.02%     
==========================================
  Files         371      372       +1     
  Lines       13042    13114      +72     
==========================================
+ Hits         9436     9492      +56     
- Misses       2917     2931      +14     
- Partials      689      691       +2
Impacted Files Coverage Δ
pkg/skaffold/errors/err_def.go 72.72% <ø> (ø)
pkg/skaffold/build/docker/errors.go 56.00% <56.00%> (ø)
pkg/skaffold/build/docker/docker.go 76.59% <83.33%> (+34.59%) ⬆️
pkg/skaffold/docker/image.go 79.53% <0.00%> (-1.40%) ⬇️
pkg/skaffold/errors/errors.go 93.18% <0.00%> (+4.54%) ⬆️

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 5b19e57...14c2e36. Read the comment docs.

@tejal29 tejal29 force-pushed the supress_build_unknowns branch from 0efcf5a to 764f25b Compare November 24, 2020 07:05
@tejal29 tejal29 marked this pull request as ready for review November 24, 2020 23:41
@tejal29 tejal29 requested a review from a team as a code owner November 24, 2020 23:41
@tejal29 tejal29 requested a review from briandealwis November 24, 2020 23:41
@tejal29 tejal29 added the docs-modifications runs the docs preview service on the given PR label Nov 24, 2020
@container-tools-bot
Copy link

Please visit http://35.235.64.137:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Nov 24, 2020
@tejal29 tejal29 force-pushed the supress_build_unknowns branch 2 times, most recently from 655f54c to 3d9f94a Compare November 26, 2020 00:33
@tejal29 tejal29 mentioned this pull request Nov 26, 2020
Merged
@tejal29 tejal29 mentioned this pull request Nov 26, 2020
Merged
briandealwis
briandealwis approved these changes Nov 26, 2020
View reviewed changes
pkg/skaffold/build/docker/docker.go
@@ -115,7 +112,7 @@ func (b *Builder) pullCacheFromImages(ctx context.Context, out io.Writer, a *lat
}

if err := b.localDocker.Pull(ctx, out, image); err != nil {
warnings.Printf("Cache-From image couldn't be pulled: %s\n", image)
warnings.Printf("cacheFrom image couldn't be pulled: %s\n", image)
Copy link
Member

Choose a reason for hiding this comment

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

This err seems like it might be useful to be reported too, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current behavior is to only warn and continue building with a warning. The docker build might actually succeed if the image is present on remote. Not sure why it was decided. There is a test to verify it too.

pkg/skaffold/build/docker/errors.go Show resolved Hide resolved
pkg/skaffold/build/docker/errors.go
func newBuildError(err error) error {
errU := errors.Unwrap(err)
if errU == nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

What kind of errors are these? Why do we return them instead of wrapping them with a BUILD_UNKNOWN error? (is it because they'll be parsed and turned into a error elsewhere?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are known docker errors. Docker wraps these for known types https://github.com/moby/moby/blob/master/errdefs/defs.go
e.g. here
https://github.com/moby/moby/blob/master/errdefs/helpers.go#L138

proto/skaffold.proto Outdated Show resolved Hide resolved
@tejal29 tejal29 force-pushed the supress_build_unknowns branch from 0ce9698 to d85b932 Compare November 30, 2020 23:27
@tejal29 tejal29 force-pushed the supress_build_unknowns branch from d85b932 to 14c2e36 Compare November 30, 2020 23:45
@tejal29 tejal29 merged commit 1ae3284 into GoogleContainerTools:master Dec 1, 2020
@fmotrifork fmotrifork mentioned this pull request Dec 3, 2020
Merged
@tejal29 tejal29 deleted the supress_build_unknowns branch April 15, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briandealwis briandealwis briandealwis approved these changes

Assignees
No one assigned
Labels
area/errors cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tejal29 @container-tools-bot @briandealwis