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

api: support upstream max requests per connection #2513

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

guydc
Copy link
Contributor

@guydc guydc commented Jan 26, 2024

What this PR does / why we need it:
Adding MaxRequestsPerConnection to CircuitBreaker, based on discussion in #2508. The implementation will translate the new attribute to the appropriate cluster's common http protocol options.

Which issue(s) this PR fixes:
Fixes #2508

@guydc guydc requested a review from a team as a code owner January 26, 2024 23:55
@@ -7,14 +7,24 @@ package v1alpha1

// CircuitBreaker defines the Circuit Breaker configuration.
type CircuitBreaker struct {
// Settings related to both TCP and HTTP upstream connections
Copy link
Contributor

@arkodg arkodg Jan 27, 2024

Choose a reason for hiding this comment

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

Suggested change
// Settings related to both TCP and HTTP upstream connections
// Settings related to TCP upstream connections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TCP settings like MaxConnections will apply to both TCP (e.g. TLS Passthrough) and HTTP connections. The HTTP struct contains settings that are relevant strictly to HTTP connections.

Copy link
Contributor

Choose a reason for hiding this comment

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

in that case, the top level tcp and http doesnt seem very useful here and can be flattened into one, wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem. I'll update the API PR accordingly.

@guydc guydc force-pushed the api-max-requests-per-connection branch from f35369a to 60a53f6 Compare January 29, 2024 21:46
@guydc guydc changed the title feat(translator): support upstream max requests per connection api: support upstream max requests per connection Jan 29, 2024
@guydc
Copy link
Contributor Author

guydc commented Jan 29, 2024

/retest

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f1a0a42) 64.64% compared to head (f2e010e) 64.64%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2513   +/-   ##
=======================================
  Coverage   64.64%   64.64%           
=======================================
  Files         116      116           
  Lines       17797    17797           
=======================================
  Hits        11505    11505           
  Misses       5556     5556           
  Partials      736      736           

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

@@ -30,4 +30,11 @@ type CircuitBreaker struct {
// +kubebuilder:default=1024
// +optional
MaxParallelRequests *int64 `json:"maxParallelRequests,omitempty"`

// The maximum number of requests that Envoy will make over a single connection to the referenced backend defined within a xRoute rule.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

can you highlight the default in the doc string ?

Signed-off-by: Guy Daich <[email protected]>
@guydc guydc force-pushed the api-max-requests-per-connection branch from cddd4e6 to 383d73b Compare January 29, 2024 22:38
Signed-off-by: Guy Daich <[email protected]>
@guydc
Copy link
Contributor Author

guydc commented Jan 29, 2024

/retest

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 !

@arkodg arkodg requested review from a team January 29, 2024 23:48
@zirain zirain merged commit 236cba5 into envoyproxy:main Jan 30, 2024
20 checks passed
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.

Support Max Requests Per Connection in Backend Traffic Policy
3 participants