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

Add --restart flag to pod create #17627

Merged
merged 4 commits into from
May 2, 2023

Conversation

umohnani8
Copy link
Member

@umohnani8 umohnani8 commented Feb 24, 2023

Add --restart flag to pod create to allow users to set the restart policy for the pod, which applies to all the containers in the pod. This reuses the restart policy already there for containers and has the same restart policy options. Add "never" to the restart policy options to match k8s syntax. It is a synonym for "no" and does the exact same thing where the containers are not restarted once exited.
Only the containers that have exited will be restarted based on the restart policy, running containers will not be restarted when an exited container is restarted in the same pod (same as is done in k8s).

Add Restarts column to the podman ps and podman pod ps output to show how many times a
container was restarted based on its restart policy when --format={{.Restarts}}.

Signed-off-by: Urvashi Mohnani [email protected]

Does this PR introduce a user-facing change?

Add --restart flag to pod create to be able to set the restart policy for all the containers in a pod.
Add Restarts column to the output of podman ps to show the number of times a container has been restarted based on its restart policy when --format={{.Restarts}}.
Add Restarts column to the output of podman pod ps when --format={{.Restarts}} to show the total number of container restarts in a pod.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umohnani8

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 Feb 24, 2023
@TomSweeneyRedHat
Copy link
Member

After a quick skim, looks good in general. Test aren't at all happy.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

I had something else in mind for pod create --restart. Currently, it does not yield a podman pod restart but instead, it'll set it for all containers. There are still some open questions in the design doc as well.

@mheon @giuseppe PTAL

cmd/podman/containers/create.go Outdated Show resolved Hide resolved
cmd/podman/containers/ps.go Outdated Show resolved Hide resolved
cmd/podman/containers/ps.go Outdated Show resolved Hide resolved
docs/source/markdown/podman-pod-create.1.md.in Outdated Show resolved Hide resolved
docs/source/markdown/podman-pod-create.1.md.in Outdated Show resolved Hide resolved
libpod/define/container.go Outdated Show resolved Hide resolved
test/e2e/pod_create_test.go Outdated Show resolved Hide resolved
test/e2e/pod_create_test.go Outdated Show resolved Hide resolved
cmd/podman/containers/ps.go Outdated Show resolved Hide resolved
libpod/options.go Outdated Show resolved Hide resolved
@umohnani8
Copy link
Member Author

If we are to follow how kubernetes does restarts for pods, it doesn't restart the whole pod based on the policy, the containers in the pod are what is restarted. So when we set the restart policy for a pod, we are setting the restart policy for all the containers in the pod and a container in the pod cannot have a different policy set for it. If the whole pod needs to be restarted, k8s removes the pod and creates a new one in place of it and that is a whole different thing that happens when using Deployments or Jobs or when the user deletes the pod and the policy was set to Always.
https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#restart-policy

The design doc outlined the object of --restart for pod to apply it to the containers and have the containers restart based on the policy and there were no objections on that. We can continue the discussions in the design doc if needed.

@vrothberg
Copy link
Member

If we are to follow how kubernetes does restarts for pods, it doesn't restart the whole pod based on the policy, the containers in the pod are what is restarted. So when we set the restart policy for a pod, we are setting the restart policy for all the containers in the pod and a container in the pod cannot have a different policy set for it. If the whole pod needs to be restarted, k8s removes the pod and creates a new one in place of it and that is a whole different thing that happens when using Deployments or Jobs or when the user deletes the pod and the policy was set to Always. https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#restart-policy

I personally don't mind K8s in this context as it's outside of kube play.

The design doc outlined the object of --restart for pod to apply it to the containers and have the containers restart based on the policy and there were no objections on that. We can continue the discussions in the design doc if needed.

OK, let's do that 👍 I misinterpreted the part in the doc w.r.t "apply it to all containers". If the pod gets restarted it affects all its containers in a way.

