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.

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

feat(infrastructure/proxy): add readiness probes in envoy proxy deployments #1568

Merged
merged 5 commits into from
Jun 26, 2023

Conversation

psinghal20
Copy link
Contributor

What type of PR is this?

This PR adds readiness probes in envoy proxy deployments to prevent pods in a not-ready state to receive traffic.

What this PR does / why we need it:
This allows envoy proxy configurations and in-place upgrades without traffic drops using the configured rollout strategy.

Which issue(s) this PR fixes:

Fixes #1567

To access the admin APIs through the readiness probe, I had to change the listener admin address to 0.0.0.0 from 127.0.0.1 to allow the envoy to listen on other network interfaces as well. Let me know if this needs to be changed.

This commit adds readiness probes in envoy proxy deployments to prevent pods in not ready state to receive traffic. This allows envoy proxy configurations and in-place upgrades without traffic drops using the configured rollout strategy.

Signed-off-by: Pratyush Singhal <psinghal20@gmail.com>
@psinghal20 psinghal20 requested a review from a team as a code owner June 20, 2023 07:46
…ootstrap config

Signed-off-by: Pratyush Singhal <psinghal20@gmail.com>
Signed-off-by: Pratyush Singhal <psinghal20@gmail.com>
@psinghal20
Copy link
Contributor Author

Hi @arkodg @zirain, I have updated the PR. I have opted for a similar approach to emissary-ingress here.

I have added a new listener on 0.0.0.0 interface on 19001 port. Let me know if any change is required for the ready port or listener address.

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #1568 (d653df9) into main (75e7ada) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1568      +/-   ##
==========================================
+ Coverage   61.73%   61.84%   +0.10%     
==========================================
  Files          81       81              
  Lines       12182    12190       +8     
==========================================
+ Hits         7521     7539      +18     
+ Misses       4191     4182       -9     
+ Partials      470      469       -1     
Impacted Files Coverage Δ
...ternal/infrastructure/kubernetes/proxy/resource.go 92.85% <100.00%> (+0.32%) ⬆️

... and 2 files with indirect coverage changes

address: {{ .ReadyServer.Address }}
port_value: {{ .ReadyServer.Port }}
protocol: TCP
filter_chains:
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

as well as the per connection setting

PerConnectionBufferLimitBytes: wrapperspb.UInt32(tcpListenerPerConnectionBufferLimitBytes),

Copy link
Contributor

Choose a reason for hiding this comment

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

that will be too noise

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah understand where you are coming from, its fine to leave it as is. My reasoning for adding it was from a correctness/visibility perspective where user wants to exactly know what requests are being made to the proxy to better correlate cpu/system consumption etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

@psinghal20 can you incorporate the PerConnectionBufferLimitBytes setting into the config, to reduce buffer space, tia !

@arkodg arkodg requested review from zirain and arkodg June 22, 2023 16:13
Copy link
Contributor

@LanceEa LanceEa left a comment

Choose a reason for hiding this comment

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

small comment, other than that it looks good to me.

internal/xds/bootstrap/bootstrap.go Show resolved Hide resolved
Copy link
Member

@qicz qicz left a comment

Choose a reason for hiding this comment

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

LGTM

@Xunzhuo Xunzhuo merged commit c3557bc into envoyproxy:main Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add readinessProbes for envoy proxy deployment
6 participants