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

Add an init phase to detect skaffold errors even before skaffold runner is created. #4926

Merged
merged 6 commits into from
Oct 21, 2020

Conversation

tejal29
Copy link
Member

@tejal29 tejal29 commented Oct 19, 2020

Relates to #4692

Add distinct error codes to reduce "UNKNOWN_ERROR" when skaffold was not able to create a runner.
where phase= "UNKNOWN", and error code is "ERROR"
This happens when skaffold.NewForConfig fails when skaffold fails to create

  • a builder,
  • deployer,
  • init build dependencies
  • init sync
  • init watch trigger

Any errors occurred here will now be part of "INIT" phase.

This PR also identifies failure when deploy context is minikube and minikube cluster is stopped.
On master run

  1. minikube stop
  2. kubectl config use-context minikube
  3. skaffold dev

Error,

 skaffold dev     
creating runner: creating builder: getting docker client: getting minikube env: running [/Users/tejaldesai/Downloads/google-cloud-sdk2/bin/minikube docker-env --shell none -p minikube]
 - stdout: "* The control plane node must be running for this command\n  - To fix this, run: \"minikube start\"\n"
 - stderr: ""
 - cause: exit status 89
➜  microservices git:(minikube_errr) ✗ 

With this PR

 ../../out/skaffold dev
minikube is probably not running. Try running "minikube start".

Make sure the error is seen in Event API
Since this happens pretty fast, In another terminal run

 watch --interval=0.1 curl localhost:50052/v1/events >> test.txt

in test.txt

{"result":{"timestamp":"2020-10-19T22:15:10.426813Z","event":{"metaEvent":{"entry":"Starting Skaffold: \u0026{Version:v1.15.0-23-gec73dafff ConfigVersi^[[10;1Hon:skaffold/v2beta9 GitVersion: GitCommit:ec73dafff8fd5714fabea148270d975de05b5449 GitTreeState:clean BuildDate:2020-10-19T14:58:30Z GoVersion:go1.14.3^[[11;2HCompiler:gc Platform:darwin/amd64}","metadata":{"build":{"numberOfArtifacts":2,"builders":[{"type":"DOCKER","count":2}],"type":"LOCAL"},"deploy":{"dep^[[12;1Hloyers":[{"type":"KUBECTL","count":1}],"cluster":"MINIKUBE"}}}}}}^M^[[13d{"result":{"timestamp":"2020-10-19T22:15:10.528741Z",

"event":{"sessionEndEvent":{
   "status":"Failed",
    "err":  {
                 "errCode":"INIT_MINIKUBE_NOT_RUNNING_ERROR",
                 "message":"creating builder: getting docker client: getting minikube env: running [/Users/tejaldesai/Downloads/google-cloud-sdk2/bin/minikube docker-env^[[15;2H--shell none -p minikube]\n - stdout: \"* The control plane node must be running for this command\\n  - To fix this, run: \\\"minikube start\\\"\\n\"\^[[16;1Hn - stderr: \"\"\n - cause: exit status 89",
   "suggestions":[
        {"suggestionCode":"START_MINIKUBE","
          action":"Try running \"minikube start\""}
   ]}}

…d in the initial phase.

If, Minikube is not running, and skaffold deploy context is set to "minikube", skaffold fails
when creating a runner.
This failure has no phase attached to it.

Adding a Init Phase, will help reduce errors where "UNKNOWN_ERROR" is seen to "INIT_KNOWNN"
@tejal29 tejal29 requested a review from a team as a code owner October 19, 2020 22:31
@google-cla google-cla bot added the cla: yes label Oct 19, 2020
@tejal29 tejal29 changed the title Add a init phase to detect skaffold errors even before skaffold runner is created. Add an init phase to detect skaffold errors even before skaffold runner is created. Oct 19, 2020
@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #4926 into master will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4926      +/-   ##
==========================================
+ Coverage   72.18%   72.36%   +0.17%     
==========================================
  Files         358      360       +2     
  Lines       12402    12474      +72     
==========================================
+ Hits         8953     9027      +74     
+ Misses       2795     2789       -6     
- Partials      654      658       +4     
Impacted Files Coverage Δ
pkg/skaffold/errors/build_problems.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/runner.go 71.05% <100.00%> (+0.78%) ⬆️
pkg/skaffold/errors/err_map.go 96.66% <100.00%> (ø)
pkg/skaffold/errors/errors.go 96.77% <100.00%> (+16.77%) ⬆️
pkg/skaffold/errors/init_problems.go 100.00% <100.00%> (ø)
pkg/skaffold/event/event.go 91.76% <100.00%> (+0.65%) ⬆️
pkg/skaffold/runner/new.go 64.28% <100.00%> (+0.23%) ⬆️
pkg/skaffold/schema/v2beta8/upgrade.go 88.88% <0.00%> (-11.12%) ⬇️
pkg/skaffold/build/cache/cache.go 53.65% <0.00%> (ø)
pkg/skaffold/deploy/util/logfile.go 80.00% <0.00%> (ø)
... and 11 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 5d8b798...22174fa. Read the comment docs.

@tejal29 tejal29 added the kokoro:force-run forces a kokoro re-run on a PR label Oct 19, 2020
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Oct 19, 2020
Copy link
Collaborator

@gsquared94 gsquared94 left a comment

Choose a reason for hiding this comment

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

LGTM, other than logEntry arriving out of order seems like a pre-existing issue.

@gsquared94 gsquared94 added the kokoro:force-run forces a kokoro re-run on a PR label Oct 20, 2020
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Oct 20, 2020
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

A few suggested renames, mostly to avoid confusion with skaffold init, and minor nits.

pkg/skaffold/errors/initProblems.go Outdated Show resolved Hide resolved
proto/skaffold.proto Outdated Show resolved Hide resolved
pkg/skaffold/errors/initProblems_test.go Outdated Show resolved Hide resolved
pkg/skaffold/event/event.go Outdated Show resolved Hide resolved
proto/skaffold.proto Outdated Show resolved Hide resolved
proto/skaffold.proto Outdated Show resolved Hide resolved
docs/content/en/docs/references/api/grpc.md Outdated Show resolved Hide resolved
proto/skaffold.proto Outdated Show resolved Hide resolved
proto/skaffold.proto Outdated Show resolved Hide resolved
proto/skaffold.proto Outdated Show resolved Hide resolved
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

One nit

proto/skaffold.proto Outdated Show resolved Hide resolved
@tejal29 tejal29 merged commit 12d64a2 into GoogleContainerTools:master Oct 21, 2020
@tejal29 tejal29 deleted the minikube_errr branch April 15, 2021 07:34
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.

4 participants