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 termination event not being sent #5258

Merged
merged 6 commits into from
Jan 21, 2021

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Jan 21, 2021

In #4926 I added a new event to send a termination event to detect failures in skaffold setup.
However, during more rigorous testing, i saw some abnormalities where the event was received.

  1. Send a nil event to indicate close of channel when skaffold terminates
  2. Log all events.

Steps to Reproduce:

  1. Please compile skaffold on this branch using make
  2. Check if helm is installed and uninstall it or simple move.
  3. if yes,
 sudo mv /usr/local/bin/helm ~/workspace/helm1
  1. Open a new terminal
watch -n 0.5  "curl -H "Expect: any" localhost:50052/v1/events >> t.txt" 

5.. Checkout a skaffold helm example run skaffold dev

cd examples/helm-deployment
../../out/skaffold dev
  1. Open t.txt and verify if "TerminationEvent" is done
{"result":{"timestamp":"2021-01-20T23:56:59.856262Z","event":{"metaEvent":{"entry":"Starting Skaffold: \u0026{Version:v1.17.1-75-g1f519d095-dirty ConfigVersion:skaffold/v2beta11 GitVersion: GitCommit:1f519d095243f08f3b3c4db72f3c585d763b4454 GitTreeState:dirty BuildDate:2021-01-20T15:47:23Z GoVersion:go1.15.3 Compiler:gc Platform:darwin/amd64}","metadata":{"build":{"numberOfArtifacts":1,"builders":[{"type":"DOCKER","count":1}],"type":"LOCAL"},"deploy":{"deployers":[{"type":"HELM","count":1}],"cluster":"MINIKUBE"}}}}}}
{"result":{"timestamp":"2021-01-20T23:56:59.857213Z","event":{
   "terminationEvent":{"status":"Failed","err":{"errCode":"DEPLOY_HELM_VERSION_ERR","message":"creating deployer: Helm not found. Please install helm via https://helm.sh/docs/intro/install.","suggestions":[{"suggestionCode":"INSTALL_HELM","action":"Please install helm via https://helm.sh/docs/intro/install"}]}}}}}
{"result":{"timestamp":"2021-01-20T23:56:59.857219Z"}}

@tejal29 tejal29 requested a review from a team as a code owner January 21, 2021 00:19
@google-cla google-cla bot added the cla: yes label Jan 21, 2021
@tejal29 tejal29 added this to the v1.18.0 milestone Jan 21, 2021
@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #5258 (b520ec9) into master (62f4dff) will increase coverage by 0.02%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5258      +/-   ##
==========================================
+ Coverage   71.90%   71.93%   +0.02%     
==========================================
  Files         388      389       +1     
  Lines       14063    14148      +85     
==========================================
+ Hits        10112    10177      +65     
- Misses       3210     3223      +13     
- Partials      741      748       +7     
Impacted Files Coverage Δ
pkg/skaffold/errors/errors.go 87.23% <33.33%> (+0.27%) ⬆️
pkg/skaffold/event/event.go 90.78% <100.00%> (+0.02%) ⬆️
pkg/skaffold/docker/image.go 78.13% <0.00%> (-1.40%) ⬇️
pkg/skaffold/schema/defaults/defaults.go 87.42% <0.00%> (-0.08%) ⬇️
cmd/skaffold/app/cmd/flags.go 87.50% <0.00%> (ø)
pkg/skaffold/config/options.go 100.00% <0.00%> (ø)
pkg/skaffold/schema/latest/config.go 58.82% <0.00%> (ø)
pkg/skaffold/runner/generate_pipeline.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/parse_config.go 73.49% <0.00%> (ø)
pkg/skaffold/schema/profiles.go 89.61% <0.00%> (+0.13%) ⬆️
... and 3 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 62f4dff...b520ec9. Read the comment docs.

@briandealwis
Copy link
Member

The TestDebug/helm is #5249 which should be fixed by #5262.

@tejal29 tejal29 force-pushed the fix_termination_event branch from c801581 to b42ea9c Compare January 21, 2021 18:00
}

ev.logEvent(*logEntry)
Copy link
Member

Choose a reason for hiding this comment

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

Aside: it doesn't seem that logEntry needs to be a pointer?

Copy link
Contributor

@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!

@tejal29 tejal29 merged commit ebfcc99 into GoogleContainerTools:master Jan 21, 2021
@tejal29 tejal29 deleted the fix_termination_event branch July 21, 2021 17:38
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.

3 participants