Make panic threshold configurable for cluster #5118
Conversation
|
|
||
| if policy.Spec.CommonLbSettings != nil { | ||
| if lbSettings, err = buildCommonLbSettings(policy.Spec.CommonLbSettings); err != nil { | ||
| err = perr.WithMessage(err, "RateLimit") |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5118 +/- ##
==========================================
+ Coverage 66.81% 66.90% +0.08%
==========================================
Files 210 210
Lines 32988 32998 +10
==========================================
+ Hits 22042 22077 +35
+ Misses 9604 9583 -21
+ Partials 1342 1338 -4 ☔ View full report in Codecov by Sentry. |
api/v1alpha1/shared_types.go
Outdated
| @@ -579,6 +579,22 @@ type ClusterSettings struct { | |||
| // | |||
| // +optional | |||
| HTTP2 *HTTP2Settings `json:"http2,omitempty"` | |||
|
|
|||
There was a problem hiding this comment.
my preference would be to add a PanicThreshold unit32 field inside HealthCheck supporting values between 0-100
gateway/api/v1alpha1/healthcheck_types.go
Line 17 in 0e52d06
internal/ir/xds.go
Outdated
| @@ -2456,6 +2459,12 @@ func (h *HealthCheck) Validate() error { | |||
| } | |||
| } | |||
|
|
|||
| if h.PanicThreshold != nil { | |||
| if *h.PanicThreshold < 0 || *h.PanicThreshold > 100 { | |||
There was a problem hiding this comment.
realistically it cannot be < 0 due to uint32 type
There was a problem hiding this comment.
Error: internal/ir/xds.go:2463:6: SA4003: no value of type uint32 is less than 0 (staticcheck)
if *h.PanicThreshold < 0 || *h.PanicThreshold > 100 {
CI is smart :)
| @@ -1421,6 +1439,7 @@ func TestValidateHealthCheck(t *testing.T) { | |||
| }, | |||
| }, | |||
| &OutlierDetection{}, | |||
| ptr.To[uint32](10), | |||
There was a problem hiding this comment.
I should be able just to set it to nil
|
there is a failure for conformance test |
There was a problem hiding this comment.
LGTM thanks !
as a follow up can you add a test case in https://github.com/envoyproxy/gateway/tree/main/internal/xds/translator/testdata/in/xds-ir
| @@ -106,6 +107,13 @@ func buildXdsCluster(args *xdsClusterArgs) *clusterv3.Cluster { | |||
| PerConnectionBufferLimitBytes: buildBackandConnectionBufferLimitBytes(args.backendConnection), | |||
| } | |||
|
|
|||
| // 50% is the Envoy default value for panic threshold. No need to explicitly set it in this case. | |||
There was a problem hiding this comment.
why is this set twice - here and below ?
There was a problem hiding this comment.
Good question, error while refactoring
Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Kateryna Nezdolii <[email protected]>
zirain
force-pushed
the
4015-panic-threshold
branch
from
a0de321 to
eb1631f
Compare
feat(api): Make panic threshold configurable per backend/cluster
Fixes #4015
Release Notes: TBD