Skip to content
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

Always pass Healthy dests to the throttler #5466

Merged
merged 1 commit into from Sep 10, 2019

Conversation

@vagababov
Copy link
Contributor

vagababov commented Sep 10, 2019

TL;DR: This is needed for future "ideal" load balancing in throttler wherein I
will need the actual pods to send traffic to -- so always send the ready
pod IPs. But this doubles as ReadyPodCount, so remove that.

As discussed with Greg, but for posterity:

Right now we know everything that is in dests is healthy. So when we switch to clusterIP
we can forward all the pods there for endpoints count reasons.

In future when we will have both healthy and unhealthy pods passed into the
we can combine and probe that combined list and send all the healthy ones to the throttler as we do now.

In any case backendCount==|dests|.

In theory we might probe unready addresses even when we're using cluster IP since we're delegating loadbalancing decision to the throttler,
but this might break the capacity computations, since there might be more pods to use than cluster IP can reach, so this might not be
a good initial approach, but a future enhancement.

/assign @greghaynes @markusthoemmes mattmoor

/lint

TL;DR: This is needed for future "ideal" load balancing in throttler wherein I
will need the actual pods to send traffic to -- so always send the ready
pod IPs. But this doubles as `ReadyPodCount`, so remove that.

As discussed with Greg, but for posterity:

Right now we know everything that is in dests is healthy. So when we switch to clusterIP
we can forward all the pods there for endpoints count reasons.

In future when we will have both healthy and unhealthy pods passed into the
we can combine and probe that combined list and send all the healthy ones to the throttler as we do now.

In any case backendCount==|dests|.

In theory we might probe unready addresses even when we're using cluster IP since we're delegating loadbalancing decision to the throttler,
but this might break the capacity computations, since there might be more pods to use than cluster IP can reach, so this might not be
a good initial approach, but a future enhancement.

/assign @greghaynes @markusthoemmes mattmoor
Copy link

knative-prow-robot left a comment

@vagababov: 0 warnings.

In response to this:

TL;DR: This is needed for future "ideal" load balancing in throttler wherein I
will need the actual pods to send traffic to -- so always send the ready
pod IPs. But this doubles as ReadyPodCount, so remove that.

As discussed with Greg, but for posterity:

Right now we know everything that is in dests is healthy. So when we switch to clusterIP
we can forward all the pods there for endpoints count reasons.

In future when we will have both healthy and unhealthy pods passed into the
we can combine and probe that combined list and send all the healthy ones to the throttler as we do now.

In any case backendCount==|dests|.

In theory we might probe unready addresses even when we're using cluster IP since we're delegating loadbalancing decision to the throttler,
but this might break the capacity computations, since there might be more pods to use than cluster IP can reach, so this might not be
a good initial approach, but a future enhancement.

/assign @greghaynes @markusthoemmes mattmoor

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-metrics-robot

This comment has been minimized.

Copy link

knative-metrics-robot commented Sep 10, 2019

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/activator/net/revision_backends.go 88.4% 88.3% -0.1
@knative-test-reporter-robot

This comment has been minimized.

Copy link

knative-test-reporter-robot commented Sep 10, 2019

The following jobs failed due to test flakiness:

Test name Triggers Retries
pull-knative-serving-integration-tests pull-knative-serving-integration-tests
pull-knative-serving-integration-tests
2/3

Automatically retrying...
/test pull-knative-serving-integration-tests

Copy link
Contributor

markusthoemmes left a comment

/lgtm
/approve

/hold
To give @greghaynes a chance to have a look as well.

@knative-prow-robot

This comment has been minimized.

Copy link

knative-prow-robot commented Sep 10, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, vagababov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

mattmoor left a comment

/lgtm

@vagababov

This comment has been minimized.

Copy link
Contributor Author

vagababov commented Sep 10, 2019

Well, I want this to soak in in case there are unexpected problems and we discussed with Greg this change, so I'm gonna
/hold cancel
and if there are issues we can send a followup change.

@greghaynes

This comment has been minimized.

Copy link
Member

greghaynes commented Sep 10, 2019

/lgtm

@vagababov

This comment has been minimized.

Copy link
Contributor Author

vagababov commented Sep 10, 2019

/test pull-knative-serving-unit-tests

@knative-prow-robot knative-prow-robot merged commit 8d80ff1 into knative:master Sep 10, 2019
8 checks passed
8 checks passed
cla/google All necessary CLAs are signed
pull-knative-serving-build-tests Job succeeded.
Details
pull-knative-serving-go-coverage Job succeeded.
Details
pull-knative-serving-integration-tests Job succeeded.
Details
pull-knative-serving-smoke-tests Job succeeded.
Details
pull-knative-serving-unit-tests Job succeeded.
Details
pull-knative-serving-upgrade-tests Job succeeded.
Details
tide In merge pool.
Details
@vagababov vagababov deleted the vagababov:always-pass-dests branch Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.