-
Notifications
You must be signed in to change notification settings - Fork 347
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: support Health Check in BackendTrafficPolicy #2244
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2244 +/- ##
==========================================
- Coverage 64.64% 64.60% -0.04%
==========================================
Files 115 115
Lines 16844 17199 +355
==========================================
+ Hits 10888 11112 +224
- Misses 5260 5382 +122
- Partials 696 705 +9 ☔ View full report in Codecov by Sentry. |
1fbb912
to
94cc6e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @lemonlinger , it's better to add some e2e test and user facing doc in the following PR.
api/v1alpha1/healthcheck_types.go
Outdated
// +kubebuilder:validation:Minimum=1 | ||
HealthyThreshold uint32 `json:"healthyThreshold"` | ||
|
||
// HealthChecker defines the concrete health checker to do health checking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a better name here, it currently looks like
healthCheck:
healthChecker:
.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about checker
?
healthCheck:
checker:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on endpoint ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel a little bit confused. Any thoughts from others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL #1821 the healthChecking
part as a reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about removing healthChecking
and copying the fields directly into top level struct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this ?
healthChecking: # optional
connectionTimeout: duration # optional
healthyCheckInterval: duration # optional, default=5s
unhealthyCheckInterval: duration, # optional, default=10s
unhealthyThreshold: int # optional, default=3
healthyThreshold: int # optional, default=1
type: Enum (grpc/http)
grpc: # required when type=grpc
upstreamName: string # required
authority: string # optional
http: # required when type=http
expectedStatuses: # required, minItems=1
- min: int # min=100, max=599
max: int # min=100, max=599
path: string # optional
hostname: string # optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup looking good !
- prefer "healthCheck" as the top level noun
- lets only support tcp and http for now, and add support for grpc later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline heathChecker to heathCheck and remove grpc checker.
done
api/v1alpha1/healthcheck_types.go
Outdated
Method string `json:"method,omitempty" yaml:"method,omitempty"` | ||
// ExpectedStatuses defines a list of HTTP response statuses considered healthy. | ||
// +optional | ||
ExpectedStatuses []Int64Range `json:"expectedStatuses,omitempty" yaml:"expectedStatuses,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
range doesnt seem like the common case for me, prefer a list of ints
e.g. []int{200, 201}
curious what others think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that using range provides more clarity and understanding compared to an array of integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for @lemonlinger, since an arrry of intergers will introduce length check, not a fun
this design is also corresponding to https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/v3/range.proto#envoy-v3-api-msg-type-v3-int64range
/retest |
Sure, will do. |
2c7203a
to
bb329db
Compare
/retest |
1 similar comment
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @lemonlinger, will start reviewing this week.
1b90fcc
to
9c234ab
Compare
9c234ab
to
ab72867
Compare
/retest |
Hey @AliceProxy, would you like to take a review on this ? |
ab72867
to
5cec58b
Compare
/retest |
1 similar comment
/retest |
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
f4e7087
to
18511d2
Compare
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
cffba8b
to
a431627
Compare
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
a431627
to
7e4022e
Compare
/retest |
@lemonlinger can you address the comments ? |
Sure thing. I'll get it done today. |
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks !
can you also raise a separate GH issue for E2E and docs
Sure thing. |
What type of PR is this?
What this PR does / why we need it:
This PR introduces active health checking to BackendTrafficPolicy, supporting 3 types of health checkers: HTTP, gRPC, and TCP.
Which issue(s) this PR fixes:
relates to #2126