Probe port different than container port #12606

izabelacg · 2022-02-09T22:00:39Z

Proposed Changes

Allow readiness probe port to be different than the user container port.

Release Note

Allow readiness probe port to be different than the user container port.

knative-prow-robot · 2022-02-09T22:00:52Z

Welcome @izabelacg! It looks like this is your first PR to knative/serving 🎉

knative-prow-robot · 2022-02-09T22:00:52Z

Hi @izabelacg. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

nader-ziada · 2022-02-09T22:01:25Z

/ok-to-test

nader-ziada · 2022-02-10T14:54:25Z

This PR is related to #8471

nader-ziada · 2022-02-15T20:58:35Z

I worked with @izabelacg on this PR, so would you folks mind reviewing @dprotaso @psschwei

test/e2e/readinessport_test.go

nader-ziada · 2022-02-15T23:29:58Z

/retest

dprotaso · 2022-02-16T14:58:40Z

kicked off the GithubAction workflows

codecov · 2022-02-16T15:03:54Z

Codecov Report

Merging #12606 (c3f62df) into main (4262dcb) will decrease coverage by 0.20%.
The diff coverage is 57.14%.

❗ Current head c3f62df differs from pull request most recent head 6d475fb. Consider uploading reports for the commit 6d475fb to get more accurate results

@@            Coverage Diff             @@
##             main   #12606      +/-   ##
==========================================
- Coverage   87.48%   87.28%   -0.21%     
==========================================
  Files         195      196       +1     
  Lines        9718     9744      +26     
==========================================
+ Hits         8502     8505       +3     
- Misses        931      953      +22     
- Partials      285      286       +1

