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.

Sign up for GitHub

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

Jump to bottom

handle the restartPolicy for play kube and generate kube #7671

Merged
merged 1 commit into from
Sep 18, 2020
Merged

handle the restartPolicy for play kube and generate kube #7671

merged 1 commit into from
Sep 18, 2020

Conversation

zhangguanzhang
Copy link
Collaborator

Fixes: #7656
Signed-off-by: zhangguanzhang [email protected]

@zhangguanzhang zhangguanzhang force-pushed the play-kube-handle-restartPolicy branch from dfaf5c5 to 1687af6 Compare September 17, 2020 14:47
mheon
mheon reviewed Sep 17, 2020
View reviewed changes
pkg/domain/infra/abi/play.go
case v1.RestartPolicyNever:
ctrRestartPolicy = libpod.RestartPolicyNo
default: // Default to Always
ctrRestartPolicy = libpod.RestartPolicyAlways
Copy link
Member

Choose a reason for hiding this comment

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

Is this the default? I remember the upstream issue mentioning OnFailure was the default for Kube. @haircommander Would you happen to know this?

Copy link
Collaborator Author

@zhangguanzhang zhangguanzhang Sep 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

[root@k8s-m1 ~]# kubectl explain pod.spec.restartPolicy
KIND:     Pod
VERSION:  v1

FIELD:    restartPolicy <string>

DESCRIPTION:
     Restart policy for all containers within the pod. One of Always, OnFailure,
     Never. Default to Always. More info:
     https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#restart-policy

Default to Always

Copy link

@dtitov dtitov Sep 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@mheon, if you are referring to my issue, then I took this info from this doc:

–restart-policy=policy

Set the systemd restart policy. The restart-policy must be one of: “no”, “on-success”, “on-failure”, “on-abnormal”, “on-watchdog”, “on-abort”, or “always”. The default policy is on-failure.

But now I see that it's a wrong doc. In Kubernetes the default restart policy is indeed Always. I have just updated the issue.

@mheon
Copy link
Member

mheon commented Sep 17, 2020

/approve
One question but otherwise LGTM

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, zhangguanzhang

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2020
haircommander
haircommander reviewed Sep 17, 2020
View reviewed changes
test/e2e/play_kube_test.go Outdated
Labels: make(map[string]string),
Annotations: make(map[string]string),
Name: defaultPodName,
RestartPolicy: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

we may want the default here to be never as that matches the behavior the tests were written to expect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

got it

@zhangguanzhang zhangguanzhang force-pushed the play-kube-handle-restartPolicy branch from 1687af6 to af08051 Compare September 17, 2020 16:43
@rhatdan
Copy link
Member

rhatdan commented Sep 17, 2020

Could you make sure that the restart policy is also written to the yaml file if podman generate kube is run?

@haircommander
Copy link
Collaborator

haircommander commented Sep 17, 2020 •
edited
Loading

Could you make sure that the restart policy is also written to the yaml file if podman generate kube is run?

a difficulty here is podman's restartPolicy is container level, k8s is pod level. what do we do when they differ?

@mheon
Copy link
Member

mheon commented Sep 17, 2020

Restart policy for pods would be a fun RFE, I might get ambitious and code that up at some point.

@zhangguanzhang zhangguanzhang force-pushed the play-kube-handle-restartPolicy branch 7 times, most recently from 6599559 to df68d47 Compare September 18, 2020 05:26
@zhangguanzhang zhangguanzhang changed the title handle the play kube with restartPolicy handle the play kube and generate kube for the restartPolicy Sep 18, 2020
@zhangguanzhang zhangguanzhang force-pushed the play-kube-handle-restartPolicy branch from df68d47 to f0ccac1 Compare September 18, 2020 05:28
@zhangguanzhang
Copy link
Collaborator Author

Could you make sure that the restart policy is also written to the yaml file if podman generate kube is run?

It had be done

@zhangguanzhang zhangguanzhang changed the title handle the play kube and generate kube for the restartPolicy handle the restartPolicy for play kube and generate kube Sep 18, 2020
@zhangguanzhang
Copy link
Collaborator Author

/retest

@zhangguanzhang
Copy link
Collaborator Author

ci had error, but no reason 😥, @vrothberg @TomSweeneyRedHat PTAL

@vrothberg
Copy link
Member

Restarted the job. Thanks for the ping!

@zhangguanzhang
Copy link
Collaborator Author

Restarted the job. Thanks for the ping!

thanks

@rhatdan
Copy link
Member

rhatdan commented Sep 18, 2020

Thanks @zhangguanzhang
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2020
@openshift-merge-robot openshift-merge-robot merged commit fc131a2 into containers:master Sep 18, 2020
@zhangguanzhang zhangguanzhang deleted the play-kube-handle-restartPolicy branch September 23, 2020 05:29
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@haircommander haircommander haircommander left review comments

@mheon mheon mheon left review comments

@dtitov dtitov dtitov left review comments

@TomSweeneyRedHat TomSweeneyRedHat Awaiting requested review from TomSweeneyRedHat

@vrothberg vrothberg Awaiting requested review from vrothberg

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

restartPolicy doesn't seem to work with podman play kube
8 participants
@zhangguanzhang @mheon @openshift-ci-robot @rhatdan @haircommander @vrothberg @dtitov @openshift-merge-robot