fix #226: support the readiness and liveness for kubernetes

[alonlong]

No description provided.

[codecov]

Codecov Report

Merging #238 (b2d0258) into main (c1babda) will decrease coverage by 1.21%.
The diff coverage is 44.34%.

@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
- Coverage   55.65%   54.44%   -1.20%     
==========================================
  Files          82       84       +2     
  Lines        3249     3470     +221     
==========================================
+ Hits         1808     1889      +81     
- Misses       1259     1390     +131     
- Partials      182      191       +9     

Impacted Files	Coverage Δ
pkg/agent/selfprofile.go	0.00% <0.00%> (ø)
pkg/agent/upstream/direct/direct.go	0.00% <0.00%> (ø)
pkg/cli/cli.go	61.25% <0.00%> (ø)
pkg/cli/server_unix.go	0.00% <0.00%> (ø)
pkg/cli/usage.go	9.76% <0.00%> (-0.50%)	⬇️
pkg/server/healthz.go	0.00% <0.00%> (ø)
pkg/server/ingest.go	56.80% <0.00%> (-3.73%)	⬇️
pkg/storage/key.go	88.71% <ø> (-0.17%)	⬇️
pkg/storage/local.go	0.00% <0.00%> (ø)
pkg/util/atexit/atexit.go	0.00% <0.00%> (ø)
... and 19 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 351586d...b2d0258. Read the comment docs.

[kolesnikovae]

LGTM.

I guess https://github.com/pyroscope-io/helm-chart should be updated accordingly. And, perhaps, we don't need a separate deployment manifest - consider use of the helm chart instead.

[alonlong]

LGTM.

I guess https://github.com/pyroscope-io/helm-chart should be updated accordingly. And, perhaps, we don't need a separate deployment manifest - consider use of the helm chart instead.

ok, i will commit a PR for helm chart

[petethepig]

LGTM

[beellz]

There should be two different endpoints for liveness and readiness.
The endpoint readiness is available (healthz) Wouldn't work for liveness.
If the readiness fails, it would not restart the pod, but it will restart the pod when the liveness fails.
It will be better to have two different endpoints for it.

[kolesnikovae]

There should be two different endpoints for liveness and readiness.
The endpoint readiness is available (healthz) Wouldn't work for liveness.
If the readiness fails, it would not restart the pod, but it will restart the pod when the liveness fails.
It will be better to have two different endpoints for it.

That makes a lot of sense. But right now we don't have any particular reason (apart from crash) for a health controller to restart pyroscope server. The same for readiness: the only condition we should meet before starting accepting incoming requests is that the local storage is accessible - and http server won't start otherwise. Therefore the same endpoint may serve both purposes: readiness probe and liveness probe.

Once we introduce more dependencies (e.g., connection to a remote storage backend), we should definitely split the endpoints and logic behind them.

@beellz @alonlong what do you think?

[beellz]

Will work for now.
Also added readiness and liveness in helm chart