Add shutdown delay and e2e test#3395
Conversation
Signed-off-by: Joe Elliott <number101010@gmail.com>
joe-elliott
requested review from
annanay25,
electron0zero,
ie-pham,
mapno,
mdisibio,
stoewer,
yvrhdn and
zalegrala
as code owners
electron0zero
left a comment
electron0zero
left a comment
There was a problem hiding this comment.
one small Q, rest lgtm 👍🏼
| // confirm the readiness flag is down | ||
| res, err := e2e.DoGet("http://" + tempo.Endpoint(3200) + "/ready") | ||
| require.NoError(t, err) | ||
| require.Equal(t, http.StatusServiceUnavailable, res.StatusCode) |
There was a problem hiding this comment.
Perhaps perform a query before and after to confirm the change in behavior.
There was a problem hiding this comment.
it's not apparent, but this line waits until /ready returns 200:
There was a problem hiding this comment.
added a comment to help clarify that the /ready point is confirmed to be up before we stop tempo
| t.Server.SetKeepAlivesEnabled(false) | ||
|
|
||
| if t.cfg.ShutdownDelay > 0 { | ||
| time.Sleep(t.cfg.ShutdownDelay) |
There was a problem hiding this comment.
Do we also want to sleep when there are no requests in flight?
There was a problem hiding this comment.
i'm concerned about the complexity about that change. this is a very simple one that will do nearly all of what we want with some config choices in k8s
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
zalegrala
left a comment
zalegrala
left a comment
There was a problem hiding this comment.
Its a step in the right direction.
* Add shutdown delay and e2e test Signed-off-by: Joe Elliott <number101010@gmail.com> * changelog Signed-off-by: Joe Elliott <number101010@gmail.com> * determinism + comment Signed-off-by: Joe Elliott <number101010@gmail.com> * .Stop in a different goroutine then .Start creates a race Signed-off-by: Joe Elliott <number101010@gmail.com> --------- Signed-off-by: Joe Elliott <number101010@gmail.com>
3 participants
What this PR does:
Adds a shutdown delay configurable parameter modeled after mimir:
grafana/mimir#3298
After receiving SIGTERM Tempo will drop its readiness handler and wait the configured seconds. In a k8s environment this will allow Tempo to cleanly roll its frontend without dropping queries.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]