Change l4 healthcheck timeout #1828
Conversation
|
Hi @code-elinka. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
yeh, trash is hard to hold for 3 times consecutive |
|
We want to introduce this change only to services with external traffic policy local, so this needs a bit more changes |
374ab11 to
8b5e361
Compare
8b5e361 to
fc000d3
Compare
fc000d3 to
235301b
Compare
|
Wrong change. |
235301b to
0367e93
Compare
|
/unassign @bowei |
|
/assign @cezarygerard |
|
/uncc @bowei |
|
/cc @cezarygerard @cezarygerard Wow, that's really nice that I did not push the change earlier and did not ask you to review. I made a stupid mistake, read Slavik comment in a wrong way and added new consts for XLB, not ExternalTrafficPolicies=Local 😀 |
|
/ok-to-test |
0367e93 to
168fc9c
Compare
168fc9c to
8b7aba1
Compare
8b7aba1 to
aa0136f
Compare
aa0136f to
ae443dc
Compare
According to l4 healthcheck constants the default set up has timeout = 8 seconds and threshold = 3 times. As result it can lead to 24 (8*3) seconds of lost requests. This change is supposed to decrease this time to only 6 seconds for sharedHc=false. Signed-off-by: Elina Akhmanova <elinka@google.com>
ae443dc to
22247d2
Compare
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cezarygerard, code-elinka 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 |
| wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold + 1, | ||
| }, | ||
| } | ||
| for _, tc := range testCases { | ||
| t.Run(tc.desc, func(t *testing.T) { | ||
| // healthcheck intervals and thresholds are common for Global and Regional healthchecks. Hence testing only Global case. | ||
| wantHC := newL4HealthCheck("hc", types.NamespacedName{Name: "svc", Namespace: "default"}, false, "/", 12345, utils.ILB, meta.Global, "") |
There was a problem hiding this comment.
this doesn't exercise shared/local that is the thing we just changed,no?
| @@ -27,7 +27,7 @@ import ( | |||
|
|
|||
| func TestMergeHealthChecks(t *testing.T) { | |||
| t.Parallel() | |||
| for _, tc := range []struct { | |||
| testCases := []struct { | |||
There was a problem hiding this comment.
if we add a shared bool var here we could this new dimension
There was a problem hiding this comment.
let me do that, seems a low hanging fruit I can use to participate here
This change adds changed l4 healthcheck timeouts to the v1.14 release. According to l4 healthcheck constants the default set up has timeout = 8 seconds and threshold = 3 times. As result it can lead to 24 (8*3) seconds of lost requests. This change is supposed to decrease this time to only 6 seconds for sharedHc=false. Signed-off-by: Elina Akhmanova <elinka@google.com>
[Cherrypick #1828] l4 healthcheck timeouts change
According to l4 healthcheck constants the default set up has timeout = 8 seconds and threshold = 3 times. As result it can lead to 24 (8*3) seconds of lost requests. This change is supposed to decrease this time to only 6 seconds for sharedHc=false.