Tweak probing retry timings.

[JRBANCEL]

Fixes #8054

Context

Until now, the probing is using the default rate limiter workqueue.DefaultControllerRateLimiter:

• 10 QPS
• Exponential back-off with a 5 ms base (10, 20, 40, 80, etc...)

In the most optimal scenario (single node, no load, 1 Envoy Pod, 1 Knative Service), it takes at least 150 ms for Istio to apply a VirtualService change. The timeline of probing is (because we mistakenly call AddRateLimited the first time enqueueing so the rate limiting is applied):

• t=10 ms : 🔴
• t=30 ms : 🔴
• t=70 ms : 🔴
• t=150 ms : 🟢

Proposed Changes

• Do not use AddRateLimited when enqueueing the first time (i.e. only throttle retries)
• Add a 200 ms delay when enqueueing the first time (trying before the ~150 ms Istio lower bound is pointless)
• Increase the base of the back-off from 5 ms to 50 ms (5 ms is super aggressive)
• Increase the QPS limit to 50 QPS (it is limited to 10 go-routines anyway)

Result

In the optimal scenario, only a single request is now necessary.
In other scenarios, more requests can be executed by unit of time while per VirtualService fewer requests will be executed.

/cc @yuzisun

[knative-prow-robot]

@JRBANCEL: GitHub didn't allow me to request PR reviews from the following users: yuzisun.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Fixes #8054

Context

Until now, the probing is using the default rate limiter workqueue.DefaultControllerRateLimiter:

• 10 QPS
• Exponential back-off with a 5 ms base (10, 20, 40, 80, etc...)

In the most optimal scenario (single node, no load, 1 Envoy Pod, 1 Knative Service), it takes ~150 ms for Istio to apply a VirtualService change. The timeline of probing is (because we mistakenly call AddRateLimited the first time enqueueing so the rate limiting is applied):

• t=10 ms : 🔴
• t=30 ms : 🔴
• t=70 ms : 🔴
• t=150 ms : 🟢

Proposed Changes

• Do not use AddRateLimited when enqueueing the first time (i.e. only throttle retries)
• Add a 200 ms delay when enqueueing the first time (trying before the ~150 ms Istio lower bound is pointless)
• Increase the base of the back-off from 5 ms to 50 ms (5 ms is super aggressive)
• Increase the QPS limit to 50 QPS (it is limited to 10 go-routines anyway)

Result

In the optimal scenario, only a single request is now necessary.
In other scenarios, more requests can be executed simultaneously while per VirtualService fewer requests will be executed.

/cc @yuzisun

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-prow-robot]

@JRBANCEL: 0 warnings.

In response to this:

Fixes #8054

Context

Until now, the probing is using the default rate limiter workqueue.DefaultControllerRateLimiter:

• 10 QPS
• Exponential back-off with a 5 ms base (10, 20, 40, 80, etc...)

In the most optimal scenario (single node, no load, 1 Envoy Pod, 1 Knative Service), it takes ~150 ms for Istio to apply a VirtualService change. The timeline of probing is (because we mistakenly call AddRateLimited the first time enqueueing so the rate limiting is applied):

• t=10 ms : 🔴
• t=30 ms : 🔴
• t=70 ms : 🔴
• t=150 ms : 🟢

Proposed Changes

• Do not use AddRateLimited when enqueueing the first time (i.e. only throttle retries)
• Add a 200 ms delay when enqueueing the first time (trying before the ~150 ms Istio lower bound is pointless)
• Increase the base of the back-off from 5 ms to 50 ms (5 ms is super aggressive)
• Increase the QPS limit to 50 QPS (it is limited to 10 go-routines anyway)

Result

In the optimal scenario, only a single request is now necessary.
In other scenarios, more requests can be executed simultaneously while per VirtualService fewer requests will be executed.

/cc @yuzisun

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.

[ZhiminXiang]

/lgtm
/approve

[vagababov]

Need to update the go.mod.

[knative-test-reporter-robot]

The following jobs failed:

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

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

test/e2e/istio.TestClusterLocalAuthorization
test/e2e/istio.TestIstioProbing
test/conformance/api/v1.TestUpdateConfigurationMetadata
test/conformance/api/v1.TestContainerErrorMsg
test/conformance/api/v1.TestContainerExitingMsg
test/conformance/api/v1.TestTranslation
test/conformance/api/v1.TestServiceAccountValidation
test/conformance/api/v1.TestAnnotationPropagation

and 46 more.

[knative-metrics-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/network/status/status.go	98.4%	98.4%	0.1

[JRBANCEL]

@ZhiminXiang please re-approve, I fixed go.mod.

[ZhiminXiang]

/lgtm
/approve

[ZhiminXiang]

/assign @tcnghia

[tcnghia]

/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JRBANCEL, tcnghia, ZhiminXiang

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:

• OWNERS [tcnghia]

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

[tcnghia]

/test pull-knative-serving-upgrade-tests

@chizhg do @JRBANCEL need to rebase with master to get my change #8158 ?

[chizhg]

/test pull-knative-serving-upgrade-tests

@chizhg do @JRBANCEL need to rebase with master to get my change #8158 ?

The PR is now ready to be merged, so Tide will merge it with HEAD of master and retrigger the tests, so the tests could pass. But if the PR is not in the merge pool, the tests will be directly run against the commit in the PR.
It's a lightly weird behavior for Prow..