feat compact: added readiness Prober by FUSAKLA · Pull Request #1297 · thanos-io/thanos

FUSAKLA · 2019-07-02T05:37:13Z

Changes

Used new Prober package in the Thanos compact to provide /-/healthy and /-/ready endpoints

I also added new defatltHTTPListener which registers the prober. I'm leaving the old metricHTTPListenGroup with TODO to remove it once all components are migrated to the new one to avoid touching different components in this PR.

Also I switched to using component.Component instead of passing the string from the main.
It felt weird to have somewhere string and somewhere the component interface.

Verification

Tested it and works as expected, also exposes metrics about Prober status

# HELP thanos_prober_healthy Represents health status of the component Prober.
# TYPE thanos_prober_healthy gauge
thanos_prober_healthy{component="compact"} 1
# HELP thanos_prober_ready Represents readiness status of the component Prober.
# TYPE thanos_prober_ready gauge
thanos_prober_ready{component="compact"} 1

FUSAKLA · 2019-07-02T05:42:46Z

@bwplotka
So first one of the series of Prober additions 🎉

Formerly we discussed:

Naming of the defaultHTTPListener
If the default listener should
- set healthy
- set ready

I chose compact as a first one since it should be most straight forward :)

GiedriusS

LGTM besides one nit

CHANGELOG.md

GiedriusS · 2019-07-02T13:48:54Z

cmd/thanos/main.go

 	registerQuery(cmds, app, "query")
 	registerRule(cmds, app, "rule")
-	registerCompact(cmds, app, "compact")
+	registerCompact(cmds, app, component.Compact)


I know that you have not introduced this parameter but I feel that in the long term this parameter should go away and we should just use component.Compact inside of the function itself since it is already designated for registering the compact command 😄

Sounds good to me, I thought about it too but I wasn't sure even with this change.
If we agree I'll be happy to change it. PR like this will come for every component so eventually I can change it for all of them.

changed in 127ee71 is this ok with you?

GiedriusS · 2019-07-02T13:51:42Z

Oh, and could we update tutorials/kubernetes-demo/manifests/thanos-compactor.yaml so that it would use these new end-points according to this https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/? 😍

FUSAKLA · 2019-07-02T15:16:20Z

@GiedriusS I knew I forgot about something, thanks for reminding me! 😄 Ill modify those manifests too.

FUSAKLA · 2019-07-02T17:38:56Z

@GiedriusS manifests should be fixed now PTAL

bwplotka

LGTM!

Thanks. Minor 2 nits only.

cmd/thanos/main.go

bwplotka · 2019-07-11T09:59:45Z

cmd/thanos/compact.go

