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: add the buffer limit knob to the cluster #3390

Merged
merged 6 commits into from
May 21, 2024

Conversation

ShyunnY
Copy link
Contributor

@ShyunnY ShyunnY commented May 14, 2024

What type of PR is this?

api: add the buffer limit knob to the cluster

What this PR does / why we need it:

This PR aims to define the configuration structure of the per_connection_buffer_limit_bytes feature knob

Which issue(s) this PR fixes:

Related issues #3278

@ShyunnY ShyunnY requested a review from a team as a code owner May 14, 2024 02:00
Signed-off-by: ShyunnY <1147212064@qq.com>
@ShyunnY
Copy link
Contributor Author

ShyunnY commented May 14, 2024

/retest

Signed-off-by: ShyunnY <1147212064@qq.com>
Signed-off-by: ShyunnY <1147212064@qq.com>
Signed-off-by: ShyunnY <1147212064@qq.com>
Copy link

codecov bot commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.42%. Comparing base (c30d09f) to head (cf605ab).
Report is 34 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3390      +/-   ##
==========================================
+ Coverage   67.11%   67.42%   +0.30%     
==========================================
  Files         164      166       +2     
  Lines       23818    19184    -4634     
==========================================
- Hits        15986    12935    -3051     
+ Misses       6912     5320    -1592     
- Partials      920      929       +9     

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

Signed-off-by: ShyunnY <1147212064@qq.com>
@ShyunnY
Copy link
Contributor Author

ShyunnY commented May 20, 2024

By the way, when Connection exists in both CTP and BTP, we can abstract them both into api/v1alpha1/connection_types.go (only a small refactoring code is required).

The current connection_types.go only defines about CTP:

type Connection struct {
// ConnectionLimit defines limits related to connections
//
// +optional
ConnectionLimit *ConnectionLimit `json:"connectionLimit,omitempty"`
// BufferLimit provides configuration for the maximum buffer size in bytes for each incoming connection.
// For example, 20Mi, 1Gi, 256Ki etc.
// Note that when the suffix is not provided, the value is interpreted as bytes.
// Default: 32768 bytes.
//
// +kubebuilder:validation:XValidation:rule="type(self) == string ? self.matches(r\"^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$\") : type(self) == int",message="bufferLimit must be of the format \"^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$\""
// +optional
BufferLimit *resource.Quantity `json:"bufferLimit,omitempty"`

Can we structure it like this?

type struct BackendTrafficPolicyConnection {
  ...
}

type struct ClientTrafficPolicyConnection {
  ...
}

@ShyunnY ShyunnY requested review from arkodg, shawnh2 and zirain May 20, 2024 06:52
@shawnh2
Copy link
Contributor

shawnh2 commented May 20, 2024

prefer BackendConnection and ClientConnection

@ShyunnY
Copy link
Contributor Author

ShyunnY commented May 20, 2024

wow, this looks great, I vote!
In this PR we will focus on the design of the bufferLimit API, and I will open an additional PR to solve the reconstruction problem in the future.
@shawnh2

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 !

lets track the connection refactor with a separate GH issue

@arkodg arkodg requested review from a team May 20, 2024 16:56
@zirain
Copy link
Contributor

zirain commented May 21, 2024

/retest

@arkodg arkodg merged commit db90b07 into envoyproxy:main May 21, 2024
26 checks passed
@ShyunnY ShyunnY deleted the buffer-cluster-api branch May 22, 2024 03:41
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.

None yet

4 participants