Bug/bz1507664 suppress health checks

[knobunc]

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)

[knobunc]

/test

@openshift/networking PTAL

[rajatchopra]

Looks right, finally.

[imcsk8]

LGTM

[pecameron]

do you need the len(alias.PreferPort) == 0 to avoid a segfault?

[pravisankar]

@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

[knobunc]

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.

[pravisankar]

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.

[pravisankar]

Assuming we want to ignore the service unit if the corresponding weight is invalid.

[pravisankar]

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?

[knobunc]

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> .

[pravisankar]

LGTM

[pravisankar]

/lgtm
/approve

[openshift-merge-robot]

[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:

• images/router/OWNERS [knobunc]
• pkg/router/OWNERS [knobunc]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[knobunc]

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> .

You commented on the reversion PR... that behavior was working before my most recent changes.

[knobunc]

/test all

[knobunc]

/retest

[knobunc]

/test extended_conformance_gce

[knobunc]

/retest

Flaked on #16972

[openshift-merge-robot]

Automatic merge from submit-queue.