Translate to tcp probe in queue-proxy when exec probe was used for readinessprobe in user-container #5712

nak3 · 2019-09-28T10:57:08Z

Proposed Changes

When users defined exec probe for readinessprobe, it becomes nil and
get unexpected error. To make matters worse, the pod never becomes ready.

This patch changes to translates exec probe in user-container to tcp
probe as per following code comment.

serving/pkg/queue/readiness/probe.go

Lines 65 to 66 in 3174fce

    
           // Should never be reachable. Exec probes to be translated to 
        
           // TCP probes when container is built.

/lint

Fixes #5711

Release Note

NONE

knative-prow-robot

@nak3: 0 warnings.

In response to this:

Proposed Changes

When users defined exec probe for readinessprobe, it becomes nil and
get unexpected error. To make matters worse, the pod never becomes ready.

This patch changes to translates exec probe in user-container to tcp
probe as per following code comment.

serving/pkg/queue/readiness/probe.go

Lines 65 to 66 in 3174fce

// Should never be reachable. Exec probes to be translated to

// TCP probes when container is built.

/lint

Fixes #5711

Release Note
NONE

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.

mattmoor · 2019-09-29T15:59:13Z

@nak3 What's the unexpected error you see?

Would it be possible to create an e2e test that exhibits the behavior you describe? I can think of ways we could abuse the relationship and the exec probe's wait period to create a solid window of 5xx errors.

nak3 · 2019-09-30T12:13:54Z

@nak3 What's the unexpected error you see?

The queue-proxy keeps producing no probe found. I've also described it in #5711.

I added the e2e test for exec prober for the readiness probe.

mattmoor

@nak3 Does "no probe found" mean this never becomes ready?

test/conformance/runtime/readiness_probe_test.go

mattmoor · 2019-09-30T14:01:29Z

test/conformance/runtime/readiness_probe_test.go

+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()


This will detach from the logstream, I'd move that in here as part of the change to t.Run

mattmoor · 2019-09-30T14:04:27Z

test/conformance/runtime/readiness_probe_test.go

