Skip to content

Add pod logs for terminated container in status check. #4303

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

Merged
merged 10 commits into from
Jun 11, 2020

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Jun 9, 2020

Relates #3952

In this Pr, add logs for containers which are not healthy. These logs are now on the command line.
TODO: @balopat should these also be part of the Event API ?

On master previously, without pod logs for terminated containers,

  1. No logs were printed.
  2. Users had no way to discover why their container terminated. In skaffold dev the pods get cleaned up. There is no way for users to access these logs after skaffold dev loop.
Tags used in deployment:
 - leeroy-web -> gcr.io/tejal-test/leeroy-web:v1.10.0-109-g16de85142@sha256:3aff2363e7901c39241f3b89d21edf56e6f6a77ad5d65396937d39584f4665db
 - leeroy-app -> gcr.io/tejal-test/leeroy-app:v1.10.0-109-g16de85142-dirty@sha256:d01bad6b31bf9608a50b88632ff3377d88672b7a5bbc6dff0421a6c499c92c86
Starting deploy...
 - deployment.apps/leeroy-web created
 - service/leeroy-app created
 - deployment.apps/leeroy-app created
Waiting for deployments to stabilize...
 - deployment/leeroy-app: waiting for rollout to finish: 0 of 1 updated replicas are available...
    - pod/leeroy-app-5bdd8b5b58-rm4td: creating container leeroy-app
 - deployment/leeroy-web: waiting for rollout to finish: 0 of 1 updated replicas are available...
    - pod/leeroy-web-7f9748df7f-2g65g: creating container leeroy-web
 - deployment/leeroy-web is ready. [1/2 deployment(s) still pending]
 - deployment/leeroy-app: waiting for rollout to finish: 0 of 1 updated replicas are available...
    - pod/leeroy-app-5bdd8b5b58-rm4td: container leeroy-app terminated with exit code 2
^CCleaning up...
 - deployment.apps "leeroy-web" deleted
 - service "leeroy-app" deleted
 - deployment.apps "leeroy-app" deleted

On this branch,

Tags used in deployment:
 - leeroy-web -> gcr.io/tejal-test/leeroy-web:v1.10.0-109-g16de85142@sha256:3aff2363e7901c39241f3b89d21edf56e6f6a77ad5d65396937d39584f4665db
 - leeroy-app -> gcr.io/tejal-test/leeroy-app:v1.10.0-109-g16de85142-dirty@sha256:d01bad6b31bf9608a50b88632ff3377d88672b7a5bbc6dff0421a6c499c92c86
Starting deploy...
 - deployment.apps/leeroy-web created
 - service/leeroy-app created
 - deployment.apps/leeroy-app created
Waiting for deployments to stabilize...
 - deployment/leeroy-app: waiting for rollout to finish: 0 of 1 updated replicas are available...
    - pod/leeroy-app-5bdd8b5b58-rm4td: creating container leeroy-app
 - deployment/leeroy-web: waiting for rollout to finish: 0 of 1 updated replicas are available...
    - pod/leeroy-web-7f9748df7f-2g65g: creating container leeroy-web
 - deployment/leeroy-web is ready. [1/2 deployment(s) still pending]
 - deployment/leeroy-app: waiting for rollout to finish: 0 of 1 updated replicas are available...
    - pod/leeroy-app-5bdd8b5b58-rm4td: container leeroy-app terminated with exit code 2
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]2020/06/09 05:35:56 leeroy app server ready
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]panic: test
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]goroutine 1 [running]:
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]main.main()
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]	/go/app.go:15 +0x79
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]
 - deployment/leeroy-app: waiting for rollout to finish: 0 of 1 updated replicas are available...
    - pod/leeroy-app-5bdd8b5b58-rm4td: container leeroy-app is backing off waiting to restart
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]2020/06/09 05:35:57 leeroy app server ready
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]panic: test
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]goroutine 1 [running]:
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]main.main()
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]	/go/app.go:15 +0x79
[leeroy-app-5bdd8b5b58-rm4td leeroy-app]
^CCleaning up...
 - deployment.apps "leeroy-web" deleted
 - service "leeroy-app" deleted
 - deployment.apps "leeroy-app" deleted

@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #4303 into master will decrease coverage by 0.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4303      +/-   ##
==========================================
- Coverage   71.85%   71.74%   -0.11%     
==========================================
  Files         324      324              
  Lines       12449    12468      +19     
==========================================
  Hits         8945     8945              
- Misses       2935     2954      +19     
  Partials      569      569              
Impacted Files Coverage Δ
pkg/diag/validator/pod.go 1.35% <0.00%> (-0.17%) ⬇️
pkg/diag/validator/resource.go 0.00% <0.00%> (ø)
pkg/skaffold/deploy/resource/deployment.go 75.25% <0.00%> (-1.59%) ⬇️

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 60f64b6...60b9739. Read the comment docs.

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.

This is a great addition.

Comment on lines 251 to 254
lines := strings.Split(string(logs), "\n")
for i, s := range lines {
lines[i] = fmt.Sprintf("[%s %s]%s", po.Name, c, s)
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this output formatting should be moved into Deployment.ReportSinceLastUpdated(). Just gather it here.

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 only issue, we dont' have the container name in the pod status. hence i added it here.
Also, if any other tool uses the diag library, they could rely on logs as is.

@tejal29 tejal29 merged commit 0d628b9 into GoogleContainerTools:master Jun 11, 2020
@tejal29 tejal29 deleted the add_pod_logs 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.

3 participants