Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

envoy: remove unnecessary wait and log message after starting envoy #24455

Merged
merged 1 commit into from Mar 22, 2023
Merged

envoy: remove unnecessary wait and log message after starting envoy #24455

merged 1 commit into from Mar 22, 2023

Conversation

mhofstetter
Copy link
Member

When the envoy proxy gets started by the Cilium Agent, the message Envoy: Sleeping for 100ms before restarting proxy gets logged.

With the introduction of supervising the Envoy Proxy in a dedicated go-routine (#2680), this message and the corresponding wait is somewhat unnecessary and gets deleted with this commit.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 20, 2023
@mhofstetter mhofstetter added the release-note/misc This PR makes changes that have no direct user impact. label Mar 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 20, 2023
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/envoy-rem-restartlog branch from 86fc994 to 3c730df Compare March 20, 2023 09:38
@mhofstetter mhofstetter marked this pull request as ready for review March 20, 2023 10:57
@mhofstetter mhofstetter requested a review from a team as a code owner March 20, 2023 10:57
@mhofstetter
Copy link
Member Author

/test

jrajahalme
jrajahalme requested changes Mar 22, 2023
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that since the introduction of the stopCh the restart loop will be exited after Envoy is intentionally stopped, so this wait is not necessary in that case. However, if there is a crash in Envoy startup this wait is necessary to limit the speed at which Envoy is started again, as in some cases Envoy would just crash again and then be perpetually restarted in a busy loop, hogging CPU resources.

How about moving the wait to be right after the "Envoy: Proxy crashed" log message, where a non-nil error was received, so that there would be some time before restart is tried in the outer goroutine?

@mhofstetter
Copy link
Member Author

Agree that since the introduction of the stopCh the restart loop will be exited after Envoy is intentionally stopped, so this wait is not necessary in that case. However, if there is a crash in Envoy startup this wait is necessary to limit the speed at which Envoy is started again, as in some cases Envoy would just crash again and then be perpetually restarted in a busy loop, hogging CPU resources.

How about moving the wait to be right after the "Envoy: Proxy crashed" log message, where a non-nil error was received, so that there would be some time before restart is tried in the outer goroutine?

thanks for pointing that out! might be really worth to only delete the "irritating" log message and move the actual wait to the point where we should wait after a crash of envoy.

@mhofstetter
envoy: remove unnecessary wait and log message after starting envoy
f068f5d 
When the envoy proxy gets started by the Cilium Agent, the
message `Envoy: Sleeping for 100ms before restarting proxy` gets logged.

With the introduction of supervising the Envoy Proxy in a dedicated
go-routine, this message and the corresponding wait is somewhat
unnecessary and gets deleted with this commit.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/envoy-rem-restartlog branch from 3c730df to f068f5d Compare March 22, 2023 13:12
@mhofstetter
Copy link
Member Author

mhofstetter commented Mar 22, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathVerifier Runs the kernel verifier against Cilium's BPF datapath

Failure Output

FAIL: terminating containers are not deleted after timeout

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

@mhofstetter
Copy link
Member Author

/test-1.25-4.19

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 22, 2023
@borkmann borkmann merged commit 0ec790a into cilium:master Mar 22, 2023
42 checks passed
@mhofstetter mhofstetter deleted the pr/mhofstetter/envoy-rem-restartlog branch March 22, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrajahalme jrajahalme jrajahalme approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mhofstetter @jrajahalme @borkmann