+		corev1.Handler{
+			Exec: &corev1.ExecAction{
+				Command: []string{"/ko-app/runtime", "probe"},
+			},


Can you add a case for TCP while you are in here?

I feel like for conformance we should have variants of these tests that FAIL the readiness probes. Could you either add that or open an issue to follow up?

cc @dgerd

We've sort of been here before, but the runtime contract suggests that only httpGet and tcpSocket probes should be used. We've quietly left exec probes in, but do we want to codify their allowance into our conformance tests?

joshrider · 2019-09-30T14:08:22Z

pkg/reconciler/revision/resources/queue.go

@@ -355,7 +355,12 @@ func applyReadinessProbeDefaults(p *corev1.Probe, port int32) {
 		p.TCPSocket.Host = localAddress
 		p.TCPSocket.Port = intstr.FromInt(int(port))
 	case p.Exec != nil:
-		//User-defined ExecProbe will still be run on user-container.
+		// User-defined ExecProbe will still be run on user-container.
+		// Translate to TCP probes.


I think we initially had something like this on #4731 but it was decided to leave it out since it didn't necessarily tell us what we needed to know. See: #4731 (comment)

joshrider · 2019-09-30T14:20:10Z

@nak3 Does "no probe found" mean this never becomes ready?

It looks like something with an execProbe will never become ready.

There's an (outdated) comment in the readinessProbe code that suggests that an execProbe will be translated to a tcp probe, but I think we just missed pulling that out when we reverted the functionality linked above. I think we missed changing pkg/queue/readiness/probe.go and, as a result, left in a bug.

A possible fix that lets us maintain the undocumented support of execProbes would be to succeed in cases where the queue-proxy does not find an httpGet, tcpSocket, or execAction probe, having ProbeContainer return true instead of false.

tcnghia · 2019-09-30T22:41:08Z

It looks like the runtime contract forbids execProbe, so we should just refuse

If specified, liveness and readiness probes are REQUIRED to be of the httpGet or tcpSocket types, and MUST target the inbound container port; platform providers SHOULD disallow other probe methods

https://github.com/knative/serving/blob/master/docs/runtime-contract.md#meta-requests

toVersus · 2019-09-30T22:59:56Z

Yeah, but how can I probe the gRPC server? Current widespread strategy is to use execProbe with grpc-health-probe bundled in the image.

test/test_images/runtime/main.go

test/conformance/runtime/readiness_probe_test.go

joshuarider · 2019-10-01T02:44:14Z

I brought up the case of the runtime contract vs. execProbes a few months ago and the consensus was to leave them be. Is it time to either revisit this or consider loosening the runtime contract?

tcnghia · 2019-10-01T02:55:01Z

/cc @evankanderson

nak3 · 2019-10-02T01:00:08Z

/test pull-knative-serving-unit-tests

test/conformance/runtime/readiness_probe_test.go

test/test_images/runtime/main.go

nak3 · 2019-10-20T11:54:49Z

So, there are three options for the change. When exec probe is used in user container,

a. Translate it to TCP probe in queue-proxy (current PR)
b. Don't translate it for queue-proxy, but return true instead of false here when no probe to let pod ready.
c. Do not allow exec probe anyway as documented. (current code)

Even if we do not support exec probe, I would like to change the current error message. The current error #5711 is very unclear error.

For a and b, I can open an issue to track it if it is difficult to make the decision.

markusthoemmes · 2019-10-28T10:07:44Z

FWIW I do prefer the current implementation (option A above). It makes sure that we test for general connectivity on the configured ports regardless on the user's probes.

I do wonder why we forbid exec probes or at least discourage them. @mattmoor any idea on that? Do we need to take this to the API WG in that regard?

nak3 · 2019-11-06T06:16:41Z

As per @dgerd 's #4086 (comment), I think option B above was the current expected workaround discussed in #4086 actually.

@dgerd any thoughts for this?

tcnghia · 2019-11-26T17:14:52Z

/test pull-knative-serving-smoke-tests

markusthoemmes

/lgtm
/approve

markusthoemmes · 2019-11-28T08:50:33Z

/assign @dgerd

dgerd · 2019-12-10T20:03:06Z

/hold cancel

dgerd · 2019-12-10T21:37:43Z

/test pull-knative-serving-upgrade-tests

nak3 · 2019-12-10T23:01:37Z

/test pull-knative-serving-upgrade-tests

nak3 · 2019-12-10T23:38:35Z

/test pull-knative-serving-upgrade-tests

nak3 · 2019-12-11T01:22:15Z

/test pull-knative-serving-upgrade-tests

    probe_test.go:71: CheckSLO() = SLI for "TestProbe" = 0.912052, wanted >= 1.000000

I think this is not related to this change. I will rebase if next re-test failed.

…adinessprobe in user-container When users defined exec probe for readinessprobe, it becomes `nil` and get unexpected error. To make matters worse, the pod never becomes ready. This patch changes to translates exec probe in user-container to tcp probe.

nak3 · 2019-12-11T01:48:41Z

/retest

nak3 · 2019-12-11T06:49:56Z

/test pull-knative-serving-upgrade-tests

It looks like TestProbe in pull-knative-serving-upgrade-tests is broken. Same error happens on other PRs.

#6162

Let's confirm it once again and will file the issue if it fail.

nak3 · 2019-12-11T22:43:38Z

/retest

markusthoemmes

/lgtm
/approve

knative-prow-robot · 2019-12-12T07:25:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, nak3

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:

~~pkg/reconciler/OWNERS~~ [markusthoemmes]
~~test/OWNERS~~ [markusthoemmes]

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

knative-test-reporter-robot · 2019-12-12T07:55:10Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

test/conformance/api/v1.TestUpdateConfigurationMetadata

nak3 · 2019-12-12T08:35:21Z

/test pull-knative-serving-integration-tests

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 28, 2019

knative-prow-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/API API objects and controllers labels Sep 28, 2019

knative-prow-robot reviewed Sep 28, 2019

View reviewed changes

knative-prow-robot requested review from mattmoor and tcnghia September 28, 2019 10:57

mattmoor assigned shashwathi and joshrider Sep 29, 2019

nak3 force-pushed the fix-exec-probe branch from dc8b2c1 to 56b52ba Compare September 30, 2019 12:09

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 30, 2019

mattmoor reviewed Sep 30, 2019

View reviewed changes

joshrider reviewed Sep 30, 2019

View reviewed changes

taragu reviewed Oct 1, 2019

View reviewed changes

test/test_images/runtime/main.go Outdated Show resolved Hide resolved

test/conformance/runtime/readiness_probe_test.go Show resolved Hide resolved

knative-prow-robot requested a review from evankanderson October 1, 2019 02:55

savitaashture reviewed Oct 11, 2019

View reviewed changes

test/conformance/runtime/readiness_probe_test.go Outdated Show resolved Hide resolved

savitaashture reviewed Oct 11, 2019

View reviewed changes

test/test_images/runtime/main.go Outdated Show resolved Hide resolved

nak3 force-pushed the fix-exec-probe branch from 625ce76 to 6f5075b Compare October 20, 2019 11:11

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 11, 2019

markusthoemmes approved these changes Nov 28, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2019

knative-prow-robot assigned dgerd Nov 28, 2019

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2019

nak3 added 5 commits December 11, 2019 10:42

Add e2e test for exec readiness probe

1296359

Add tcp probe to conformance test

8d4623f

Fix review comment

3b5f340

Move test inline

f19ff68

nak3 force-pushed the fix-exec-probe branch from 7b7806c to f19ff68 Compare December 11, 2019 01:42

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2019

nak3 mentioned this pull request Dec 11, 2019

TestProbe in upgrade-test (pull-knative-serving-upgrade-tests) is broken #6197

Closed

markusthoemmes approved these changes Dec 12, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2019

knative-prow-robot merged commit 353f8fb into knative:master Dec 12, 2019

nak3 deleted the fix-exec-probe branch December 12, 2019 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate to tcp probe in queue-proxy when exec probe was used for readinessprobe in user-container #5712

Translate to tcp probe in queue-proxy when exec probe was used for readinessprobe in user-container #5712

nak3 commented Sep 28, 2019

knative-prow-robot left a comment

mattmoor commented Sep 29, 2019

nak3 commented Sep 30, 2019

mattmoor left a comment

mattmoor Sep 30, 2019

mattmoor Sep 30, 2019

joshrider Sep 30, 2019 •

edited

Loading

joshrider Sep 30, 2019

joshrider commented Sep 30, 2019

tcnghia commented Sep 30, 2019

toVersus commented Sep 30, 2019

joshuarider commented Oct 1, 2019

tcnghia commented Oct 1, 2019

nak3 commented Oct 2, 2019

nak3 commented Oct 20, 2019 •

edited

Loading

markusthoemmes commented Oct 28, 2019

nak3 commented Nov 6, 2019

tcnghia commented Nov 26, 2019

markusthoemmes left a comment

markusthoemmes commented Nov 28, 2019

dgerd commented Dec 10, 2019

dgerd commented Dec 10, 2019

nak3 commented Dec 10, 2019

nak3 commented Dec 10, 2019

nak3 commented Dec 11, 2019

nak3 commented Dec 11, 2019

nak3 commented Dec 11, 2019 •

edited

Loading

nak3 commented Dec 11, 2019

markusthoemmes left a comment

knative-prow-robot commented Dec 12, 2019

knative-test-reporter-robot commented Dec 12, 2019

nak3 commented Dec 12, 2019

	// Should never be reachable. Exec probes to be translated to
	// TCP probes when container is built.

Translate to tcp probe in queue-proxy when exec probe was used for readinessprobe in user-container #5712

Translate to tcp probe in queue-proxy when exec probe was used for readinessprobe in user-container #5712

Conversation

nak3 commented Sep 28, 2019

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

mattmoor commented Sep 29, 2019

nak3 commented Sep 30, 2019

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor Sep 30, 2019

Choose a reason for hiding this comment

mattmoor Sep 30, 2019

Choose a reason for hiding this comment

joshrider Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

joshrider Sep 30, 2019

Choose a reason for hiding this comment

joshrider commented Sep 30, 2019

tcnghia commented Sep 30, 2019

toVersus commented Sep 30, 2019

joshuarider commented Oct 1, 2019

tcnghia commented Oct 1, 2019

nak3 commented Oct 2, 2019

nak3 commented Oct 20, 2019 • edited Loading

markusthoemmes commented Oct 28, 2019

nak3 commented Nov 6, 2019

tcnghia commented Nov 26, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

markusthoemmes commented Nov 28, 2019

dgerd commented Dec 10, 2019

dgerd commented Dec 10, 2019

nak3 commented Dec 10, 2019

nak3 commented Dec 10, 2019

nak3 commented Dec 11, 2019

nak3 commented Dec 11, 2019

nak3 commented Dec 11, 2019 • edited Loading

nak3 commented Dec 11, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Dec 12, 2019

knative-test-reporter-robot commented Dec 12, 2019

nak3 commented Dec 12, 2019

joshrider Sep 30, 2019 •

edited

Loading

nak3 commented Oct 20, 2019 •

edited

Loading

nak3 commented Dec 11, 2019 •

edited

Loading