Remove non-functional pre-stop hook parts #4801
Merged
Conversation
This file contains 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
sebgl
approved these changes
Aug 31, 2021
david-kow
approved these changes
Sep 1, 2021
| First, the PreStop lifecycle hook keeps querying DNS for `PRE_STOP_MAX_WAIT_SECONDS` (defaulting to 20) until the Pod IP is no longer referenced. | ||
| Then, it waits for an additional `PRE_STOP_ADDITIONAL_WAIT_SECONDS` (defaulting to 30). Additional wait is used to: | ||
| To address this issue and minimize unavailability, ECK relies on a link:https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/[PreStop lifecycle hook]. | ||
| It waits for an additional `PRE_STOP_ADDITIONAL_WAIT_SECONDS` (defaulting to 50). The additional wait time is used to: | ||
|
|
||
| 1. Give time to in-flight requests to be completed. | ||
| 2. Give clients time to use the terminating Pod IP resolved just before DNS record was updated. |
There was a problem hiding this comment.
Line 22 should say environment variable instead of environment variableS:
The exact behavior is configurable using environment variable, for example:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #4698
The pre-stop hook script intended to improve availability during rolling upgrades has not been doing what it is advertising to do since ECK 1.3.0.
cloud-on-k8s/pkg/controller/elasticsearch/nodespec/statefulset.go
Line 51 in 72bfb73
We are including non-ready Pods in the headless service which we then try to use to determine when a terminating Pod has been removed from DNS (never, in this constellation). We have since added functionality in #3837 that relies on this setting for client-side node discovery (aka sniffing).
This means that our pre-stop hook was always running the full 20+30 secs before terminating the Pod. I see two potential fixes for this problem:
I have opted for sticking with 50 seconds of wait time (but I am open to cutting this down)
NOTE: I also kept the
terminationGracePeriodat 180s to avoid a rolling restart on ECK upgrade.