Bug/bz1507664 suppress health checks #17155
Conversation
…ute" This reverts commit 9835dca.
knobunc
added
component/routing
kind/bug
|
/test @openshift/networking PTAL |
|
Looks right, finally. |
Previously we had attempted to suppress the health checks, but it was looking at the endpoints of the service, not the servers that back the route. The problem is that if we just look at endpoints, then if a route has two backing services, each with one endpoint, we would incorrectly suppress the health checks. This is the third attempt. I had missed a case in the last one, so now the logic is moved to be computed and recorded by the router controller, but used in the template. Fixes bug 1507664 (https://bugzilla.redhat.com/show_bug.cgi?id=1507664)
knobunc
force-pushed
the
bug/bz1507664-suppress-health-checks
branch
from
0d964b2
to
8cb7b95
Compare
|
LGTM |
| if len(alias.PreferPort) == 0 || endpoint.PortName == alias.PreferPort || endpoint.Port == alias.PreferPort { | ||
| for i := range svc.EndpointTable { | ||
| endpoint := svc.EndpointTable[i] | ||
| if endpoint.PortName == alias.PreferPort || endpoint.Port == alias.PreferPort { |
There was a problem hiding this comment.
do you need the len(alias.PreferPort) == 0 to avoid a segfault?
There was a problem hiding this comment.
@pecameron
No, len(alias.PreferPort) == 0 implies user did not specific any preferred port (http/https), then we don't need to filter any endpoints. That case is already handled in Line 160
There was a problem hiding this comment.
There are two commits in this PR. The first simply reverts my previous bad PR restoring the previous behavior. It probably makes sense to look at the second commit only.
openshift-merge-robot
added
the
approved
| if weight < 0 { | ||
| weight = 0 | ||
| } else if weight > 256 { | ||
| weight = 256 |
There was a problem hiding this comment.
When weight >= 256, don't we want to set it to 0?
Otherwise in Line 918, we count this serviceUnit toward active endpoints which doesn't seem right.
There was a problem hiding this comment.
Assuming we want to ignore the service unit if the corresponding weight is invalid.
There was a problem hiding this comment.
weight=0 clearly means user want to ignore the endpoint but weight > 256 means mostly some typo/calculation error and may not mean ignore the endpoint.
It may be useful to log a warning when we are defaulting invalid weights to 0 or 256?
|
In my mind, if you manage to get a weight that is later than the max in,
why would you disable that service?
…On Nov 2, 2017 5:59 PM, "Ravi Sankar Penta" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/router/template/router.go
<#17155 (comment)>:
> }
return serviceUnits
}
+// getServiceUnitWeight takes a reference to a weight and returns its value or the default.
+// It also checks that it is in the correct range.
+func getServiceUnitWeight(weightRef *int32) int32 {
+ // Default to 1 if there is no weight
+ var weight int32 = 1
+ if weightRef != nil {
+ weight = *weightRef
+ }
+
+ // Do a bounds check
+ if weight < 0 {
+ weight = 0
+ } else if weight > 256 {
+ weight = 256
Assuming we want to ignore the service unit if the corresponding weight is
invalid.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#17155 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJkavX0_zrq2iC-xX5p-SqbvX84KEW3Fks5syjsugaJpZM4QQMgg>
.
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knobunc, pravisankar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Yeah, that's a good idea. I'll add that in a follow-on PR
Thanks
…On Nov 2, 2017 8:01 PM, "Ravi Sankar Penta" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/router/template/router.go
<#17155 (comment)>:
> }
return serviceUnits
}
+// getServiceUnitWeight takes a reference to a weight and returns its value or the default.
+// It also checks that it is in the correct range.
+func getServiceUnitWeight(weightRef *int32) int32 {
+ // Default to 1 if there is no weight
+ var weight int32 = 1
+ if weightRef != nil {
+ weight = *weightRef
+ }
+
+ // Do a bounds check
+ if weight < 0 {
+ weight = 0
+ } else if weight > 256 {
+ weight = 256
weight=0 clearly means user want to ignore the endpoint but weight > 256
means mostly some typo/calculation error and may not mean ignore the
endpoint.
It may be useful to log a warning when we are defaulting invalid weights
to 0 or 256?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#17155 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJkavdlRkffe1owCo7aIN2vg_-LxWkv-ks5sylfPgaJpZM4QQMgg>
.
|
knobunc
dismissed
pecameron’s stale review
You commented on the reversion PR... that behavior was working before my most recent changes.
|
/test all |
|
/retest |
|
/test extended_conformance_gce |
|
/retest Flaked on #16972 |
openshift-ci-robot
added
the
size/L
|
Automatic merge from submit-queue. |
Fix the "supress health checks when only one backing service" logic
This reverts commit 9835dca because it is wrong, then the second commit implements the correct change.
Previously we had attempted to suppress the health checks, but it was looking at the endpoints of the service, not the servers that back the route. The problem is that if we just look at endpoints, then if a route has two backing services, each with one endpoint, we would incorrectly suppress the health checks.
This is the third attempt. I had missed a case in the last one, so now the logic is moved to be computed and recorded by the router controller, but used in the template.
Fixes bug 1507664 (https://bugzilla.redhat.com/show_bug.cgi?id=1507664)