Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

podman play kube support container startup probe #16794

Merged
merged 2 commits into from
Dec 21, 2022

Conversation

karta0807913
Copy link
Contributor

@karta0807913 karta0807913 commented Dec 9, 2022

I made two changes in this PR.

First, I add the kube startup probe support. Since the container startup probe has already been merged (#13909), I think we can make kube play also support this feature.

Second, I change the implementation of the "command probe".
The current command probe implementation doesn't have the same behavior with k8s; for example:

livenessProbe:
  exec:
    command:
      - /bin/sh
      - -c
      - curl -f -s localhost:80

You will get the following error:

{
     "Start": "2022-12-09T17:19:12.724152979+08:00",
     "End": "2022-12-09T17:19:12.753324336+08:00",
     "ExitCode": 1,
     "Output": "curl: try 'curl --help' or 'curl --manual' for more information"
}

You can use the following yml to reproduce this problem.

apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx
spec:
  replicas: 1
  template:
    spec:
      restartPolicy: Never
      containers:
      - name: nginx
        image: docker.io/library/nginx
        livenessProbe:
          exec:
            command:
            - /bin/sh
            - -c
            - curl -f -s localhost:80
          initialDelaySeconds: 0
          periodSeconds: 1

That's because the code concat it simply.

switch {
case probeHandler.Exec != nil:
execString := strings.Join(probeHandler.Exec.Command, " ")
commandString = fmt.Sprintf("%s || %s", execString, failureCmd)
case probeHandler.HTTPGet != nil:

Signed-off-by: Liang Chu-Xuan [email protected]

Does this PR introduce a user-facing change?

Yes

podman play kube support startup probe
make probe use json string array instead of CMD-SHELL

@karta0807913
Copy link
Contributor Author

/kind feature

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 9, 2022
@karta0807913 karta0807913 force-pushed the main branch 2 times, most recently from 6d8caed to da5cc61 Compare December 9, 2022 16:12
@karta0807913
Copy link
Contributor Author

agent does not respond, rerun the test.

* podman kube play support startup probe
* make probe use json string array instead of CMD-SHELL

Signed-off-by: Liang Chu-Xuan <[email protected]>
@vrothberg
Copy link
Member

Thanks for opening the PR. I will take a close look early next week.

In general, I recommend to open an issue before opening a PR. This gives us a chance to discuss how to best implement the feature.

@mheon PTAL

@mheon
Copy link
Member

mheon commented Dec 16, 2022

Code seems OK here, but someone more familiar with K8S should review. @umohnani8 PTAL

@rhatdan
Copy link
Member

rhatdan commented Dec 16, 2022

// configure healthcheck on the basis of Handler Actions.
switch {
case probeHandler.Exec != nil:
cmd, err := json.Marshal(probeHandler.Exec.Command)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not immediately obvious to me why this works. do you have a go doc reference or code you based this off that could explain why/how it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func makeHealthCheck(inCmd string, interval int32, retries int32, timeout int32, startPeriod int32) (*manifest.Schema2HealthConfig, error) {
// Every healthcheck requires a command
if len(inCmd) == 0 {
return nil, errors.New("must define a healthcheck command for all healthchecks")
}
// first try to parse option value as JSON array of strings...
cmd := []string{}
if inCmd == "none" {
cmd = []string{define.HealthConfigTestNone}
} else {
err := json.Unmarshal([]byte(inCmd), &cmd)
if err != nil {
// ...otherwise pass it to "/bin/sh -c" inside the container
cmd = []string{define.HealthConfigTestCmdShell}
cmd = append(cmd, strings.Split(inCmd, " ")...)
}
}

the function makeHealthCheck supports input string as a json array :D

@haircommander
Copy link
Collaborator

one request for an additional comment, but in general LGTM. the json.Marshal trick is slick

@karta0807913
Copy link
Contributor Author

Thanks for opening the PR. I will take a close look early next week.

In general, I recommend to open an issue before opening a PR. This gives us a chance to discuss how to best implement the feature.

@mheon PTAL

I am sorry for that :(
I will create a new issue next time when I want to do some change :D

* also, change makeHealthCheck to the standard test command structure

Signed-off-by: Liang Chu-Xuan <[email protected]>
@umohnani8
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2022
@rhatdan
Copy link
Member

rhatdan commented Dec 21, 2022

/approve
Thanks @karta0807913

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 21, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karta0807913, rhatdan

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 21, 2022
@openshift-merge-robot openshift-merge-robot merged commit 0a34a4f into containers:main Dec 21, 2022
@void-spark
Copy link

Does this mean startup probes are supported in kube play?
Should that not be reflected in the documentation?
E.g.:
https://docs.podman.io/en/v4.4/markdown/podman-kube-play.1.html
image

@rhatdan
Copy link
Member

rhatdan commented Jun 22, 2023

Yes care to open a PR?

@rhatdan
Copy link
Member

rhatdan commented Jun 22, 2023

Actually the code looks like they are blocked.

		if initCtr.Lifecycle != nil || initCtr.LivenessProbe != nil || initCtr.ReadinessProbe != nil || initCtr.StartupProbe != nil {
			return nil, nil, fmt.Errorf("cannot create an init container that has either of lifecycle, livenessProbe, readinessProbe, or startupProbe set")
		}

@rhatdan
Copy link
Member

rhatdan commented Jun 22, 2023

@umohnani8 Please investigate.

@github-actions 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 Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants