From 012ffbb5e9925543c20b23e0d6f26e4958d414b5 Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Sat, 28 Sep 2019 19:40:26 +0900 Subject: [PATCH 1/5] Translate to tcp probe in queue-proxy when exec probe was used for readinessprobe 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. --- pkg/reconciler/revision/resources/deploy_test.go | 2 +- pkg/reconciler/revision/resources/queue.go | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/revision/resources/deploy_test.go b/pkg/reconciler/revision/resources/deploy_test.go index 64f6f8f62bd5..9479708fa054 100644 --- a/pkg/reconciler/revision/resources/deploy_test.go +++ b/pkg/reconciler/revision/resources/deploy_test.go @@ -612,7 +612,7 @@ func TestMakePodSpec(t *testing.T) { withExecReadinessProbe([]string{"echo", "hello"})), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "0"), - withEnvVar("SERVING_READINESS_PROBE", "{}"), + withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":8080,"host":"127.0.0.1"}}`), ), }), }, { diff --git a/pkg/reconciler/revision/resources/queue.go b/pkg/reconciler/revision/resources/queue.go index 028f781fe03b..8e1df78374cc 100644 --- a/pkg/reconciler/revision/resources/queue.go +++ b/pkg/reconciler/revision/resources/queue.go @@ -356,7 +356,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. + // Use TCP probe in queue-proxy. + p.TCPSocket = &corev1.TCPSocketAction{ + Host: localAddress, + Port: intstr.FromInt(int(port)), + } p.Exec = nil } From 1296359b2a818b36e21e84b5668cca6026b37e15 Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Mon, 30 Sep 2019 09:49:00 +0900 Subject: [PATCH 2/5] Add e2e test for exec readiness probe --- .../runtime/readiness_probe_test.go | 76 +++++++++++++------ test/test_images/runtime/main.go | 14 +++- 2 files changed, 65 insertions(+), 25 deletions(-) diff --git a/test/conformance/runtime/readiness_probe_test.go b/test/conformance/runtime/readiness_probe_test.go index 2daffb5ae309..0c54e3abd07a 100644 --- a/test/conformance/runtime/readiness_probe_test.go +++ b/test/conformance/runtime/readiness_probe_test.go @@ -30,6 +30,33 @@ import ( v1a1test "knative.dev/serving/test/v1alpha1" ) +// testCases for table-driven testing. +var testCases = []struct { + // name of the test case, which will be inserted in names of routes, configurations, etc. + // Use a short name here to avoid hitting the 63-character limit in names + // (e.g., "service-to-service-call-svc-cluster-local-uagkdshh-frkml-service" is too long.) + name string + // handler to be used for readiness probe in user container. + handler corev1.Handler +}{ + { + "httpGet", + corev1.Handler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: "/healthz", + }, + }, + }, + { + "exec", + corev1.Handler{ + Exec: &corev1.ExecAction{ + Command: []string{"/ko-app/runtime", "probe"}, + }, + }, + }, +} + func TestProbeRuntime(t *testing.T) { t.Parallel() cancel := logstream.Start(t) @@ -37,31 +64,32 @@ func TestProbeRuntime(t *testing.T) { clients := test.Setup(t) - names := test.ResourceNames{ - Service: test.ObjectNameForTest(t), - Image: "runtime", - } + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + names := test.ResourceNames{ + Service: test.ObjectNameForTest(t), + Image: test.Runtime, + } - test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) - defer test.TearDown(clients, names) - - t.Log("Creating a new Service") - resources, _, err := v1a1test.CreateRunLatestServiceReady(t, clients, &names, - false, /* https TODO(taragu) turn this on after helloworld test running with https */ - v1a1opts.WithReadinessProbe( - &corev1.Probe{ - Handler: corev1.Handler{ - HTTPGet: &corev1.HTTPGetAction{ - Path: "/healthz", - }, - }, - })) - if err != nil { - t.Fatalf("Failed to create initial Service: %v: %v", names.Service, err) - } + test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) + defer test.TearDown(clients, names) - // Check if scaling down works even if access from liveness probe exists. - if err := e2e.WaitForScaleToZero(t, revisionresourcenames.Deployment(resources.Revision), clients); err != nil { - t.Fatalf("Could not scale to zero: %v", err) + t.Log("Creating a new Service") + resources, _, err := v1a1test.CreateRunLatestServiceReady(t, clients, &names, + false, /* https TODO(taragu) turn this on after helloworld test running with https */ + v1a1opts.WithReadinessProbe( + &corev1.Probe{ + Handler: tc.handler, + })) + if err != nil { + t.Fatalf("Failed to create initial Service: %v: %v", names.Service, err) + } + // Check if scaling down works even if access from liveness probe exists. + if err := e2e.WaitForScaleToZero(t, revisionresourcenames.Deployment(resources.Revision), clients); err != nil { + t.Fatalf("Could not scale to zero: %v", err) + } + }) } } diff --git a/test/test_images/runtime/main.go b/test/test_images/runtime/main.go index 36387fdbb1cd..b8a4879ae4bb 100644 --- a/test/test_images/runtime/main.go +++ b/test/test_images/runtime/main.go @@ -14,6 +14,7 @@ limitations under the License. package main import ( + "flag" "log" "net/http" "os" @@ -23,7 +24,6 @@ import ( ) func main() { - // We expect PORT to be defined in a Knative environment // and don't want to mask this failure in a test image. port, isSet := os.LookupEnv("PORT") @@ -31,6 +31,18 @@ func main() { log.Fatal("Environment variable PORT is not set.") } + // This is an option for exec readiness probe test. + flag.Parse() + args := flag.Args() + if len(args) > 0 && args[0] == "probe" { + url := "http://localhost:" + port + _, err := http.Get(url) + if err != nil { + log.Fatalf("failed to probe %v", err) + } + return + } + mux := http.NewServeMux() handlers.InitHandlers(mux) From 8d4623f643c1af3b360a982370fb4aad35906588 Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Wed, 2 Oct 2019 09:37:12 +0900 Subject: [PATCH 3/5] Add tcp probe to conformance test --- .../runtime/readiness_probe_test.go | 31 ++++++++++--------- test/test_images/runtime/main.go | 2 +- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/test/conformance/runtime/readiness_probe_test.go b/test/conformance/runtime/readiness_probe_test.go index 0c54e3abd07a..c9beffad3ace 100644 --- a/test/conformance/runtime/readiness_probe_test.go +++ b/test/conformance/runtime/readiness_probe_test.go @@ -38,27 +38,28 @@ var testCases = []struct { name string // handler to be used for readiness probe in user container. handler corev1.Handler -}{ - { - "httpGet", - corev1.Handler{ - HTTPGet: &corev1.HTTPGetAction{ - Path: "/healthz", - }, +}{{ + "httpGet", + corev1.Handler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: "/healthz", }, }, - { - "exec", - corev1.Handler{ - Exec: &corev1.ExecAction{ - Command: []string{"/ko-app/runtime", "probe"}, - }, +}, { + "tcpSocket", + corev1.Handler{ + TCPSocket: &corev1.TCPSocketAction{}, + }, +}, { + "exec", + corev1.Handler{ + Exec: &corev1.ExecAction{ + Command: []string{"/ko-app/runtime", "probe"}, }, }, -} +}} func TestProbeRuntime(t *testing.T) { - t.Parallel() cancel := logstream.Start(t) defer cancel() diff --git a/test/test_images/runtime/main.go b/test/test_images/runtime/main.go index b8a4879ae4bb..7644190172d1 100644 --- a/test/test_images/runtime/main.go +++ b/test/test_images/runtime/main.go @@ -38,7 +38,7 @@ func main() { url := "http://localhost:" + port _, err := http.Get(url) if err != nil { - log.Fatalf("failed to probe %v", err) + log.Fatalf("Failed to probe %v", err) } return } From 3b5f3409d9ccfb7974ae0e0362434d2de3ccb781 Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Sun, 20 Oct 2019 20:08:16 +0900 Subject: [PATCH 4/5] Fix review comment --- test/test_images/runtime/main.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/test_images/runtime/main.go b/test/test_images/runtime/main.go index 7644190172d1..d231e82a737d 100644 --- a/test/test_images/runtime/main.go +++ b/test/test_images/runtime/main.go @@ -36,8 +36,7 @@ func main() { args := flag.Args() if len(args) > 0 && args[0] == "probe" { url := "http://localhost:" + port - _, err := http.Get(url) - if err != nil { + if _, err := http.Get(url); err != nil { log.Fatalf("Failed to probe %v", err) } return From f19ff68faf150c52e186ac47735ba2f06754f9ea Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Mon, 11 Nov 2019 16:49:12 +0900 Subject: [PATCH 5/5] Move test inline --- .../runtime/readiness_probe_test.go | 57 +++++++++---------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/test/conformance/runtime/readiness_probe_test.go b/test/conformance/runtime/readiness_probe_test.go index c9beffad3ace..a2bf1246f44a 100644 --- a/test/conformance/runtime/readiness_probe_test.go +++ b/test/conformance/runtime/readiness_probe_test.go @@ -30,41 +30,40 @@ import ( v1a1test "knative.dev/serving/test/v1alpha1" ) -// testCases for table-driven testing. -var testCases = []struct { - // name of the test case, which will be inserted in names of routes, configurations, etc. - // Use a short name here to avoid hitting the 63-character limit in names - // (e.g., "service-to-service-call-svc-cluster-local-uagkdshh-frkml-service" is too long.) - name string - // handler to be used for readiness probe in user container. - handler corev1.Handler -}{{ - "httpGet", - corev1.Handler{ - HTTPGet: &corev1.HTTPGetAction{ - Path: "/healthz", - }, - }, -}, { - "tcpSocket", - corev1.Handler{ - TCPSocket: &corev1.TCPSocketAction{}, - }, -}, { - "exec", - corev1.Handler{ - Exec: &corev1.ExecAction{ - Command: []string{"/ko-app/runtime", "probe"}, - }, - }, -}} - func TestProbeRuntime(t *testing.T) { cancel := logstream.Start(t) defer cancel() clients := test.Setup(t) + var testCases = []struct { + // name of the test case, which will be inserted in names of routes, configurations, etc. + // Use a short name here to avoid hitting the 63-character limit in names + // (e.g., "service-to-service-call-svc-cluster-local-uagkdshh-frkml-service" is too long.) + name string + // handler to be used for readiness probe in user container. + handler corev1.Handler + }{{ + "httpGet", + corev1.Handler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: "/healthz", + }, + }, + }, { + "tcpSocket", + corev1.Handler{ + TCPSocket: &corev1.TCPSocketAction{}, + }, + }, { + "exec", + corev1.Handler{ + Exec: &corev1.ExecAction{ + Command: []string{"/ko-app/runtime", "probe"}, + }, + }, + }} + for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) {