Impacted Files	Coverage Δ
pkg/apis/serving/k8s_validation.go	`93.17% <50.00%> (-0.45%)`	⬇️
pkg/reconciler/revision/resources/queue.go	`98.22% <66.66%> (-0.01%)`	⬇️
pkg/activator/net/throttler.go	`87.95% <0.00%> (-0.90%)`	⬇️
pkg/apis/serving/v1/revision_types.go	`100.00% <0.00%> (ø)`
pkg/reconciler/revision/rate_limiter.go	`16.66% <0.00%> (ø)`
pkg/reconciler/configuration/configuration.go	`86.15% <0.00%> (+1.53%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4262dcb...6d475fb. Read the comment docs.

nader-ziada · 2022-02-16T17:29:25Z

kicked off the GithubAction workflows

why aren't the actions triggered automatically? and do you need special permissions to kick it off?

nader-ziada · 2022-02-16T17:29:46Z

@carlisia would you mind reviewing as well?

izabelacg · 2022-02-16T23:09:08Z

/retest

dprotaso · 2022-02-17T18:12:27Z

why aren't the actions triggered automatically? and do you need special permissions to kick it off?

PRs from non-org members don't automatically kick off the actions for security reasons - ie. bitcoin miners

I created an issue for /ok-to-test approve the action run

knative/test-infra#3057

izabelacg · 2022-02-22T15:00:01Z

/retest

test/test_images/readinessport/README.md

dprotaso · 2022-02-17T18:57:24Z

test/e2e/readinessport_test.go

+ v1test "knative.dev/serving/test/v1"
+)
+
+func TestReadinessPort(t *testing.T) {


Suggested change

func TestReadinessPort(t *testing.T) {

func TestReadinessAlternatePort(t *testing.T) {

this was reverted

pkg/apis/serving/k8s_validation_test.go

pkg/apis/serving/k8s_validation.go

pkg/reconciler/revision/resources/deploy_test.go

pkg/reconciler/revision/resources/queue.go

pkg/reconciler/revision/resources/queue_test.go

dprotaso · 2022-02-22T18:12:32Z

pkg/reconciler/revision/resources/queue_test.go

 if container == nil {
 return fmt.Sprintf(testProbeJSONTemplate, v1.DefaultUserPort)
 }
-
+ if container.ReadinessProbe.TCPSocket != nil && container.ReadinessProbe.TCPSocket.Port != intstr.FromInt(int(0)) {
+ return fmt.Sprintf(testProbeJSONTemplate, container.ReadinessProbe.TCPSocket.Port.IntVal)
+ }
+ if container.ReadinessProbe.HTTPGet != nil && container.ReadinessProbe.HTTPGet.Port != intstr.FromInt(int(0)) {
+ return fmt.Sprintf(testProbeJSONTemplate, container.ReadinessProbe.HTTPGet.Port.IntVal)
+ }
 if ports := container.Ports; len(ports) > 0 && ports[0].ContainerPort != 0 {
 return fmt.Sprintf(testProbeJSONTemplate, ports[0].ContainerPort)
 }


nit: I'd probably just make a switch statement so this reads a bit easier.

I was scanning to see if the template changes under any condition and it doesn't

but what would you switch on, the if condition is checking different fields, can you clarify please?

dprotaso · 2022-03-01T16:25:49Z

/retest

dprotaso · 2022-03-01T17:37:23Z

@izabelacg the failures are legit and not flakey

dprotaso · 2022-03-01T17:56:10Z

test/test_images/readiness/readiness.go

+ probeServer.HandleFunc("/", handleHealthz)
+
+ go func() {
+ http.ListenAndServe(":"+strconv.Itoa(healthcheckPort), probeServer)


if the health check port isn't set we want it on the mainServer

That's probably the cause of the failures

Or you could add the handleHealthz handler to both servers to keep things simple

dprotaso · 2022-03-01T17:58:26Z

test/test_images/readiness/readiness.go

+ http.ListenAndServe(":8080", mainServer)
+ }()
+
+ <-finish


You can drop the use of the channel and just call http.ListenAndServe to block. Invoking that in a go routine isn't needed

I think the go func is required cause you have two ListenAndServe, which doesn't work on the same thread

If you invoke one ListenAndServe in a go routine and the other on the main go routine you don't need the finish channel

dprotaso · 2022-03-01T18:02:34Z

I resolved some of the earlier comments - there's one or two outstanding

carlisia

LGTM! I only have a couple super minor comments.

test/e2e/readinessport_test.go

pkg/apis/serving/k8s_validation_test.go

test/conformance.go

dprotaso · 2022-03-07T15:34:02Z

pkg/apis/serving/k8s_validation.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Knative Authors
+Copyright 2022 The Knative Authors


we don't need to modify the copyright of existing files - please undo

oh sorry, so basically only test/e2e/readinessport_test.go needs change on the copyright. Will revert changes

dprotaso · 2022-03-07T15:43:19Z

Updated the boilerplate year checks here: #12699

dprotaso · 2022-03-08T02:18:58Z

/lgtm
/approve

knative-prow-robot · 2022-03-08T02:19:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, izabelacg

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/apis/OWNERS~~ [dprotaso]
~~pkg/reconciler/OWNERS~~ [dprotaso]
~~test/OWNERS~~ [dprotaso]

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

izabelacg added 2 commits February 7, 2022 17:09

remove validation to allow container port different than the probe port

77a5623

fixed tests

30fcf7f

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 9, 2022

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/API API objects and controllers labels Feb 9, 2022

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 9, 2022

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 11, 2022

izabelacg added 4 commits February 14, 2022 15:33

remove conformance tests

426fab6

add readiness port constant and tests

bd596ac

add readiness port e2e test

75c9795

remove port from pod spec

05ff51d

izabelacg changed the title ~~[WIP] Probe port different than container port~~ Probe port different than container port Feb 15, 2022

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 15, 2022

cleaning up code

72968dd

nader-ziada reviewed Feb 15, 2022

View reviewed changes

test/e2e/readinessport_test.go Show resolved Hide resolved

add copyright text to e2e test

a46b5e3

fix go imports

fdecd83

carlisia self-assigned this Feb 17, 2022

remove unused struct from tests

5f3b6f8

dprotaso reviewed Feb 22, 2022

View reviewed changes

izabelacg added 2 commits February 28, 2022 18:00

merge readiness and readiness port image and address pr feedback

74e008d

address pr feedback

1e0308d

dprotaso reviewed Mar 1, 2022

View reviewed changes

carlisia reviewed Mar 2, 2022

View reviewed changes

test/e2e/readinessport_test.go Outdated Show resolved Hide resolved

pkg/apis/serving/k8s_validation_test.go Show resolved Hide resolved

izabelacg added 2 commits March 2, 2022 22:13

address more pr feedback

b5c77af

remove duplicated test

c3f62df

dprotaso reviewed Mar 7, 2022

View reviewed changes

test/conformance.go Outdated Show resolved Hide resolved

dprotaso reviewed Mar 7, 2022

View reviewed changes

izabelacg added 2 commits March 7, 2022 14:52

remove unnecessary changes on copyright

2541366

remove extra go routine and channel from test image

6d475fb

knative-prow-robot assigned dprotaso Mar 8, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2022

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2022

knative-prow-robot merged commit afe46a9 into knative:main Mar 8, 2022

astefanutti mentioned this pull request Dec 6, 2022

Health Trait : probe with HTTPS scheme not working apache/camel-k#3877

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Probe port different than container port #12606

Probe port different than container port #12606

izabelacg commented Feb 9, 2022 •

edited

Loading

knative-prow-robot commented Feb 9, 2022

knative-prow-robot commented Feb 9, 2022

nader-ziada commented Feb 9, 2022

nader-ziada commented Feb 10, 2022

nader-ziada commented Feb 15, 2022

nader-ziada commented Feb 15, 2022

dprotaso commented Feb 16, 2022

codecov bot commented Feb 16, 2022 •

edited

Loading

nader-ziada commented Feb 16, 2022

nader-ziada commented Feb 16, 2022

izabelacg commented Feb 16, 2022

dprotaso commented Feb 17, 2022

izabelacg commented Feb 22, 2022

dprotaso Feb 17, 2022

dprotaso Mar 7, 2022

dprotaso Feb 22, 2022 •

edited

Loading

nader-ziada Mar 3, 2022

dprotaso commented Mar 1, 2022

dprotaso commented Mar 1, 2022

dprotaso Mar 1, 2022

dprotaso Mar 7, 2022

dprotaso Mar 1, 2022 •

edited

Loading

nader-ziada Mar 3, 2022

dprotaso Mar 7, 2022

dprotaso commented Mar 1, 2022

carlisia left a comment

dprotaso Mar 7, 2022 •

edited

Loading

izabelacg Mar 7, 2022

dprotaso commented Mar 7, 2022

dprotaso commented Mar 8, 2022

knative-prow-robot commented Mar 8, 2022

	func TestReadinessPort(t *testing.T) {
	func TestReadinessAlternatePort(t *testing.T) {

Probe port different than container port #12606

Probe port different than container port #12606

Conversation

izabelacg commented Feb 9, 2022 • edited Loading

Proposed Changes

knative-prow-robot commented Feb 9, 2022

knative-prow-robot commented Feb 9, 2022

nader-ziada commented Feb 9, 2022

nader-ziada commented Feb 10, 2022

nader-ziada commented Feb 15, 2022

nader-ziada commented Feb 15, 2022

dprotaso commented Feb 16, 2022

codecov bot commented Feb 16, 2022 • edited Loading

Codecov Report

nader-ziada commented Feb 16, 2022

nader-ziada commented Feb 16, 2022

izabelacg commented Feb 16, 2022

dprotaso commented Feb 17, 2022

izabelacg commented Feb 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso commented Mar 1, 2022

dprotaso commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso commented Mar 1, 2022

carlisia left a comment

Choose a reason for hiding this comment

dprotaso Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso commented Mar 7, 2022

dprotaso commented Mar 8, 2022

knative-prow-robot commented Mar 8, 2022

izabelacg commented Feb 9, 2022 •

edited

Loading

codecov bot commented Feb 16, 2022 •

edited

Loading

dprotaso Feb 22, 2022 •

edited

Loading

dprotaso Mar 1, 2022 •

edited

Loading

dprotaso Mar 7, 2022 •

edited

Loading