-func registerCompact(m map[string]setupFunc, app *kingpin.Application, name string) {
-	cmd := app.Command(name, "continuously compacts blocks in an object store bucket")
+func registerCompact(m map[string]setupFunc, app *kingpin.Application) {
+	component := component.Compact


const component = component.Compact?

hm it looks like struct or any other more complex data structure cannot be a constant IIUC?
I renamed the variable at least to comp so it does not collide with the package name.

FUSAKLA · 2019-07-11T15:55:40Z

thanks @bwplotka for the cr!

povilasv

So we are setting healthy when we start listening for metrics and ready when initialization finishes?

I think for compact we should set ready and healthy after initialization finishes. As for compact ready isn't important as it isn't serving traffic, so we actually only care about healthy.

Thoughts?

FUSAKLA · 2019-07-16T14:44:57Z

Yes, the healthy status is set by the defaultHTTPListener right from the time when the /-/healthy endpoint is available which happen to be the same time we start to listen for metrics.

The readiness is left there just so it is somewhere. As you said in case of compactor it does not matter.

IIUC you would move setting healthiness to the same place as readiness? IMHO this would not change anything since the server is not listening yet so setting healthy before it starts has no meaning and just would require to take out the generic liveness setting from the defaultHTTPListener. But maybe I'm missing out something?

povilasv · 2019-07-23T07:39:14Z

I think we should just remove readiness, it doesn't make sense in compactor and we shouldn't expose it. Users should just use healthy. WDYT?

FUSAKLA · 2019-07-25T15:34:36Z

TBH I'd rather leave it there. All other components will hopefully have it and user could be confused that it's missing. I'd just say it's ready right from the start?

But if you prefer removing it I'll make it disappear :)

midnightconman · 2019-07-25T15:56:01Z

TBH I'd rather leave it there. All other components will hopefully have it and user could be confused that it's missing. I'd just say it's ready right from the start?

But if you prefer removing it I'll make it disappear :)

I agree let's leave it in. This makes helm / deployment logic simpler. Standards reduce cognitive load :-)

GiedriusS

LGTM and I agree with the comments. Could we fix up the CHANGELOG.md changes so that it would go under the relevant section before merging this? Plus you have some merge conflicts :)

FUSAKLA · 2019-07-29T21:15:10Z

@GiedriusS should be rebased now with all the conflicts resolved and changelog entry adjusted as well. Sorry for the delay

FUSAKLA · 2019-07-30T19:09:51Z

hm.. weird. retriggered the CI and it passed and before it failed on Azure tests.
seems to be somehow flaky :/

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>

GiedriusS · 2019-07-31T12:40:25Z

LGTM, @bwplotka could you make the final call here in light of comments by Povilas?

povilasv · 2019-08-05T06:21:33Z

@GiedriusS I agree let's reduce cognitive load and keep it ;)

povilasv · 2019-08-05T06:24:20Z

Regarding

IIUC you would move setting healthiness to the same place as readiness? IMHO this would not change anything since the server is not listening yet so setting healthy before it starts has no meaning and just would require to take out the generic liveness setting from the defaultHTTPListener. But maybe I'm missing out something?

I would still set readiness and healthiness in the same place if possible, to reduce the cognitive burden on developers. But other than that LGTM

* master: iter.go: error message typo correction (thanos-io#1376) Fix usage of $GOPATH in Makefile (thanos-io#1379) Moved Prometheus 2.11.1 and TSDB to 0.9.1 (thanos-io#1380) Store latest config hash and timestamp as metrics (thanos-io#1378) pkg/receive/handler.go: log errors (thanos-io#1372) receive: Hash-ring metrics (thanos-io#1363) receiver: avoid race of hashring (thanos-io#1371) feat compact: added readiness Prober (thanos-io#1297) Add changelog entry for S3 option (thanos-io#1361) Multipart features (thanos-io#1358) Added katacoda.yaml (thanos-io#1359) Remove deprecated option from example (thanos-io#1351) Move suggestion about admin API to appropriate place (thanos-io#1355)

FUSAKLA · 2019-08-07T17:01:59Z

Great, thanks for the review and merging this!

I'll start with PR for next thanos component to add the prober

FUSAKLA force-pushed the fus-compact-prober branch from 25100a8 to f91182c Compare July 2, 2019 05:38

FUSAKLA mentioned this pull request Jul 2, 2019

WIP: Added /-/healthy and /-/ready endpoints to all thanos components #656

Closed

GiedriusS requested changes Jul 2, 2019

View reviewed changes

bwplotka approved these changes Jul 11, 2019

View reviewed changes

FUSAKLA force-pushed the fus-compact-prober branch from 127ee71 to 67fbcc6 Compare July 11, 2019 15:53

povilasv requested changes Jul 16, 2019

View reviewed changes

GiedriusS approved these changes Jul 25, 2019

View reviewed changes

FUSAKLA force-pushed the fus-compact-prober branch 3 times, most recently from 0ad706e to 129035b Compare July 29, 2019 21:13

FUSAKLA force-pushed the fus-compact-prober branch from 129035b to ed91fdb Compare July 30, 2019 15:16

feat compact: added readiness Prober

55ebdb6

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>

FUSAKLA force-pushed the fus-compact-prober branch from ed91fdb to 55ebdb6 Compare July 30, 2019 19:23

povilasv approved these changes Aug 5, 2019

View reviewed changes

GiedriusS merged commit 4cf32d0 into thanos-io:master Aug 5, 2019

kakkoyun mentioned this pull request Sep 17, 2019

feat store: added readiness and livenes prober #1460

Merged

1 task

This was referenced Sep 18, 2019

downsample: Add liveness and readiness probe #1540

Merged

Add missing /-/healthy endpoint #644

Closed

mhyllander mentioned this pull request May 18, 2022

[prometheus-thanos] compactor missing liveness and readiness probes kiwigrid/helm-charts#448

Open

Conversation

FUSAKLA commented Jul 2, 2019

Changes

Verification

Uh oh!

FUSAKLA commented Jul 2, 2019

Uh oh!

GiedriusS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GiedriusS Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

FUSAKLA Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

FUSAKLA Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

GiedriusS commented Jul 2, 2019

Uh oh!

FUSAKLA commented Jul 2, 2019

Uh oh!

FUSAKLA commented Jul 2, 2019

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwplotka Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

FUSAKLA Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

FUSAKLA commented Jul 11, 2019

Uh oh!

povilasv left a comment

Choose a reason for hiding this comment

Uh oh!

FUSAKLA commented Jul 16, 2019

Uh oh!

povilasv commented Jul 23, 2019

Uh oh!

FUSAKLA commented Jul 25, 2019

Uh oh!

midnightconman commented Jul 25, 2019

Uh oh!

GiedriusS left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FUSAKLA commented Jul 29, 2019

Uh oh!

FUSAKLA commented Jul 30, 2019

Uh oh!

GiedriusS commented Jul 31, 2019

Uh oh!

povilasv commented Aug 5, 2019

Uh oh!

povilasv commented Aug 5, 2019

Uh oh!

FUSAKLA commented Aug 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GiedriusS left a comment •

edited

Loading