conformance: promote container exec probe timeout tests

[andrewsykim]

What type of PR is this?
/kind feature

What this PR does / why we need it:
In v1.20 we fixed exec probe timeouts (#94115). Along with that PR, we also un-skipped a test exercising this behavior with liveness probes and added a new test for readiness probes. In past discussions in SIG Node, there was general agreement that exec probe timeout tests should probably be included in conformance. #96694 added those tests to NodeConformance, this PR adds them to actual conformance.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[andrewsykim]

/assign @derekwaynecarr @dchen1107 @SergeyKanzhelev @johnbelamaric

[andrewsykim]

/triage accepted

[SergeyKanzhelev]

I'd suggest to change it to 1.20 since it is only fixed than. Keeping 1.9 is misleading

[johnbelamaric]

you can list both, to indicate the change in behavior in 1.20: v1.9, v1.20

[johnbelamaric]

once that change is in, I can approve

[SergeyKanzhelev]

/lgtm

I wonder if any positive test would be useful here. Like pod will stay alive if exec prob successful. It may be a slow test though, just thinking out loud.

[andrewsykim]

@SergeyKanzhelev I think we do have positive test cases, see this one for example

kubernetes/test/e2e/common/container_probe.go

Lines 133 to 148 in 2242d0f

	/*
	Release: v1.9
	Testname: Pod liveness probe, using local file, no restart
	Description: Pod is created with liveness probe that uses 'exec' command to cat /temp/health file. Liveness probe MUST not fail to check health and the restart count should remain 0.
	*/
	framework.ConformanceIt("should *not* be restarted with a exec \"cat /tmp/health\" liveness probe [NodeConformance]", func() {
	cmd := []string{"/bin/sh", "-c", "echo ok >/tmp/health; sleep 600"}
	livenessProbe := &v1.Probe{
	Handler: execHandler([]string{"cat", "/tmp/health"}),
	InitialDelaySeconds: 15,
	TimeoutSeconds: 5, // default 1s can be pretty aggressive in CI environments with low resources
	FailureThreshold: 1,
	}
	pod := busyBoxPodSpec(nil, livenessProbe, cmd)
	RunLivenessTest(f, pod, 0, defaultObservationTimeout)
	})

[SergeyKanzhelev]

@johnbelamaric can you please approve?

[dims]

I was part of the discussion in #94115, there was a change to add this to NodeConformance as well #96694 I think this is now ready for Conformance as we need to enforce this across all the CRI implementations (which we already confirmed with various impls)

So @johnbelamaric and @spiffxp please approve the changes to testdata @SergeyKanzhelev from sig-node has verified this as well.

[spiffxp]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, spiffxp

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:

• test/conformance/testdata/OWNERS [spiffxp]
• test/e2e/common/OWNERS [andrewsykim,spiffxp]

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

[spiffxp]

/test pull-kubernetes-integration
flake #98606

[SergeyKanzhelev]

/test pull-kubernetes-e2e-kind

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[SergeyKanzhelev]

/test pull-kubernetes-e2e-kind

[derekwaynecarr]

This test should not have been elevated into conformance as long as the feature gate to disable (and preserve the old buggy behavior) remained.

[derekwaynecarr]

We need to revert this PR prior to shipping 1.21.

[spiffxp]

Does the feature gate default to the old buggy behavior today (in v1.21)?

[SergeyKanzhelev]

Does the feature gate default to the old buggy behavior today (in v1.21)?

Feature gate defaults to a "fixed" state starting 1.20.

[derekwaynecarr]

To be more precise:

the test should be taken out of conformance
the tests should be updated so it skips if the feature gate is disabled
the test will not be allowed to move into conformance until the feature gate is removed

@spiffxp the present behavior defaults the feature gate to "on"

[SergeyKanzhelev]

#100035

[SergeyKanzhelev]

Also from @dims: #100036 =)

[spiffxp]

#99909 (comment) WDYT about this approach

[SergeyKanzhelev]

#99909 (comment) WDYT about this approach

the problem is not version skew, it is that AKS wants to run conformance tests while this flag is off. And they want the flag off for the same reason as we extended the removal of a feature flag - some customers may not be ready