fix: use correct probes - #101
Merged
Merged
Conversation
aaron-steinfeld
requested review from
a team,
avinashkolluru,
laxmanchekka,
ravisingal,
skjindal93 and
tim-mwangi
August 25, 2021 21:21
Codecov Report
@@ Coverage Diff @@
## main #101 +/- ##
=========================================
Coverage 81.59% 81.59%
Complexity 232 232
=========================================
Files 28 28
Lines 788 788
Branches 57 57
=========================================
Hits 643 643
Misses 97 97
Partials 48 48
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
tim-mwangi
reviewed
Aug 25, 2021
| failureThreshold: 2 | ||
| startupProbe: | ||
| periodSeconds: 5 | ||
| failureThreshold: 24 |
Contributor
There was a problem hiding this comment.
oh wow! is this a stop gap solution?
Contributor
Author
There was a problem hiding this comment.
No - why? I gave up to 2m to attempt start up. If it's quicker than that, with this config, it will short circuit.
tim-mwangi
approved these changes
Aug 26, 2021
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The main change in this PR is to replace our probing pattern. Previously (before 0.13.0) we used a static initial delay for liveness, by default 10s, upon which we began to check the tcp port for liveness. In addition, we had a readiness probe that checked the admin http server for readiness. This was close to right (the readiness probe meant the http server, not the grpc server had started - but close), but was problematic because
In other words, the behavior we really wanted was that of the liveness probe - the problem is that if failed on startup, we'll get into a crash loop - so it needs enough of a grace period to ensure a full startup. Unfortunately, the server can take wildly different times to startup depending on instrumentation and resource allocation. So we could change the startup delay to be really large, but then we're really setting a minimum startup time, rather than maximum.
In this PR, by using a startup probe we now have the following behavior (values are configurable but defaulted):
Immediately start checking if the grpc server is healthy every 5s on startup, continuing for up to 2m. Once the server is marked live, it can begin serving traffic and we'll check the status of its grpc server every 5s to make sure it's still live. If it fails 2 consecutive checks, we'll restart the pod.
Other changes:
Testing
I deployed this change to a local k8s cluster to verify correctness.