generate systemd: handle --restart

Handle custom restart policies of containers when generating the unit files; those should be set on the unit level and removed from ExecStart flags. Fixes: containers#11438 Signed-off-by: Valentin Rothberg <[email protected]>
vrothberg · Sep 7, 2021 · d1573b9 · d1573b9
1 parent e095667
commit d1573b9
Show file tree

Hide file tree

Showing 12 changed files with 166 additions and 113 deletions.
diff --git a/cmd/podman/generate/systemd.go b/cmd/podman/generate/systemd.go
@@ -12,15 +12,22 @@ import (
 	"github.com/containers/podman/v3/cmd/podman/registry"
 	"github.com/containers/podman/v3/cmd/podman/utils"
 	"github.com/containers/podman/v3/pkg/domain/entities"
+	systemDefine "github.com/containers/podman/v3/pkg/systemd/define"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
 
+const (
+	restartPolicyFlagName = "restart-policy"
+	timeFlagName          = "time"
+)
+
 var (
 	files              bool
 	format             string
 	systemdTimeout     uint
+	systemdRestart     string
 	systemdOptions     = entities.GenerateSystemdOptions{}
 	systemdDescription = `Generate systemd units for a pod or container.
   The generated units can later be controlled via systemctl(1).`
@@ -47,7 +54,6 @@ func init() {
 	flags.BoolVarP(&systemdOptions.Name, "name", "n", false, "Use container/pod names instead of IDs")
 	flags.BoolVarP(&files, "files", "f", false, "Generate .service files instead of printing to stdout")
 
-	timeFlagName := "time"
 	flags.UintVarP(&systemdTimeout, timeFlagName, "t", containerConfig.Engine.StopTimeout, "Stop timeout override")
 	_ = systemdCmd.RegisterFlagCompletionFunc(timeFlagName, completion.AutocompleteNone)
 	flags.BoolVarP(&systemdOptions.New, "new", "", false, "Create a new container or pod instead of starting an existing one")
@@ -65,8 +71,7 @@ func init() {
 	flags.StringVar(&systemdOptions.Separator, separatorFlagName, "-", "Systemd unit name separator between name/id and prefix")
 	_ = systemdCmd.RegisterFlagCompletionFunc(separatorFlagName, completion.AutocompleteNone)
 
-	restartPolicyFlagName := "restart-policy"
-	flags.StringVar(&systemdOptions.RestartPolicy, restartPolicyFlagName, "on-failure", "Systemd restart-policy")
+	flags.StringVar(&systemdRestart, restartPolicyFlagName, systemDefine.DefaultRestartPolicy, "Systemd restart-policy")
 	_ = systemdCmd.RegisterFlagCompletionFunc(restartPolicyFlagName, common.AutocompleteSystemdRestartOptions)
 
 	formatFlagName := "format"
@@ -77,9 +82,12 @@ func init() {
 }
 
 func systemd(cmd *cobra.Command, args []string) error {
-	if cmd.Flags().Changed("time") {
+	if cmd.Flags().Changed(timeFlagName) {
 		systemdOptions.StopTimeout = &systemdTimeout
 	}
+	if cmd.Flags().Changed(restartPolicyFlagName) {
+		systemdOptions.RestartPolicy = &systemdRestart
+	}
 
 	if registry.IsRemote() {
 		logrus.Warnln("The generated units should be placed on your remote system")

diff --git a/pkg/api/handlers/libpod/generate.go b/pkg/api/handlers/libpod/generate.go
@@ -16,16 +16,15 @@ func GenerateSystemd(w http.ResponseWriter, r *http.Request) {
 	runtime := r.Context().Value("runtime").(*libpod.Runtime)
 	decoder := r.Context().Value("decoder").(*schema.Decoder)
 	query := struct {
-		Name            bool   `schema:"useName"`
-		New             bool   `schema:"new"`
-		NoHeader        bool   `schema:"noHeader"`
-		RestartPolicy   string `schema:"restartPolicy"`
-		StopTimeout     uint   `schema:"stopTimeout"`
-		ContainerPrefix string `schema:"containerPrefix"`
-		PodPrefix       string `schema:"podPrefix"`
-		Separator       string `schema:"separator"`
+		Name            bool    `schema:"useName"`
+		New             bool    `schema:"new"`
+		NoHeader        bool    `schema:"noHeader"`
+		RestartPolicy   *string `schema:"restartPolicy"`
+		StopTimeout     uint    `schema:"stopTimeout"`
+		ContainerPrefix string  `schema:"containerPrefix"`
+		PodPrefix       string  `schema:"podPrefix"`
+		Separator       string  `schema:"separator"`
 	}{
-		RestartPolicy:   "on-failure",
 		StopTimeout:     util.DefaultContainerConfig().Engine.StopTimeout,
 		ContainerPrefix: "container",
 		PodPrefix:       "pod",
@@ -49,6 +48,7 @@ func GenerateSystemd(w http.ResponseWriter, r *http.Request) {
 		PodPrefix:       query.PodPrefix,
 		Separator:       query.Separator,
 	}
+
 	report, err := containerEngine.GenerateSystemd(r.Context(), utils.GetName(r), options)
 	if err != nil {
 		utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrap(err, "error generating systemd units"))

diff --git a/pkg/domain/entities/generate.go b/pkg/domain/entities/generate.go
@@ -9,7 +9,7 @@ type GenerateSystemdOptions struct {
 	// New - create a new container instead of starting a new one.
 	New bool
 	// RestartPolicy - systemd restart policy.
-	RestartPolicy string
+	RestartPolicy *string
 	// StopTimeout - time when stopping the container.
 	StopTimeout *uint
 	// ContainerPrefix - systemd unit name prefix for containers

diff --git a/pkg/domain/infra/tunnel/generate.go b/pkg/domain/infra/tunnel/generate.go
@@ -9,7 +9,10 @@ import (
 
 func (ic *ContainerEngine) GenerateSystemd(ctx context.Context, nameOrID string, opts entities.GenerateSystemdOptions) (*entities.GenerateSystemdReport, error) {
 	options := new(generate.SystemdOptions).WithUseName(opts.Name).WithContainerPrefix(opts.ContainerPrefix).WithNew(opts.New).WithNoHeader(opts.NoHeader)
-	options.WithPodPrefix(opts.PodPrefix).WithRestartPolicy(opts.RestartPolicy).WithSeparator(opts.Separator)
+	options.WithPodPrefix(opts.PodPrefix).WithSeparator(opts.Separator)
+	if opts.RestartPolicy != nil {
+		options.WithRestartPolicy(*opts.RestartPolicy)
+	}
 	if to := opts.StopTimeout; to != nil {
 		options.WithStopTimeout(*opts.StopTimeout)
 	}

diff --git a/pkg/systemd/define/const.go b/pkg/systemd/define/const.go
@@ -1,8 +1,13 @@
 package define
 
-// EnvVariable "PODMAN_SYSTEMD_UNIT" is set in all generated systemd units and
-// is set to the unit's (unique) name.
-const EnvVariable = "PODMAN_SYSTEMD_UNIT"
+const (
+	// Default restart policy for generated unit files.
+	DefaultRestartPolicy = "on-failure"
+
+	// EnvVariable "PODMAN_SYSTEMD_UNIT" is set in all generated systemd units and
+	// is set to the unit's (unique) name.
+	EnvVariable = "PODMAN_SYSTEMD_UNIT"
+)
 
 // RestartPolicies includes all valid restart policies to be used in a unit
 // file.

diff --git a/pkg/systemd/generate/common.go b/pkg/systemd/generate/common.go
@@ -71,12 +71,13 @@ func filterCommonContainerFlags(command []string, argCount int) []string {
 		case s == "--rm":
 			// Boolean flags support --flag and --flag={true,false}.
 			continue
-		case s == "--sdnotify", s == "--cgroups", s == "--cidfile":
+		case s == "--sdnotify", s == "--cgroups", s == "--cidfile", s == "--restart":
 			i++
 			continue
 		case strings.HasPrefix(s, "--rm="),
 			strings.HasPrefix(s, "--cgroups="),
-			strings.HasPrefix(s, "--cidfile="):
+			strings.HasPrefix(s, "--cidfile="),
+			strings.HasPrefix(s, "--restart="):
 			continue
 		}
 		processed = append(processed, s)

diff --git a/pkg/systemd/generate/common_test.go b/pkg/systemd/generate/common_test.go
@@ -117,12 +117,12 @@ func TestFilterCommonContainerFlags(t *testing.T) {
 			1,
 		},
 		{
-			[]string{"podman", "run", "--cgroups=foo", "alpine"},
+			[]string{"podman", "run", "--cgroups=foo", "--restart=foo", "alpine"},
 			[]string{"podman", "run", "alpine"},
 			1,
 		},
 		{
-			[]string{"podman", "run", "--cgroups=foo", "--rm", "alpine"},
+			[]string{"podman", "run", "--cgroups=foo", "--rm", "--restart", "foo", "alpine"},
 			[]string{"podman", "run", "alpine"},
 			1,
 		},

diff --git a/pkg/systemd/generate/containers.go b/pkg/systemd/generate/containers.go
@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/containers/podman/v3/libpod"
+	libpodDefine "github.com/containers/podman/v3/libpod/define"
 	"github.com/containers/podman/v3/pkg/domain/entities"
 	"github.com/containers/podman/v3/pkg/systemd/define"
 	"github.com/containers/podman/v3/version"
@@ -34,6 +35,8 @@ type containerInfo struct {
 	StopTimeout uint
 	// RestartPolicy of the systemd unit (e.g., no, on-failure, always).
 	RestartPolicy string
+	// Custom number of restart attempts.
+	StartLimitBurst string
 	// PIDFile of the service. Required for forking services. Must point to the
 	// PID of the associated conmon process.
 	PIDFile string
@@ -101,6 +104,9 @@ Environment={{{{.EnvVariable}}}}=%n
 Environment={{{{- range $index, $value := .ExtraEnvs -}}}}{{{{if $index}}}} {{{{end}}}}{{{{ $value }}}}{{{{end}}}}
 {{{{- end}}}}
 Restart={{{{.RestartPolicy}}}}
+{{{{- if .StartLimitBurst}}}}
+StartLimitBurst={{{{.StartLimitBurst}}}}
+{{{{- end}}}}
 TimeoutStopSec={{{{.TimeoutStopSec}}}}
 {{{{- if .ExecStartPre}}}}
 ExecStartPre={{{{.ExecStartPre}}}}
@@ -175,7 +181,7 @@ func generateContainerInfo(ctr *libpod.Container, options entities.GenerateSyste
 	info := containerInfo{
 		ServiceName:       serviceName,
 		ContainerNameOrID: nameOrID,
-		RestartPolicy:     options.RestartPolicy,
+		RestartPolicy:     define.DefaultRestartPolicy,
 		PIDFile:           conmonPidFile,
 		StopTimeout:       timeout,
 		GenerateTimestamp: true,
@@ -202,8 +208,11 @@ func containerServiceName(ctr *libpod.Container, options entities.GenerateSystem
 // containerInfo.  Note that the containerInfo is also post processed and
 // completed, which allows for an easier unit testing.
 func executeContainerTemplate(info *containerInfo, options entities.GenerateSystemdOptions) (string, error) {
-	if err := validateRestartPolicy(info.RestartPolicy); err != nil {
-		return "", err
+	if options.RestartPolicy != nil {
+		if err := validateRestartPolicy(*options.RestartPolicy); err != nil {
+			return "", err
+		}
+		info.RestartPolicy = *options.RestartPolicy
 	}
 
 	// Make sure the executable is set.
@@ -275,6 +284,7 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
 		fs.Bool("replace", false, "")
 		fs.StringArrayP("env", "e", nil, "")
 		fs.String("sdnotify", "", "")
+		fs.String("restart", "", "")
 		fs.Parse(remainingCmd)
 
 		remainingCmd = filterCommonContainerFlags(remainingCmd, fs.NArg())
@@ -339,6 +349,27 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
 			}
 		}
 
+		// Unless the user explicitly set a restart policy, check
+		// whether the container was created with a custom one and use
+		// it instead of the default.
+		if options.RestartPolicy == nil {
+			restartPolicy, err := fs.GetString("restart")
+			if err != nil {
+				return "", err
+			}
+			if restartPolicy != "" {
+				if strings.HasPrefix(restartPolicy, "on-failure:") {
+					// Special case --restart=on-failure:5
+					spl := strings.Split(restartPolicy, ":")
+					restartPolicy = spl[0]
+					info.StartLimitBurst = spl[1]
+				} else if restartPolicy == libpodDefine.RestartPolicyUnlessStopped {
+					restartPolicy = libpodDefine.RestartPolicyAlways
+				}
+				info.RestartPolicy = restartPolicy
+			}
+		}
+
 		envs, err := fs.GetStringArray("env")
 		if err != nil {
 			return "", err