Change GCE LB health check interval from 2s to 8s #70099
Conversation
|
/priority critical-urgent |
grayluck
force-pushed
the
five-sec-hc
branch
2 times, most recently
from
301d5dd to
250c4b3
Compare
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
sig/cloud-provider
|
/remove-sig cloud-provider |
|
/sig network |
k8s-ci-robot
added
cncf-cla: yes
|
e2e test passed. e2e test log: Seeing health check interval being reconciled to 5 sec. |
test/e2e/network/service.go
Outdated
There was a problem hiding this comment.
I think this needs to be [serial] no? Otherwise, you might have the healthcheck recreated by another test running at the same time?
There was a problem hiding this comment.
You're right. This test might be disrupted.
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
There was a problem hiding this comment.
We should be reducing the gceHcUnhealthyThreshold to 3, otherwise it will be very unresponsive.
There was a problem hiding this comment.
Let's increase this to 8 s.
Then we have 3 * 8 = 24 s to notice a VM is down.
There was a problem hiding this comment.
I think we probably want to add logic to keep higher values for the parameters on the healthcheck if the user has increased the value outside of Kubernetes. This gives the user a way out if their cluster is being impacted by the healthcheck volume.
There was a problem hiding this comment.
Logic added. Unit test also added to guard the logic.
k8s-ci-robot
removed
the
size/M
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Reconcile HealthCheck interval to be >= the new default interval.
E.g. old health check interval is 2s, new one is 8. Reconcile to 8.
If the existing health check > the default interval, keep the existing health check.
There was a problem hiding this comment.
Changed to needToUpdateHttpHealthChecks
There was a problem hiding this comment.
if needToUpdateHealthChecks(old, new) {
newHC := mergeHealthCheck(old, new)
update(newHC)
}
There was a problem hiding this comment.
make it consistent with external LB
if needToUpdateHealthChecks(old, new) {
newHC := mergeHealthCheck(old, new)
update(newHC)....
}
|
just nits. |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
…d to 3 Force ELB to ensureHealthCheck when target pool exists. Add e2e test to ensure that HC interval will be reconciled when kube-controller-manager restarts. Health checks with bigger thresholds and larger intervals will not be reconciled. Add unittest for ILB and ELB to ensure HC reconciles and is configurable.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/test pull-kubernetes-integration |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, grayluck The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@grayluck: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
…99-upstream-release-1.11 Automated cherry pick of #70099
…99-upstream-release-1.12 Automated cherry pick of #70099: Change GCE LB health check interval from 2s to 8s,
…99-upstream-release-1.10 Automated cherry pick of #70099
5 participants
Let ELB always ensure HttpHealthCheck.
e2e test included to test whether health check interval will be
reconciled when kube-controller-manager restarts.
What type of PR is this?
What this PR does / why we need it:
The CPU and network overhead for health check with 2s interval which overruns the cluster and make nodes unhealthy. HC check interval 2s -> 5s is a relief to the traffic and decrease the QPS by 2.5x.
Special notes for your reviewer:
/assign @bowei
Does this PR introduce a user-facing change?: