knative / serving

Kubernetes-based, scale-to-zero, request-driven compute
https://knative.dev/docs/serving/
Apache License 2.0
5.51k stars 1.15k forks source link

Queue Proxy Probe doesn't include an informative error message #6808

Closed dprotaso closed 4 years ago

dprotaso commented 4 years ago

What version of Knative?

HEAD

Expected Behavior

When the readiness probe fails the event includes an informative message

Actual Behavior

The probe's logger shouldn't dump logs to stdout as this is reserved for informing the user

Readiness probe failed: {"level":"info","ts":"2020-02-11T14:27:52.903Z","caller":"logging/config.go:108","msg":"Successfully created the logger."}

Steps to Reproduce the Problem

The default logger shouldn't be initialized & used when the probing https://github.com/knative/serving/blob/afbd55a1df2d1548f603bd249b4672141beb8007/cmd/queue/main.go#L350-L374

markusthoemmes commented 4 years ago

I agree. Let's move that block above again and just use a default log.Fatalf(...) instead of a proper logger for the validateEnv error handling.

/kind good-first-issue /good-first-issue

knative-prow-robot commented 4 years ago

@markusthoemmes: This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-good-first-issue command.

In response to [this](https://github.com/knative/serving/issues/6808): >I agree. Let's move that block above again and just use a default `log.Fatalf(...)` instead of a proper logger for the `validateEnv` error handling. > >/kind good-first-issue >/good-first-issue Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
vagababov commented 4 years ago

There's a comment to why it is so. Do we want to ask the authors for why?

dprotaso commented 4 years ago

@vagababov which author you referring to?

vagababov commented 4 years ago

/me rolls up sleeves and opens blame view recent change points to @nimakaviani but he just relocated the code, which was I think added here https://github.com/knative/serving/pull/4148, so @joshrider

markusthoemmes commented 4 years ago

The point is, we want the clear log output that Josh put in in the first place by not using the logger. That was broken by the move of the logic recently. We can just undo that move as described above.

dprotaso commented 4 years ago

Using & initializing the logger while the queue proxy is probing will result in json appearing in the k8s event which isn't user friendly

serving-tests            18m         Warning   Unhealthy                      pod/call-to-public-service-kpshoeih-hhncq-deployment-b8fcb9c46cmhv7           Readiness probe failed: {"level":"info","ts":"2020-02-10T15:38:51.113Z","caller":"logging/config.go:108","msg":"Successfully created the logger."}
{"level":"info","ts":"2020-02-10T15:38:51.113Z","caller":"logging/config.go:109","msg":"Logging level set to debug"}
vagababov commented 4 years ago

eh, I see. IT's the opposite.