Clarify startupProbe e2e tests #84291
Conversation
k8s-ci-robot
added
release-note-none
|
/sig node |
k8s-ci-robot
added
sig/node
k8s-ci-robot
added
area/test
sig/testing
test/e2e_node/startup_probe_test.go
Outdated
| */ | ||
| framework.ConformanceIt("should be restarted with a exec \"cat /tmp/health\" because startup probe does not delay it long enough [NodeConformance]", func() { | ||
| framework.ConformanceIt("should be restarted by liveness probe because startup probe does not delay it long enough [NodeConformance]", func() { |
There was a problem hiding this comment.
Shouldn't this container restart because startup probe fails?
There was a problem hiding this comment.
Yup, this test doesn't make sense...
k8s-ci-robot
added
the
lgtm
matthyx
force-pushed
the
startup_probe_test
branch
from
423a831
to
8588fa2
Compare
k8s-ci-robot
removed
the
lgtm
k8s-ci-robot
added
the
lgtm
|
/assign tallclair |
|
/assign @derekwaynecarr |
|
@tallclair that one was already assigned to you, thanks! |
|
@Random-Liu, @tallclair, @derekwaynecarr, @yujuhong, @ConnorDoyle this PR is really needed for 1.17, can you please make sure we don't miss code-freeze? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, matthyx 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 |
k8s-ci-robot
added
the
approved
|
/lgtm We should get this change in after #84279 |
|
/retest Review the full test history for this PR. Silence the bot with an |
| */ | ||
| framework.ConformanceIt("should *not* be restarted with a exec \"cat /tmp/health\" because startup probe delays it [NodeConformance]", func() { | ||
| framework.ConformanceIt("should be restarted startup probe fails [NodeConformance]", func() { |
There was a problem hiding this comment.
I'm very surprised these renames didn't require changing test/conformance/testdata/conformance.txt
@johnbelamaric, should this have touched that file?
There was a problem hiding this comment.
Hmm... looks like there are no files under test/e2e_node listed in conformance.txt. Are these actually considered conformance tests?
There was a problem hiding this comment.
Not sure... what's the requirement to consider them as conformance?
|
e2e_node does not run for conformance at all, so those are not really
conformance tests. In fact IIUC, tests in that directory do not run against
a full cluster but against some subset of components.
…On Tue, Nov 12, 2019 at 10:51 PM Matthias Bertschy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/e2e_node/startup_probe_test.go
<#84291 (comment)>
:
> */
- framework.ConformanceIt("should *not* be restarted with a exec \"cat /tmp/health\" because startup probe delays it [NodeConformance]", func() {
+ framework.ConformanceIt("should be restarted startup probe fails [NodeConformance]", func() {
Not sure... what's the requirement to consider them as conformance?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#84291?email_source=notifications&email_token=ACIHRM5FM3F3XKXY5XSTWATQTOPXVA5CNFSM4JETLHC2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLLDSVA#discussion_r345592415>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACIHRMYMFKC2ZJFDTNCAOXLQTOPXVANCNFSM4JETLHCQ>
.
|
|
Ok, understood. As stated here these tests will be relocated once the feature gate is no longer needed. |
|
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Make startupProbe e2e clearer to understand.
Does this PR introduce a user-facing change?: