cilium-health: Fix broken retry loop in cilium-health-ep controller
#31622
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
gandro
added
release-note/bug
This commit fixes a bug in the `cilium-health-ep` controller restart
logic where it did not give the cilium-health endpoint enough time to
startup before it was re-created.
For context, the `cilium-health-ep` performs two tasks:
1. Launch the cilium-health endpoint when the controller is started
for the first time.
2. Ping the cilium-health endpoint, and if it does not reply, destroy
and re-create it.
The controller has a `RunInterval` of 60 seconds and a default
`ErrorRetryBaseDuration` of 1 second. This means that after launching
the initial cilium-health endpoint, we wait for 60 seconds before we
attempt to ping it. If that ping succeeds, we then keep pinging the
health endpoint every 60 seconds.
However, if a ping fails, the controller deletes the existing endpoint
and creates a new one. Because the controller then also returns an
error, it is immediately re-run after one second, because in the failure
case a controller retries with an interval of `consecutiveErrors *
ErrorRetryBaseDuration`.
This meant that after a failed ping, we deleted the unreachable
endpoint, recreated a new one, and after 1s would immediately try to
ping it. Because the newly launched endpoint will is unlikely to be
reachable after just one second (it requires a full endpoint
regeneration with BPF compilation), the `cilium-health-ep` logic would
declare the still starting endpoint as dead and re-create it. This loop
would continue endlessly, causing lots of unnecessary CPU churn, until
enough consecutive errors have happened for the wait time between launch
and the first ping to be long enough for a cilium-health endpoint to be
fully regenerated.
This commit attempts to fix the logic by not immediately killing a
unreachable health endpoint and instead waiting for three minutes to
pass before we attempt to try again. Three minutes should hopefully be
enough time for the initial endpoint regeneration to succeed.
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro
force-pushed
the
pr/gandro/fix-cilium-health-restart-logic
branch
from
e35b861
to
aad9783
Compare
|
The reason for back-porting is that this also causes CI flakes sometimes, as in CI the endpoint regeneration at startup can take up some time. For example: |
|
/test |
michi-covalent
approved these changes
Mar 27, 2024
maintainer-s-little-helper
bot
added
the
ready-to-merge
3 tasks
joamaki
added
backport-pending/1.13
joamaki
added
backport-pending/1.14
joamaki
added
backport-pending/1.15
github-actions
bot
removed
the
backport-pending/1.14
github-actions
bot
added
backport-done/1.14
brb
added a commit
that referenced
this pull request
Apr 11, 2024
Until #31622 has been released. Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb
added a commit
that referenced
this pull request
Apr 11, 2024
Until #31622 has been released. Signed-off-by: Martynas Pumputis <m@lambda.lt>
brb
added a commit
that referenced
this pull request
Apr 11, 2024
Until #31622 has been released. Signed-off-by: Martynas Pumputis <m@lambda.lt>
This was referenced Apr 11, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 11, 2024
Until #31622 has been released. Signed-off-by: Martynas Pumputis <m@lambda.lt>
This was referenced Apr 11, 2024
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.
This commit fixes a bug in the
cilium-health-epcontroller restart logic where it did not give the cilium-health endpoint enough time to startup before it was re-created.For context, the
cilium-health-epperforms two tasks:The controller has a
RunIntervalof 60 seconds and a defaultErrorRetryBaseDurationof 1 second. This means that after launching the initial cilium-health endpoint, we wait for 60 seconds before we attempt to ping it. If that ping succeeds, we then keep pinging the health endpoint every 60 seconds.However, if a ping fails, the controller deletes the existing endpoint and creates a new one. Because the controller then also returns an error, it is immediately re-run after one second, because in the failure case a controller retries with an interval of
consecutiveErrors * ErrorRetryBaseDuration.This meant that after a failed ping, we deleted the unreachable endpoint, recreated a new one, and after 1s would immediately try to ping it. Because the newly launched endpoint will is unlikely to be reachable after just one second (it requires a full endpoint regeneration with BPF compilation), the
cilium-health-eplogic would declare the still starting endpoint as dead and re-create it. This loop would continue endlessly, causing lots of unnecessary CPU churn, until enough consecutive errors have happened for the wait time between launch and the first ping to be long enough for a cilium-health endpoint to be fully regenerated.This commit attempts to fix the logic by not immediately killing a unreachable health endpoint and instead waiting for three minutes to pass before we attempt to try again. Three minutes should hopefully be enough time for the initial endpoint regeneration to succeed.