Fix the "supress health checks when only one backing service" logic #17077
Conversation
This reverts commit 4833eb5. This logic is not correct. If a router is backed by two services, it will misbehave.
openshift-ci-robot
added
do-not-merge/work-in-progress
openshift-merge-robot
added
the
approved
knobunc
force-pushed
the
fix/health-supression
branch
from
e1704f5
to
aad81e1
Compare
openshift-merge-robot
removed
the
lgtm
knobunc
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
Ok, cleaned up the PR. The first commit is a complete reversion of my bad change. The second is just fixing a TODO, since I looked over the code and it is sane. The third is the correct change, and is the one that should be looked at in isolation to make better sense of the work. |
knobunc
force-pushed
the
fix/health-supression
branch
from
21df730
to
6ae67c3
Compare
|
/retest |
|
@openshift/networking PTAL (but see comment #17077 (comment)) |
There was a TODO asking for the code to be reviewed. I've done that, so I removed the TODO.
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. Fixes bug 1507664 (https://bugzilla.redhat.com/show_bug.cgi?id=1507664)
knobunc
force-pushed
the
fix/health-supression
branch
from
6ae67c3
to
9835dca
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knobunc, rajatchopra 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 |
|
/retest |
|
/kind bug |
openshift-ci-robot
added
the
kind/bug
There was a problem hiding this comment.
From the comment not sure what you are going for. The code seems to generate health check on a per service basis when the service has more than 1 endpoint. So if the route has 2 services, A and B, A has 1 endpoint and there is now health check for that service and B has 3 endpoints we would have health checks for that service. So if A and B both have 1 endpoint no health checks are needed for the route. Did I get that right?
Other than my fuzzy understanding this looks OK.
|
/retest |
|
Automatic merge from submit-queue. |
This reverts commit 4833eb5 and then fixes the implementation.
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.
Fixes bug 1507664 (https://bugzilla.redhat.com/show_bug.cgi?id=1507664)