From edbeee523827b321cbee61c4002f4f4f05ea87f6 Mon Sep 17 00:00:00 2001 From: Urvashi Mohnani Date: Sun, 19 Mar 2023 20:07:19 -0400 Subject: [PATCH 1/4] Add --restart flag to pod create 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 --- cmd/podman/common/create.go | 16 +-- cmd/podman/containers/create.go | 2 + cmd/podman/pods/create.go | 1 + docs/source/markdown/options/restart.md | 3 +- docs/source/markdown/podman-pod-clone.1.md.in | 4 + .../source/markdown/podman-pod-create.1.md.in | 4 + .../markdown/podman-pod-inspect.1.md.in | 1 + libpod/define/pod_inspect.go | 2 + libpod/options.go | 34 ++++++ libpod/pod.go | 13 ++ libpod/pod_api.go | 1 + pkg/domain/entities/pods.go | 9 ++ pkg/specgen/generate/container_create.go | 18 ++- pkg/specgen/generate/pod_create.go | 4 + pkg/specgen/podspecgen.go | 11 ++ pkg/specgenutil/specgen.go | 25 +--- pkg/util/utils.go | 35 ++++++ test/e2e/pod_create_test.go | 112 ++++++++++++++++++ 18 files changed, 263 insertions(+), 32 deletions(-) diff --git a/cmd/podman/common/create.go b/cmd/podman/common/create.go index 78fd1d7216..c85de4d127 100644 --- a/cmd/podman/common/create.go +++ b/cmd/podman/common/create.go @@ -386,14 +386,6 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions, ) _ = cmd.RegisterFlagCompletionFunc(requiresFlagName, AutocompleteContainers) - restartFlagName := "restart" - createFlags.StringVar( - &cf.Restart, - restartFlagName, "", - `Restart policy to apply when a container exits ("always"|"no"|"on-failure"|"unless-stopped")`, - ) - _ = cmd.RegisterFlagCompletionFunc(restartFlagName, AutocompleteRestartOption) - createFlags.BoolVar( &cf.Rm, "rm", false, @@ -635,6 +627,14 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions, ) } if mode == entities.InfraMode || (mode == entities.CreateMode) { // infra container flags, create should also pick these up + restartFlagName := "restart" + createFlags.StringVar( + &cf.Restart, + restartFlagName, "", + `Restart policy to apply when a container exits ("always"|"no"|"never"|"on-failure"|"unless-stopped")`, + ) + _ = cmd.RegisterFlagCompletionFunc(restartFlagName, AutocompleteRestartOption) + shmSizeFlagName := "shm-size" createFlags.String( shmSizeFlagName, shmSize(), diff --git a/cmd/podman/containers/create.go b/cmd/podman/containers/create.go index e25c90f4d7..d6a6ff3773 100644 --- a/cmd/podman/containers/create.go +++ b/cmd/podman/containers/create.go @@ -137,6 +137,7 @@ func create(cmd *cobra.Command, args []string) error { } cliVals.InitContainerType = initctr } + // TODO: v5.0 block users from setting restart policy for a container if the container is in a pod cliVals, err := CreateInit(cmd, cliVals, false) if err != nil { @@ -405,6 +406,7 @@ func createPodIfNecessary(cmd *cobra.Command, s *specgen.SpecGenerator, netOpts CpusetCpus: cliVals.CPUSetCPUs, Pid: cliVals.PID, Userns: uns, + Restart: cliVals.Restart, } // Unset config values we passed to the pod to prevent them being used twice for the container and pod. s.ContainerBasicConfig.Hostname = "" diff --git a/cmd/podman/pods/create.go b/cmd/podman/pods/create.go index db1eec07b6..5cc4505ff2 100644 --- a/cmd/podman/pods/create.go +++ b/cmd/podman/pods/create.go @@ -259,6 +259,7 @@ func create(cmd *cobra.Command, args []string) error { podSpec.InfraContainerSpec = specgen.NewSpecGenerator(imageName, false) podSpec.InfraContainerSpec.RawImageName = rawImageName podSpec.InfraContainerSpec.NetworkOptions = podSpec.NetworkOptions + podSpec.InfraContainerSpec.RestartPolicy = podSpec.RestartPolicy err = specgenutil.FillOutSpecGen(podSpec.InfraContainerSpec, &infraOptions, []string{}) if err != nil { return err diff --git a/docs/source/markdown/options/restart.md b/docs/source/markdown/options/restart.md index 3a5e665c12..7d0219600e 100644 --- a/docs/source/markdown/options/restart.md +++ b/docs/source/markdown/options/restart.md @@ -1,5 +1,5 @@ ####> This option file is used in: -####> podman create, run +####> podman create, pod clone, pod create, run ####> If file is edited, make sure the changes ####> are applicable to all of those. #### **--restart**=*policy* @@ -10,6 +10,7 @@ Restart policy will not take effect if a container is stopped via the **podman k Valid _policy_ values are: - `no` : Do not restart containers on exit +- `never` : Synonym for **no**; do not restart containers on exit - `on-failure[:max_retries]` : Restart containers when they exit with a non-zero exit code, retrying indefinitely or until the optional *max_retries* count is hit - `always` : Restart containers when they exit, regardless of status, retrying indefinitely - `unless-stopped` : Identical to **always** diff --git a/docs/source/markdown/podman-pod-clone.1.md.in b/docs/source/markdown/podman-pod-clone.1.md.in index a474436ee4..a442be80c9 100644 --- a/docs/source/markdown/podman-pod-clone.1.md.in +++ b/docs/source/markdown/podman-pod-clone.1.md.in @@ -67,6 +67,10 @@ Set a custom name for the cloned pod. The default if not specified is of the syn @@option pid.pod +@@option restart + +Default restart policy for all the containers in a pod. + @@option security-opt @@option shm-size diff --git a/docs/source/markdown/podman-pod-create.1.md.in b/docs/source/markdown/podman-pod-create.1.md.in index 322c637813..28ce755316 100644 --- a/docs/source/markdown/podman-pod-create.1.md.in +++ b/docs/source/markdown/podman-pod-create.1.md.in @@ -143,6 +143,10 @@ but only by the pod itself. @@option replace +@@option restart + +Default restart policy for all the containers in a pod. + @@option security-opt #### **--share**=*namespace* diff --git a/docs/source/markdown/podman-pod-inspect.1.md.in b/docs/source/markdown/podman-pod-inspect.1.md.in index b43f6977ce..e4697dcf1f 100644 --- a/docs/source/markdown/podman-pod-inspect.1.md.in +++ b/docs/source/markdown/podman-pod-inspect.1.md.in @@ -50,6 +50,7 @@ Valid placeholders for the Go template are listed below: | .Name | Pod name | | .Namespace | Namespace | | .NumContainers | Number of containers in the pod | +| .RestartPolicy | Restart policy of the pod | | .SecurityOpts | Security options | | .SharedNamespaces | Pod shared namespaces | | .State | Pod state | diff --git a/libpod/define/pod_inspect.go b/libpod/define/pod_inspect.go index dc82af2201..9607bf868d 100644 --- a/libpod/define/pod_inspect.go +++ b/libpod/define/pod_inspect.go @@ -83,6 +83,8 @@ type InspectPodData struct { BlkioWeight uint64 `json:"blkio_weight,omitempty"` // BlkioWeightDevice contains the blkio weight device limits for the pod BlkioWeightDevice []InspectBlkioWeightDevice `json:"blkio_weight_device,omitempty"` + // RestartPolicy of the pod. + RestartPolicy string `json:"RestartPolicy,omitempty"` } // InspectPodInfraConfig contains the configuration of the pod's infra diff --git a/libpod/options.go b/libpod/options.go index bc70e4a32c..a974caeebf 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -2020,6 +2020,40 @@ func WithPodExitPolicy(policy string) PodCreateOption { } } +// WithPodRestartPolicy sets the restart policy of the pod. +func WithPodRestartPolicy(policy string) PodCreateOption { + return func(pod *Pod) error { + if pod.valid { + return define.ErrPodFinalized + } + + switch policy { + //TODO: v5.0 if no restart policy is set, follow k8s convention and default to Always + case define.RestartPolicyNone, define.RestartPolicyNo, define.RestartPolicyOnFailure, define.RestartPolicyAlways, define.RestartPolicyUnlessStopped: + pod.config.RestartPolicy = policy + default: + return fmt.Errorf("%q is not a valid restart policy: %w", policy, define.ErrInvalidArg) + } + + return nil + } +} + +// WithPodRestartRetries sets the number of retries to use when restarting a +// container with the "on-failure" restart policy. +// 0 is an allowed value, and indicates infinite retries. +func WithPodRestartRetries(tries uint) PodCreateOption { + return func(pod *Pod) error { + if pod.valid { + return define.ErrPodFinalized + } + + pod.config.RestartRetries = &tries + + return nil + } +} + // WithPodHostname sets the hostname of the pod. func WithPodHostname(hostname string) PodCreateOption { return func(pod *Pod) error { diff --git a/libpod/pod.go b/libpod/pod.go index e9ce876373..cdd0f3d715 100644 --- a/libpod/pod.go +++ b/libpod/pod.go @@ -81,6 +81,12 @@ type PodConfig struct { // The pod's exit policy. ExitPolicy config.PodExitPolicy `json:"ExitPolicy,omitempty"` + // The pod's restart policy + RestartPolicy string `json:"RestartPolicy,omitempty"` + + // The max number of retries for a pod based on restart policy + RestartRetries *uint `json:"RestartRetries,omitempty"` + // ID of the pod's lock LockID uint32 `json:"lockID"` @@ -522,3 +528,10 @@ func (p *Pod) Config() (*PodConfig, error) { return conf, err } + +// ConfigNoCopy returns the configuration used by the pod. +// Note that the returned value is not a copy and must hence +// only be used in a reading fashion. +func (p *Pod) ConfigNoCopy() *PodConfig { + return p.config +} diff --git a/libpod/pod_api.go b/libpod/pod_api.go index b5b676b40e..f8e70cc8f5 100644 --- a/libpod/pod_api.go +++ b/libpod/pod_api.go @@ -741,6 +741,7 @@ func (p *Pod) Inspect() (*define.InspectPodData, error) { CPUSetMems: p.CPUSetMems(), BlkioDeviceWriteBps: p.BlkiThrottleWriteBps(), CPUShares: p.CPUShares(), + RestartPolicy: p.config.RestartPolicy, } return &inspectData, nil diff --git a/pkg/domain/entities/pods.go b/pkg/domain/entities/pods.go index b0ff5cdaf4..eeda81ef64 100644 --- a/pkg/domain/entities/pods.go +++ b/pkg/domain/entities/pods.go @@ -135,6 +135,7 @@ type PodCreateOptions struct { Net *NetOptions `json:"net,omitempty"` Share []string `json:"share,omitempty"` ShareParent *bool `json:"share_parent,omitempty"` + Restart string `json:"restart,omitempty"` Pid string `json:"pid,omitempty"` Cpus float64 `json:"cpus,omitempty"` CpusetCpus string `json:"cpuset_cpus,omitempty"` @@ -375,6 +376,14 @@ func ToPodSpecGen(s specgen.PodSpecGenerator, p *PodCreateOptions) (*specgen.Pod s.ShareParent = p.ShareParent s.PodCreateCommand = p.CreateCommand s.VolumesFrom = p.VolumesFrom + if p.Restart != "" { + policy, retries, err := util.ParseRestartPolicy(p.Restart) + if err != nil { + return nil, err + } + s.RestartPolicy = policy + s.RestartRetries = &retries + } // Networking config diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index 5964607fa1..266c4c5883 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -555,12 +555,24 @@ func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *l } // Default used if not overridden on command line - if s.RestartPolicy != "" { + var ( + restartPolicy string + retries uint + ) + // If the container is running in a pod, use the pod's restart policy for all the containers + if pod != nil { + podConfig := pod.ConfigNoCopy() + if podConfig.RestartRetries != nil { + retries = *podConfig.RestartRetries + } + restartPolicy = podConfig.RestartPolicy + } else if s.RestartPolicy != "" { if s.RestartRetries != nil { - options = append(options, libpod.WithRestartRetries(*s.RestartRetries)) + retries = *s.RestartRetries } - options = append(options, libpod.WithRestartPolicy(s.RestartPolicy)) + restartPolicy = s.RestartPolicy } + options = append(options, libpod.WithRestartRetries(retries), libpod.WithRestartPolicy(restartPolicy)) if s.ContainerHealthCheckConfig.HealthConfig != nil { options = append(options, libpod.WithHealthCheck(s.ContainerHealthCheckConfig.HealthConfig)) diff --git a/pkg/specgen/generate/pod_create.go b/pkg/specgen/generate/pod_create.go index 6d45a728e1..03f15f1903 100644 --- a/pkg/specgen/generate/pod_create.go +++ b/pkg/specgen/generate/pod_create.go @@ -165,6 +165,10 @@ func createPodOptions(p *specgen.PodSpecGenerator) ([]libpod.PodCreateOption, er } options = append(options, libpod.WithPodExitPolicy(p.ExitPolicy)) + options = append(options, libpod.WithPodRestartPolicy(p.RestartPolicy)) + if p.RestartRetries != nil { + options = append(options, libpod.WithPodRestartRetries(*p.RestartRetries)) + } return options, nil } diff --git a/pkg/specgen/podspecgen.go b/pkg/specgen/podspecgen.go index 2d855bfae1..5c8e561ecc 100644 --- a/pkg/specgen/podspecgen.go +++ b/pkg/specgen/podspecgen.go @@ -64,6 +64,17 @@ type PodBasicConfig struct { // Conflicts with NoInfra=true. // Optional. SharedNamespaces []string `json:"shared_namespaces,omitempty"` + // RestartPolicy is the pod's restart policy - an action which + // will be taken when one or all the containers in the pod exits. + // If not given, the default policy will be set to Always, which + // restarts the containers in the pod when they exit indefinitely. + // Optional. + RestartPolicy string `json:"restart_policy,omitempty"` + // RestartRetries is the number of attempts that will be made to restart + // the container. + // Only available when RestartPolicy is set to "on-failure". + // Optional. + RestartRetries *uint `json:"restart_tries,omitempty"` // PodCreateCommand is the command used to create this pod. // This will be shown in the output of Inspect() on the pod, and may // also be used by some tools that wish to recreate the pod diff --git a/pkg/specgenutil/specgen.go b/pkg/specgenutil/specgen.go index 409018086d..903232e448 100644 --- a/pkg/specgenutil/specgen.go +++ b/pkg/specgenutil/specgen.go @@ -808,27 +808,12 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions s.OOMScoreAdj = c.OOMScoreAdj } if c.Restart != "" { - splitRestart := strings.Split(c.Restart, ":") - switch len(splitRestart) { - case 1: - // No retries specified - case 2: - if strings.ToLower(splitRestart[0]) != "on-failure" { - return errors.New("restart policy retries can only be specified with on-failure restart policy") - } - retries, err := strconv.Atoi(splitRestart[1]) - if err != nil { - return fmt.Errorf("parsing restart policy retry count: %w", err) - } - if retries < 0 { - return errors.New("must specify restart policy retry count as a number greater than 0") - } - var retriesUint = uint(retries) - s.RestartRetries = &retriesUint - default: - return errors.New("invalid restart policy: may specify retries at most once") + policy, retries, err := util.ParseRestartPolicy(c.Restart) + if err != nil { + return err } - s.RestartPolicy = splitRestart[0] + s.RestartPolicy = policy + s.RestartRetries = &retries } if len(s.Secrets) == 0 || len(c.Secrets) != 0 { diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 69e23fcd01..4aab28964c 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -21,6 +21,7 @@ import ( "github.com/containers/image/v5/types" encconfig "github.com/containers/ocicrypt/config" enchelpers "github.com/containers/ocicrypt/helpers" + "github.com/containers/podman/v4/libpod/define" "github.com/containers/podman/v4/pkg/errorhandling" "github.com/containers/podman/v4/pkg/namespaces" "github.com/containers/podman/v4/pkg/rootless" @@ -665,3 +666,37 @@ func DecryptConfig(decryptionKeys []string) (*encconfig.DecryptConfig, error) { return decryptConfig, nil } + +// ParseRestartPolicy parses the value given to the --restart flag and returns the policy +// and restart retries value +func ParseRestartPolicy(policy string) (string, uint, error) { + var ( + retriesUint uint + policyType string + ) + splitRestart := strings.Split(policy, ":") + switch len(splitRestart) { + case 1: + // No retries specified + policyType = splitRestart[0] + if strings.ToLower(splitRestart[0]) == "never" { + policyType = define.RestartPolicyNo + } + case 2: + if strings.ToLower(splitRestart[0]) != "on-failure" { + return "", 0, errors.New("restart policy retries can only be specified with on-failure restart policy") + } + retries, err := strconv.Atoi(splitRestart[1]) + if err != nil { + return "", 0, fmt.Errorf("parsing restart policy retry count: %w", err) + } + if retries < 0 { + return "", 0, errors.New("must specify restart policy retry count as a number greater than 0") + } + retriesUint = uint(retries) + policyType = splitRestart[0] + default: + return "", 0, errors.New("invalid restart policy: may specify retries at most once") + } + return policyType, retriesUint, nil +} diff --git a/test/e2e/pod_create_test.go b/test/e2e/pod_create_test.go index 008eb3543f..bff398721f 100644 --- a/test/e2e/pod_create_test.go +++ b/test/e2e/pod_create_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strconv" "strings" + "time" "github.com/containers/common/pkg/apparmor" "github.com/containers/common/pkg/seccomp" @@ -1212,4 +1213,115 @@ ENTRYPOINT ["sleep","99999"] Expect(inspect).Should(Exit(0)) Expect(inspect.OutputToString()).Should(Equal(pod2Name)) }) + + It("podman pod create --restart set to default", func() { + // When the --restart flag is not set, the default value is No + // TODO: v5.0 change this so that the default value is Always + podName := "mypod" + testCtr := "ctr1" + session := podmanTest.Podman([]string{"pod", "create", podName}) + session.WaitWithDefaultTimeout() + Expect(session).Should(Exit(0)) + // add container to pod + ctrRun := podmanTest.Podman([]string{"run", "--name", testCtr, "-d", "--pod", podName, ALPINE, "echo", "hello"}) + ctrRun.WaitWithDefaultTimeout() + Expect(ctrRun).Should(Exit(0)) + // Wait about 1 second, so we can check the number of restarts as default restart policy is set to No + time.Sleep(1 * time.Second) + ps := podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + testCtr, "--format", "{{.Restarts}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + restarts, err := strconv.Atoi(ps.OutputToString()) + Expect(err).ToNot(HaveOccurred()) + Expect(restarts).To(BeNumerically("==", 0)) + ps = podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + testCtr, "--format", "{{.Status}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + Expect(ps.OutputToString()).To(ContainSubstring("Exited")) + }) + + It("podman pod create --restart=on-failure", func() { + // Restart policy set to on-failure with max 2 retries + podName := "mypod" + runningCtr := "ctr1" + testCtr := "ctr2" + session := podmanTest.Podman([]string{"pod", "create", "--restart", "on-failure:2", podName}) + session.WaitWithDefaultTimeout() + Expect(session).Should(Exit(0)) + // add container to pod + ctrRun := podmanTest.Podman([]string{"run", "--name", runningCtr, "-d", "--pod", podName, ALPINE, "sleep", "100"}) + ctrRun.WaitWithDefaultTimeout() + Expect(ctrRun).Should(Exit(0)) + ctrRun = podmanTest.Podman([]string{"run", "--name", testCtr, "-d", "--pod", podName, ALPINE, "sh", "-c", "echo hello && exit 1"}) + ctrRun.WaitWithDefaultTimeout() + Expect(ctrRun).Should(Exit(0)) + // Wait about 2 seconds, so we can check the number of restarts after failure + time.Sleep(2 * time.Second) + ps := podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + testCtr, "--format", "{{.Restarts}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + restarts, err := strconv.Atoi(ps.OutputToString()) + Expect(err).ToNot(HaveOccurred()) + Expect(restarts).To(BeNumerically("==", 2)) + ps = podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + testCtr, "--format", "{{.Status}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + Expect(ps.OutputToString()).To(ContainSubstring("Exited")) + ps = podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + runningCtr, "--format", "{{.Status}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + Expect(ps.OutputToString()).To(ContainSubstring("Up")) + }) + + It("podman pod create --restart=no/never", func() { + // never and no are the same, just different words to do the same thing + policy := []string{"no", "never"} + for _, p := range policy { + podName := "mypod-" + p + runningCtr := "ctr1-" + p + testCtr := "ctr2-" + p + testCtr2 := "ctr3-" + p + session := podmanTest.Podman([]string{"pod", "create", "--restart", p, podName}) + session.WaitWithDefaultTimeout() + Expect(session).Should(Exit(0)) + // add container to pod + ctrRun := podmanTest.Podman([]string{"run", "--name", runningCtr, "-d", "--pod", podName, ALPINE, "sleep", "100"}) + ctrRun.WaitWithDefaultTimeout() + Expect(ctrRun).Should(Exit(0)) + ctrRun = podmanTest.Podman([]string{"run", "--name", testCtr, "-d", "--pod", podName, ALPINE, "echo", "hello"}) + ctrRun.WaitWithDefaultTimeout() + Expect(ctrRun).Should(Exit(0)) + ctrRun = podmanTest.Podman([]string{"run", "--name", testCtr2, "-d", "--pod", podName, ALPINE, "sh", "-c", "echo hello && exit 1"}) + ctrRun.WaitWithDefaultTimeout() + Expect(ctrRun).Should(Exit(0)) + // Wait 1 second, so we can check the number of restarts and make sure the container has actually ran + time.Sleep(1 * time.Second) + // check first test container - container exits with exit code 0 + ps := podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + testCtr, "--format", "{{.Restarts}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + restarts, err := strconv.Atoi(ps.OutputToString()) + Expect(err).ToNot(HaveOccurred()) + Expect(restarts).To(BeNumerically("==", 0)) + ps = podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + testCtr, "--format", "{{.Status}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + Expect(ps.OutputToString()).To(ContainSubstring("Exited")) + // Check second test container - container exits with non-zero exit code + ps = podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + testCtr2, "--format", "{{.Restarts}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + restarts, err = strconv.Atoi(ps.OutputToString()) + Expect(err).ToNot(HaveOccurred()) + Expect(restarts).To(BeNumerically("==", 0)) + ps = podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + testCtr2, "--format", "{{.Status}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + Expect(ps.OutputToString()).To(ContainSubstring("Exited")) + ps = podmanTest.Podman([]string{"ps", "-a", "--filter", "name=" + runningCtr, "--format", "{{.Status}}"}) + ps.WaitWithDefaultTimeout() + Expect(ps).Should(Exit(0)) + Expect(ps.OutputToString()).To(ContainSubstring("Up")) + } + }) }) From 0fef113a4ba23044bbf54533eaa2a298bd8e8846 Mon Sep 17 00:00:00 2001 From: Urvashi Mohnani Date: Sun, 19 Mar 2023 20:16:38 -0400 Subject: [PATCH 2/4] Add {{.Restarts}} to podman ps 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 --- cmd/podman/containers/ps.go | 6 ++++++ docs/source/markdown/podman-ps.1.md | 1 + libpod/container.go | 13 ++++++++++++- pkg/domain/entities/container_ps.go | 4 ++++ pkg/ps/ps.go | 7 +++++++ 5 files changed, 30 insertions(+), 1 deletion(-) diff --git a/cmd/podman/containers/ps.go b/cmd/podman/containers/ps.go index 09f73ffd22..621cc23e5e 100644 --- a/cmd/podman/containers/ps.go +++ b/cmd/podman/containers/ps.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "strconv" "strings" "time" @@ -300,6 +301,7 @@ func createPsOut() ([]map[string]string, string) { "PIDNS": "pidns", "Pod": "pod id", "PodName": "podname", // undo camelcase space break + "Restarts": "restarts", "RunningFor": "running for", "UTS": "uts", "User": "userns", @@ -377,6 +379,10 @@ func (l psReporter) Status() string { return state } +func (l psReporter) Restarts() string { + return strconv.Itoa(int(l.ListContainer.Restarts)) +} + func (l psReporter) RunningFor() string { return l.CreatedHuman() } diff --git a/docs/source/markdown/podman-ps.1.md b/docs/source/markdown/podman-ps.1.md index 43a5eb82cb..efe7a15c27 100644 --- a/docs/source/markdown/podman-ps.1.md +++ b/docs/source/markdown/podman-ps.1.md @@ -89,6 +89,7 @@ Valid placeholders for the Go template are listed below: | .Pod | Pod the container is associated with (SHA) | | .PodName | Seems to be empty no matter what | | .Ports | Exposed ports | +| .Restarts | Display the container restart count | | .RunningFor | Time elapsed since container was started | | .Size | Size of container | | .StartedAt | Time (epoch seconds) the container started | diff --git a/libpod/container.go b/libpod/container.go index b92255d7ed..640e24a5f9 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -203,7 +203,6 @@ type ContainerState struct { // restart policy. This is NOT incremented by normal container restarts // (only by restart policy). RestartCount uint `json:"restartCount,omitempty"` - // StartupHCPassed indicates that the startup healthcheck has // succeeded and the main healthcheck can begin. StartupHCPassed bool `json:"startupHCPassed,omitempty"` @@ -730,6 +729,18 @@ func (c *Container) State() (define.ContainerStatus, error) { return c.state.State, nil } +func (c *Container) RestartCount() (uint, error) { + if !c.batched { + c.lock.Lock() + defer c.lock.Unlock() + + if err := c.syncContainer(); err != nil { + return 0, err + } + } + return c.state.RestartCount, nil +} + // Mounted returns whether the container is mounted and the path it is mounted // at (if it is mounted). // If the container is not mounted, no error is returned, and the mountpoint diff --git a/pkg/domain/entities/container_ps.go b/pkg/domain/entities/container_ps.go index 519d2b7da7..93fdae7470 100644 --- a/pkg/domain/entities/container_ps.go +++ b/pkg/domain/entities/container_ps.go @@ -55,6 +55,10 @@ type ListContainer struct { PodName string // Port mappings Ports []types.PortMapping + // Restarts is how many times the container was restarted by its + // restart policy. This is NOT incremented by normal container restarts + // (only by restart policy). + Restarts uint // Size of the container rootfs. Requires the size boolean to be true Size *define.ContainerSize // Time when container started diff --git a/pkg/ps/ps.go b/pkg/ps/ps.go index 44a1568806..abbe38f1c5 100644 --- a/pkg/ps/ps.go +++ b/pkg/ps/ps.go @@ -145,6 +145,7 @@ func ListContainerBatch(rt *libpod.Runtime, ctr *libpod.Container, opts entities portMappings []libnetworkTypes.PortMapping networks []string healthStatus string + restartCount uint ) batchErr := ctr.Batch(func(c *libpod.Container) error { @@ -193,6 +194,11 @@ func ListContainerBatch(rt *libpod.Runtime, ctr *libpod.Container, opts entities return err } + restartCount, err = c.RestartCount() + if err != nil { + return err + } + if !opts.Size && !opts.Namespace { return nil } @@ -251,6 +257,7 @@ func ListContainerBatch(rt *libpod.Runtime, ctr *libpod.Container, opts entities StartedAt: startedTime.Unix(), State: conState.String(), Status: healthStatus, + Restarts: restartCount, } if opts.Pod && len(conConfig.Pod) > 0 { podName, err := rt.GetPodName(conConfig.Pod) From db4ad54f92ecf3636abd52f7b0d99c7f145db9d7 Mon Sep 17 00:00:00 2001 From: Urvashi Mohnani Date: Sun, 19 Mar 2023 20:17:42 -0400 Subject: [PATCH 3/4] Add {{.Restarts}} to podman pod ps 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 --- cmd/podman/pods/ps.go | 12 +++++++++ docs/source/markdown/podman-pod-ps.1.md.in | 29 +++++++++++----------- pkg/domain/entities/pods.go | 7 +++--- pkg/domain/infra/abi/pods.go | 11 +++++--- 4 files changed, 39 insertions(+), 20 deletions(-) diff --git a/cmd/podman/pods/ps.go b/cmd/podman/pods/ps.go index 7ba9403f41..405d7cdc03 100644 --- a/cmd/podman/pods/ps.go +++ b/cmd/podman/pods/ps.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "sort" + "strconv" "strings" "time" @@ -147,6 +148,7 @@ func pods(cmd *cobra.Command, _ []string) error { "ContainerStatuses": "STATUS", "Cgroup": "CGROUP", "Namespace": "NAMESPACES", + "Restarts": "RESTARTS", }) if err := rpt.Execute(headers); err != nil { @@ -178,6 +180,7 @@ func podPsFormat() string { if !psInput.CtrStatus && !psInput.CtrNames && !psInput.CtrIds { row = append(row, "{{.NumberOfContainers}}") } + return "{{range . }}" + strings.Join(row, "\t") + "\n" + "{{end -}}" } @@ -265,6 +268,15 @@ func (l ListPodReporter) ContainerStatuses() string { return strings.Join(statuses, ",") } +// Restarts returns the total number of restarts for all the containers in the pod +func (l ListPodReporter) Restarts() string { + restarts := 0 + for _, c := range l.Containers { + restarts += int(c.RestartCount) + } + return strconv.Itoa(restarts) +} + func sortPodPsOutput(sortBy string, lprs []*entities.ListPodsReport) error { switch sortBy { case "created": diff --git a/docs/source/markdown/podman-pod-ps.1.md.in b/docs/source/markdown/podman-pod-ps.1.md.in index a0d99a722a..6e369119ff 100644 --- a/docs/source/markdown/podman-pod-ps.1.md.in +++ b/docs/source/markdown/podman-pod-ps.1.md.in @@ -77,20 +77,21 @@ Pretty-print containers to JSON or using a Go template Valid placeholders for the Go template are listed below: -| **Placeholder** | **Description** | -|---------------------|----------------------------------------------------| -| .Cgroup | Cgroup path of pod | -| .ContainerIds | Comma-separated list of container IDs in the pod | -| .ContainerNames | Comma-separated list of container names in the pod | -| .ContainerStatuses | Comma-separated list of container statuses | -| .Created | Creation time of pod | -| .ID | Container ID | -| .InfraID | Pod infra container ID | -| .Labels | All the labels assigned to the pod | -| .Name | Name of pod | -| .Networks | Show all networks connected to the infra container | -| .NumberOfContainers | Show the number of containers attached to pod | -| .Status | Status of pod | +| **Placeholder** | **Description** | +|---------------------|------------------------------------------------------| +| .Cgroup | Cgroup path of pod | +| .ContainerIds | Comma-separated list of container IDs in the pod | +| .ContainerNames | Comma-separated list of container names in the pod | +| .ContainerStatuses | Comma-separated list of container statuses | +| .Created | Creation time of pod | +| .ID | Container ID | +| .InfraID | Pod infra container ID | +| .Labels | All the labels assigned to the pod | +| .Name | Name of pod | +| .Networks | Show all networks connected to the infra container | +| .NumberOfContainers | Show the number of containers attached to pod | +| .Restarts | Show the total number of container restarts in a pod | +| .Status | Status of pod | #### **--help**, **-h** diff --git a/pkg/domain/entities/pods.go b/pkg/domain/entities/pods.go index eeda81ef64..585b0e6ca5 100644 --- a/pkg/domain/entities/pods.go +++ b/pkg/domain/entities/pods.go @@ -38,9 +38,10 @@ type ListPodsReport struct { } type ListPodContainer struct { - Id string //nolint:revive,stylecheck - Names string - Status string + Id string //nolint:revive,stylecheck + Names string + Status string + RestartCount uint } type PodPauseOptions struct { diff --git a/pkg/domain/infra/abi/pods.go b/pkg/domain/infra/abi/pods.go index 4b269d9258..e54e287d7a 100644 --- a/pkg/domain/infra/abi/pods.go +++ b/pkg/domain/infra/abi/pods.go @@ -429,10 +429,15 @@ func (ic *ContainerEngine) listPodReportFromPod(p *libpod.Pod) (*entities.ListPo if err != nil { return nil, err } + restartCount, err := c.RestartCount() + if err != nil { + return nil, err + } lpcs[i] = &entities.ListPodContainer{ - Id: c.ID(), - Names: c.Name(), - Status: state.String(), + Id: c.ID(), + Names: c.Name(), + Status: state.String(), + RestartCount: restartCount, } } infraID, err := p.InfraContainerID() From fa1ba17bc16fd3e4be650f436784ed7cef2f159f Mon Sep 17 00:00:00 2001 From: Urvashi Mohnani Date: Thu, 13 Apr 2023 13:45:28 -0400 Subject: [PATCH 4/4] Update kube gen & play to use pod restart policy 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 --- libpod/kube.go | 54 +++++++++++++++---------- pkg/domain/infra/abi/play.go | 11 +++-- test/e2e/generate_kube_test.go | 73 +++++++++++++++++++++++++++++++--- test/e2e/play_kube_test.go | 2 +- 4 files changed, 108 insertions(+), 32 deletions(-) diff --git a/libpod/kube.go b/libpod/kube.go index 11fb8da856..093760de72 100644 --- a/libpod/kube.go +++ b/libpod/kube.go @@ -97,23 +97,8 @@ func (p *Pod) GenerateForKube(ctx context.Context, getService bool) (*v1.Pod, [] } pod.Spec.HostAliases = extraHost - // vendor/k8s.io/api/core/v1/types.go: v1.Container cannot save restartPolicy - // so set it at here - for _, ctr := range allContainers { - if !ctr.IsInfra() { - switch ctr.config.RestartPolicy { - case define.RestartPolicyAlways: - pod.Spec.RestartPolicy = v1.RestartPolicyAlways - case define.RestartPolicyOnFailure: - pod.Spec.RestartPolicy = v1.RestartPolicyOnFailure - case define.RestartPolicyNo: - pod.Spec.RestartPolicy = v1.RestartPolicyNever - default: // some pod create from cmdline, such as "", so set it to "" as k8s automatically defaults to always - pod.Spec.RestartPolicy = "" - } - break - } - } + // Set the pod's restart policy + pod.Spec.RestartPolicy = getPodRestartPolicy(p.config.RestartPolicy) if p.SharesPID() { // unfortunately, go doesn't have a nice way to specify a pointer to a bool @@ -136,7 +121,7 @@ func (p *Pod) getInfraContainer() (*Container, error) { // kind YAML. func GenerateForKubeDeployment(ctx context.Context, pod *YAMLPod, options entities.GenerateKubeOptions) (*YAMLDeployment, error) { // Restart policy for Deployments can only be set to Always - if options.Type == define.K8sKindDeployment && !(pod.Spec.RestartPolicy == "" || pod.Spec.RestartPolicy == define.RestartPolicyAlways) { + if options.Type == define.K8sKindDeployment && !(pod.Spec.RestartPolicy == "" || pod.Spec.RestartPolicy == v1.RestartPolicyAlways) { return nil, fmt.Errorf("k8s Deployments can only have restartPolicy set to Always") } @@ -599,6 +584,7 @@ func simplePodWithV1Containers(ctx context.Context, ctrs []*Container, getServic kubeAnnotations := make(map[string]string) ctrNames := make([]string, 0, len(ctrs)) var hostname string + var restartPolicy *string for _, ctr := range ctrs { ctrNames = append(ctrNames, removeUnderscores(ctr.Name())) for k, v := range ctr.config.Spec.Annotations { @@ -623,6 +609,11 @@ func simplePodWithV1Containers(ctx context.Context, ctrs []*Container, getServic } } + // Use the restart policy of the first non-init container + if !isInit && restartPolicy == nil { + restartPolicy = &ctr.config.RestartPolicy + } + if ctr.config.Spec.Process != nil { var ulimitArr []string defaultUlimits := util.DefaultContainerConfig().Ulimits() @@ -700,7 +691,7 @@ func simplePodWithV1Containers(ctx context.Context, ctrs []*Container, getServic podName += "-pod" } - return newPodObject( + pod := newPodObject( podName, kubeAnnotations, kubeInitCtrs, @@ -709,7 +700,30 @@ func simplePodWithV1Containers(ctx context.Context, ctrs []*Container, getServic &podDNS, hostNetwork, hostUsers, - hostname), nil + hostname) + + // Set the pod's restart policy + policy := "" + if restartPolicy != nil { + policy = *restartPolicy + } + pod.Spec.RestartPolicy = getPodRestartPolicy(policy) + + return pod, nil +} + +// getPodRestartPolicy returns the pod restart policy to be set in the generated kube yaml +func getPodRestartPolicy(policy string) v1.RestartPolicy { + switch policy { + case define.RestartPolicyNo: + return v1.RestartPolicyNever + case define.RestartPolicyAlways: + return v1.RestartPolicyAlways + case define.RestartPolicyOnFailure: + return v1.RestartPolicyOnFailure + default: // some pod/ctr create from cmdline, such as "" - set it to "" and let k8s handle the defaults + return "" + } } // containerToV1Container converts information we know about a libpod container diff --git a/pkg/domain/infra/abi/play.go b/pkg/domain/infra/abi/play.go index dfcdb0ce70..ce602b5e74 100644 --- a/pkg/domain/infra/abi/play.go +++ b/pkg/domain/infra/abi/play.go @@ -592,16 +592,16 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY return nil, nil, err } - var ctrRestartPolicy string + // Set the restart policy from the kube yaml at the pod level in podman switch podYAML.Spec.RestartPolicy { case v1.RestartPolicyAlways: - ctrRestartPolicy = define.RestartPolicyAlways + podSpec.PodSpecGen.RestartPolicy = define.RestartPolicyAlways case v1.RestartPolicyOnFailure: - ctrRestartPolicy = define.RestartPolicyOnFailure + podSpec.PodSpecGen.RestartPolicy = define.RestartPolicyOnFailure case v1.RestartPolicyNever: - ctrRestartPolicy = define.RestartPolicyNo + podSpec.PodSpecGen.RestartPolicy = define.RestartPolicyNo default: // Default to Always - ctrRestartPolicy = define.RestartPolicyAlways + podSpec.PodSpecGen.RestartPolicy = define.RestartPolicyAlways } if podOpt.Infra { @@ -775,7 +775,6 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY PodName: podName, PodSecurityContext: podYAML.Spec.SecurityContext, ReadOnly: readOnly, - RestartPolicy: ctrRestartPolicy, SeccompPaths: seccompPaths, SecretsManager: secretsManager, UserNSIsHost: p.Userns.IsHost(), diff --git a/test/e2e/generate_kube_test.go b/test/e2e/generate_kube_test.go index ee7339c7e9..12ae1ff340 100644 --- a/test/e2e/generate_kube_test.go +++ b/test/e2e/generate_kube_test.go @@ -528,18 +528,21 @@ var _ = Describe("Podman kube generate", func() { Expect(err).ToNot(HaveOccurred()) }) - It("podman generate kube on pod with restartPolicy", func() { + It("podman generate kube on pod with restartPolicy set for container in a pod", func() { + //TODO: v5.0 - change/remove test once we block --restart on container when it is in a pod // podName, set, expect testSli := [][]string{ {"testPod1", "", ""}, // some pod create from cmdline, so set it to an empty string and let k8s default it to Always {"testPod2", "always", "Always"}, {"testPod3", "on-failure", "OnFailure"}, {"testPod4", "no", "Never"}, + {"testPod5", "never", "Never"}, } for k, v := range testSli { podName := v[0] - podSession := podmanTest.Podman([]string{"pod", "create", "--name", podName}) + // Need to set --restart during pod creation as gen kube only picks up the pod's restart policy + podSession := podmanTest.Podman([]string{"pod", "create", "--restart", v[1], "--name", podName}) podSession.WaitWithDefaultTimeout() Expect(podSession).Should(Exit(0)) @@ -561,6 +564,67 @@ var _ = Describe("Podman kube generate", func() { } }) + It("podman generate kube on pod with restartPolicy", func() { + // podName, set, expect + testSli := [][]string{ + {"testPod1", "", ""}, + {"testPod2", "always", "Always"}, + {"testPod3", "on-failure", "OnFailure"}, + {"testPod4", "no", "Never"}, + {"testPod5", "never", "Never"}, + } + + for k, v := range testSli { + podName := v[0] + podSession := podmanTest.Podman([]string{"pod", "create", "--restart", v[1], podName}) + podSession.WaitWithDefaultTimeout() + Expect(podSession).Should(Exit(0)) + + ctrName := "ctr" + strconv.Itoa(k) + ctr1Session := podmanTest.Podman([]string{"create", "--name", ctrName, "--pod", podName, ALPINE, "top"}) + ctr1Session.WaitWithDefaultTimeout() + Expect(ctr1Session).Should(Exit(0)) + + kube := podmanTest.Podman([]string{"generate", "kube", podName}) + kube.WaitWithDefaultTimeout() + Expect(kube).Should(Exit(0)) + + pod := new(v1.Pod) + err := yaml.Unmarshal(kube.Out.Contents(), pod) + Expect(err).ToNot(HaveOccurred()) + + Expect(string(pod.Spec.RestartPolicy)).To(Equal(v[2])) + } + }) + + It("podman generate kube on ctr with restartPolicy", func() { + // podName, set, expect + testSli := [][]string{ + {"", ""}, // some ctr created from cmdline, set it to "" and let k8s default it to Always + {"always", "Always"}, + {"on-failure", "OnFailure"}, + {"no", "Never"}, + {"never", "Never"}, + } + + for k, v := range testSli { + ctrName := "ctr" + strconv.Itoa(k) + ctrSession := podmanTest.Podman([]string{"create", "--restart", v[0], "--name", ctrName, ALPINE, "top"}) + ctrSession.WaitWithDefaultTimeout() + Expect(ctrSession).Should(Exit(0)) + + kube := podmanTest.Podman([]string{"generate", "kube", ctrName}) + kube.WaitWithDefaultTimeout() + Expect(kube).Should(Exit(0)) + + pod := new(v1.Pod) + err := yaml.Unmarshal(kube.Out.Contents(), pod) + Expect(err).ToNot(HaveOccurred()) + + Expect(string(pod.Spec.RestartPolicy)).To(Equal(v[1])) + } + }) + It("podman generate kube on pod with memory limit", func() { SkipIfRootlessCgroupsV1("Not supported for rootless + CgroupsV1") podName := "testMemoryLimit" @@ -1465,13 +1529,12 @@ USER test1` }) It("podman generate kube on pod with --type=deployment and --restart=no should fail", func() { - // TODO: When we add --restart for pods, fix this test to reflect that podName := "test-pod" - session := podmanTest.Podman([]string{"pod", "create", podName}) + session := podmanTest.Podman([]string{"pod", "create", "--restart", "no", podName}) session.WaitWithDefaultTimeout() Expect(session).Should(Exit(0)) - session = podmanTest.Podman([]string{"create", "--pod", podName, "--restart", "no", ALPINE, "top"}) + session = podmanTest.Podman([]string{"create", "--pod", podName, ALPINE, "top"}) session.WaitWithDefaultTimeout() Expect(session).Should(Exit(0)) diff --git a/test/e2e/play_kube_test.go b/test/e2e/play_kube_test.go index dc2e07555f..b3d5ffe18f 100644 --- a/test/e2e/play_kube_test.go +++ b/test/e2e/play_kube_test.go @@ -2382,7 +2382,7 @@ var _ = Describe("Podman play kube", func() { kube.WaitWithDefaultTimeout() Expect(kube).Should(Exit(0)) - inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(pod), "--format", "{{.HostConfig.RestartPolicy.Name}}"}) + inspect := podmanTest.Podman([]string{"inspect", pod.Name, "--format", "{{.RestartPolicy}}"}) inspect.WaitWithDefaultTimeout() Expect(inspect).Should(Exit(0)) Expect(inspect.OutputToString()).To(Equal(v[2]))