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

xds: move ready to dynamic listener #5079

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zirain
Copy link
Member

@zirain zirain commented Jan 16, 2025

fixes: #2810

@zirain zirain requested a review from a team as a code owner January 16, 2025 05:18
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 66.86%. Comparing base (7813ef0) to head (2a5a127).

Files with missing lines Patch % Lines
internal/gatewayapi/listener.go 60.00% 11 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5079      +/-   ##
==========================================
- Coverage   66.93%   66.86%   -0.08%     
==========================================
  Files         210      211       +1     
  Lines       32984    33228     +244     
==========================================
+ Hits        22079    22217     +138     
- Misses       9570     9655      +85     
- Partials     1335     1356      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zirain
Copy link
Member Author

zirain commented Jan 16, 2025

wait #5117

@@ -86,6 +83,7 @@ type bootstrapParameters struct {
// EnablePrometheusCompression defines whether to enable HTTP compression on metrics endpoint for prometheus.
EnablePrometheusCompression bool
Copy link
Contributor

Choose a reason for hiding this comment

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

should we delete fields no longer used ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I recall I addressed this with a TODO for following refactor.

@guydc
Copy link
Contributor

guydc commented Jan 21, 2025

Do we want to maybe keep prom stats in bootstrap config and serve readiness on a different port to avoid breaking scraping?
I think that metrics have some value even if the proxy is not ready, e.g. they can reveal issues in the proxy subscribing for XDS with EG, handling the bootstrap config, etc.

@zirain
Copy link
Member Author

zirain commented Jan 21, 2025

Do we want to maybe keep prom stats in bootstrap config and serve readiness on a different port to avoid breaking scraping? I think that metrics have some value even if the proxy is not ready, e.g. they can reveal issues in the proxy subscribing for XDS with EG, handling the bootstrap config, etc.

IIUC, now they share the listener.

@arkodg arkodg modified the milestones: v1.3.0-rc.1, v1.3.0 Jan 21, 2025
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
@zirain zirain force-pushed the xds-ready-listener branch from 11f2104 to 2a5a127 Compare January 23, 2025 00:25
@zirain zirain modified the milestones: v1.3.0, Backlog Jan 23, 2025
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.

Improve EG Gateway xDS & startup Reliability (custom k8s health prob)
3 participants