@umohnani8 umohnani8 force-pushed the pod-restart branch 2 times, most recently from 458b5b7 to b888479 Compare March 20, 2023 14:01
cmd/podman/containers/ps.go Outdated Show resolved Hide resolved
@umohnani8 umohnani8 force-pushed the pod-restart branch 2 times, most recently from 2339656 to f002086 Compare March 20, 2023 17:13
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2023
@umohnani8
Copy link
Member Author

Once this is in, I will create a follow up PR for kube gen and play to start using the pod restart policy.

@umohnani8
Copy link
Member Author

@edsantiago @mheon is there a way for me to not list .RestartCount in the docs? I want the option the users know and use to be .Restarts.
This is the validation error I am getting https://github.com/containers/podman/pull/17627/checks?check_run_id=12662786279

@edsantiago
Copy link
Member

/me groans in agony.

@Luap99 you might be able to answer that more concisely and accurately than anything I could do.

@Luap99
Copy link
Member

Luap99 commented Apr 11, 2023

The reason this shows up is because it simply speaking a valid go template option. As soon a s you add a public field or function to a type it will be included in the shell completion which the validate script compare against.
As of now there is no way within the go code to explicitly hide a field or function. I agree that we may want to hide some fields, there is #17472 which goes into more detail but is very much not even close to be implement any time soon.

For now the simple solution is to just rename the field from RestartCount to Restarts on the ListContainer type and drop the Restarts function from the psReporter type. I think this should give you your desired result.

@umohnani8 umohnani8 force-pushed the pod-restart branch 2 times, most recently from a190a99 to 2569859 Compare April 13, 2023 18:16
@umohnani8 umohnani8 force-pushed the pod-restart branch 2 times, most recently from d1d49ed to 1f0b641 Compare May 1, 2023 17:18
@umohnani8
Copy link
Member Author

Tests are green, @containers/podman-maintainers PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM other than the final nits

cmd/podman/containers/create.go Show resolved Hide resolved
docs/source/markdown/podman-pod-clone.1.md.in Outdated Show resolved Hide resolved
docs/source/markdown/podman-pod-create.1.md.in Outdated Show resolved Hide resolved
pkg/specgen/generate/container_create.go Outdated Show resolved Hide resolved
umohnani8 added 4 commits May 2, 2023 10:29
Add --restart flag to pod create to allow users to set the
restart policy for the pod, which applies to all the containers
in the pod. This reuses the restart policy already there for
containers and has the same restart policy options.
Add "never" to the restart policy options to match k8s syntax.
It is a synonym for "no" and does the exact same thing where the
containers are not restarted once exited.
Only the containers that have exited will be restarted based on the
restart policy, running containers will not be restarted when an exited
container is restarted in the same pod (same as is done in k8s).

Signed-off-by: Urvashi Mohnani <[email protected]>
Add Restarts column to the podman ps output to show how many times a
container was restarted based on its restart policy. This column will be
displayed when --format={{.Restarts}}.

Signed-off-by: Urvashi Mohnani <[email protected]>
Add Restarts column to the podman pod ps output to show the total number
of times the containers in a pod were restarted. This is the same as the
restarts column displayed by kubernetes with kubectl get pods. This will
only be displayed when --format={{.Restarts}}.

Signed-off-by: Urvashi Mohnani <[email protected]>
Podman kube generate now uses the pod's restart policy
when generating the kube yaml. If generating from containers
only, use the restart policy of the first non-init container.
Podman kube play applies the pod restart policy from the yaml
file to the pod. The containers within a pod inherit this restart
policy.

Signed-off-by: Urvashi Mohnani <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented May 2, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2023
@openshift-merge-robot openshift-merge-robot merged commit 09c11a8 into containers:main May 2, 2023
@vrothberg
Copy link
Member

@mheon, no need to backport #18481 since the changes here did not make it into 4.5. The reports were against 4.6-dev, so we're good.

@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 Aug 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 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. 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.

7 participants