Better support for Kubernetes health probes #19173

hedayat · 2023-07-09T12:12:06Z

Add support for using port names in Kubernetes probes
Support TCPSocket probes without specifying any host names

Does this PR introduce a user-facing change?

From now on, you can also use containerPort names inside Kubernetes liveness probes to refer to the desired ports, which was not previously supported by podman.

rhatdan · 2023-07-10T18:37:46Z

@umohnani8 @ygalblum @baude PTAL

rhatdan · 2023-07-10T19:16:19Z

You need to add tests or add [NO NEW TESTS NEEDED] flag to the PR.

TomSweeneyRedHat · 2023-07-10T21:06:48Z

pkg/specgen/generate/kube/kube.go

+		if err != nil {
+			return nil, err
+		}
+		commandString = fmt.Sprintf("curl -f %s://%s:%d%s || %s", uriScheme, host, portNum, path, failureCmd)


I know this is old code, but I don't believe curl is native to Mac. If not, should we break this out into Linux, Mac, and Windows code streams? Beyond which, I'm always leery about making a curl call from code, but that seems to be water under the bridge at this point.

Note that this command is being run inside the container, not the host. Although, this is also not desirable.

I guess the ideal solution would be either for podman itself to do the probes inside code, or to have a separate container (probably like or extending the pause container) running the probes.

Closes containers#18645 Signed-off-by: Hedayat Vatankhah <[email protected]>

Signed-off-by: Hedayat Vatankhah <[email protected]>

vrothberg

I think @umohnani8 started looking into this issue as well. Let's wait for her to have a look.

umohnani8 · 2023-07-11T14:52:40Z

This is a bit different than the issue I am looking at. What I am looking at is the probe fails with a curl command because the container doesn't have a shell or curl installed as the podman code currently runs the probe commands inside the container created. We need to figure out a way to handle http commands outside of the container to check for healthy status and will be a bigger rework.

I think this PR can go it as it is addressing another issue around ports with probes and there is no need to block it on the future rework.

Changes LGTM

hedayat · 2023-07-11T16:59:25Z

This is a bit different than the issue I am looking at. What I am looking at is the probe fails with a curl command because the container doesn't have a shell or curl installed as the podman code currently runs the probe commands inside the container created. We need to figure out a way to handle http commands outside of the container to check for healthy status and will be a bigger rework.

That's #18318, and its also the case for TCP probes which is also done by running nc inside the container. And there will be gRPC probes when implemented in podman :)

Changes LGTM

Thanks a lot for the review!

vrothberg

I´d love to see a test or two being added (or extended) in https://github.com/containers/podman/blob/main/test/e2e/play_kube_test.go.

vrothberg · 2023-07-12T08:40:32Z

pkg/specgen/generate/kube/kube.go

@@ -569,6 +569,7 @@ func probeToHealthConfig(probe *v1.Probe, containerPorts []v1.ContainerPort) (*m
 	var commandString string
 	failureCmd := "exit 1"
 	probeHandler := probe.Handler
+	host := "localhost" // Kubernetes default is host IP, but with Podman currently we run inside the container


Any chance we can add a test for it?

ygalblum · 2023-07-12T13:12:34Z

LGTM for the code. But, I agree with @vrothberg about the need for tests

hedayat · 2023-07-13T14:48:29Z

@ygalblum @vrothberg

Thanks for the review. I added a number of tests covering new fixes

Signed-off-by: Hedayat Vatankhah <[email protected]>

rhatdan · 2023-07-13T17:30:00Z

/lgtm
I don't think this is a new feature ,so it should be back ported.

openshift-ci · 2023-07-13T17:30:24Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: hedayat

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:

OWNERS

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

rhatdan · 2023-07-14T11:08:29Z

/lgtm

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Jul 9, 2023

hedayat force-pushed the support-port-name-in-probes branch 2 times, most recently from e7a7f43 to 733d154 Compare July 10, 2023 00:13

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

hedayat force-pushed the support-port-name-in-probes branch from 733d154 to 5d97385 Compare July 10, 2023 00:16

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

hedayat force-pushed the support-port-name-in-probes branch 4 times, most recently from 0d2b9a9 to 09198d5 Compare July 10, 2023 17:02

hedayat changed the title ~~Add support for using port names in Kubernetes health probes~~ Better support for Kubernetes health probes Jul 10, 2023

hedayat force-pushed the support-port-name-in-probes branch from 09198d5 to 30e66b4 Compare July 10, 2023 17:05

hedayat marked this pull request as ready for review July 10, 2023 17:20

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2023

TomSweeneyRedHat reviewed Jul 10, 2023

View reviewed changes

hedayat force-pushed the support-port-name-in-probes branch from 30e66b4 to ed99c18 Compare July 10, 2023 21:14

hedayat added 2 commits July 11, 2023 01:00

Add support for using port names in Kubernetes health probes

ac3a115

Closes containers#18645 Signed-off-by: Hedayat Vatankhah <[email protected]>

Fix TCP probes when the optional host field is not given

600de05

Signed-off-by: Hedayat Vatankhah <[email protected]>

hedayat force-pushed the support-port-name-in-probes branch from ed99c18 to 600de05 Compare July 10, 2023 21:31

vrothberg reviewed Jul 11, 2023

View reviewed changes

vrothberg reviewed Jul 12, 2023

View reviewed changes

Add more tests for liveness probes with default hostname & named ports

a8d8c94

Signed-off-by: Hedayat Vatankhah <[email protected]>

hedayat force-pushed the support-port-name-in-probes branch from 5568306 to a8d8c94 Compare July 13, 2023 14:54

openshift-ci bot assigned rhatdan Jul 13, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2023

rhatdan added approved Indicates a PR has been approved by an approver from all required OWNERS files. 4.6 and removed lgtm Indicates that a PR is ready to be merged. labels Jul 13, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2023

openshift-merge-robot merged commit 960a764 into containers:main Jul 14, 2023

hedayat deleted the support-port-name-in-probes branch July 14, 2023 22:03

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 13, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better support for Kubernetes health probes #19173

Better support for Kubernetes health probes #19173

hedayat commented Jul 9, 2023 •

edited

Loading

rhatdan commented Jul 10, 2023

rhatdan commented Jul 10, 2023

TomSweeneyRedHat Jul 10, 2023

hedayat Jul 10, 2023

vrothberg left a comment

umohnani8 commented Jul 11, 2023

hedayat commented Jul 11, 2023

vrothberg left a comment

vrothberg Jul 12, 2023

ygalblum commented Jul 12, 2023

hedayat commented Jul 13, 2023

rhatdan commented Jul 13, 2023

openshift-ci bot commented Jul 13, 2023

rhatdan commented Jul 14, 2023

Better support for Kubernetes health probes #19173

Better support for Kubernetes health probes #19173

Conversation

hedayat commented Jul 9, 2023 • edited Loading

Does this PR introduce a user-facing change?

rhatdan commented Jul 10, 2023

rhatdan commented Jul 10, 2023

TomSweeneyRedHat Jul 10, 2023

Choose a reason for hiding this comment

hedayat Jul 10, 2023

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

umohnani8 commented Jul 11, 2023

hedayat commented Jul 11, 2023

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Jul 12, 2023

Choose a reason for hiding this comment

ygalblum commented Jul 12, 2023

hedayat commented Jul 13, 2023

rhatdan commented Jul 13, 2023

openshift-ci bot commented Jul 13, 2023

rhatdan commented Jul 14, 2023

hedayat commented Jul 9, 2023 •

edited

Loading