HAProxy Router: Invert health-check idled check #10596
Conversation
|
Since this regressed the product, please add an integration or e2e test that validates this is correct so it does not break in the future. |
|
cc @knobunc |
|
[test] |
DirectXMan12
force-pushed
the
bug/reencrypt-unidling-fixed
branch
from
7d3dd11 to
e8454b6
Compare
|
Alright, I had an extended test which could potentially have been a bit flaky, but I've changed it so that it should not be (the default health check interval is 5 seconds, and I've up the "consistently" time period to 20 seconds). However, I do not think that extended test was running -- I just looked through a Jenkins result, and only saw networking and conformance extended tests being run. @smarterclayton do we not run most of the extended tests? If that's the case, should I add a new section to the e2e test, or what? |
|
You should add [conformance] to your test. If you have fast, core
function tests, always add conformance preemptively. If your tests
aren't fast, make them fast.
|
|
@smarterclayton Define fast? This is testing a negative. So the test has to sleep to make sure something doesn't happen within a reasonable amount of time. |
|
@smarterclayton these tests are somewhat necessarily slow (they run in about 43s per for the "idling only" on my machine) because we have to wait for endpoints to come up (may take a few seconds), and then wait at least 10s (currently 20s to be safe) in order to make sure we've gone through what would be 1 health check interval (5s) if the health checks were running, checking to make sure we don't get any pods coming up. I'm assuming anything reported as slow by the test runner is too slow to be marked as conformance, right? |
|
The change LGTM. |
|
43 seconds is not too slow. The test runner doesn't know what slow is really. Slow would be 2 minutes probably. |
|
@smarterclayton ack, I'll stick conformance on the appropriate ones. |
DirectXMan12
force-pushed
the
bug/reencrypt-unidling-fixed
branch
from
e8454b6 to
be3ba8c
Compare
|
I've marked the entire idling test suite (~5-6m total) as "[Conformance]". If that's too much, I can just mark a couple of tests (which would be ~1m30s) that cover this case and basic unidling instead. |
|
[test] |
|
your budget is 2 minutes |
|
ack, I'll just make that two then |
DirectXMan12
force-pushed
the
bug/reencrypt-unidling-fixed
branch
from
be3ba8c to
b08af4a
Compare
|
alright, the basic idling with DC and basic unidling with TCP are now marked as conformance |
The check for no endpoints got inverted at some point during the PR, causing health checks to be enabled *only* when a service was idled, instead of the other way around. This fixes that. Fixes bug 1366180
|
[test] |
|
Evaluated for origin test up to b08af4a |
|
LGTM [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8383/) (Image: devenv-rhel7_4914) |
|
Evaluated for origin merge up to b08af4a |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8373/) |
|
Looks like a flake on the DC deployment conformance tests:
|
|
Please link the appropriate flake issue. On Tue, Aug 23, 2016 at 4:37 PM, Solly Ross [email protected]
|
4 participants
The check for no endpoints got inverted at some point during the PR,
causing health checks to be enabled only when a service was idled,
instead of the other way around. This fixes that.
Fixes bug 1366180