container/pod id file: truncate instead of throwing an error

Truncate the container and pod ID files instead of throwing an error.
The main motivation is to prevent redundant work when starting systemd
units.  Throwing an error when the file already exists is not preventing
races or file corruptions, so let's leave that to the user which in
almost all cases are generated (and tested) systemd units.

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

cmd/podman/containers/create.go

@@ -169,7 +169,7 @@ func create(cmd *cobra.Command, args []string) error {
 	}
 
 	if cliVals.CIDFile != "" {
-		if err := util.CreateCidFile(cliVals.CIDFile, report.Id); err != nil {
+		if err := util.CreateIDFile(cliVals.CIDFile, report.Id); err != nil {
 			return err
 		}
 	}

cmd/podman/pods/create.go

@@ -19,7 +19,6 @@ import (
 	"github.com/containers/podman/v4/cmd/podman/utils"
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/containers/podman/v4/pkg/domain/entities"
-	"github.com/containers/podman/v4/pkg/errorhandling"
 	"github.com/containers/podman/v4/pkg/specgen"
 	"github.com/containers/podman/v4/pkg/specgenutil"
 	"github.com/containers/podman/v4/pkg/util"
@@ -104,7 +103,6 @@ func init() {
 func create(cmd *cobra.Command, args []string) error {
 	var (
 		err          error
-		podIDFD      *os.File
 		imageName    string
 		rawImageName string
 		podName      string
@@ -188,18 +186,6 @@ func create(cmd *cobra.Command, args []string) error {
 		createOptions.Name = podName
 	}
 
-	if cmd.Flag("pod-id-file").Changed {
-		podIDFD, err = util.OpenExclusiveFile(podIDFile)
-		if err != nil && os.IsExist(err) {
-			return fmt.Errorf("pod id file exists. Ensure another pod is not using it or delete %s", podIDFile)
-		}
-		if err != nil {
-			return fmt.Errorf("opening pod-id-file %s", podIDFile)
-		}
-		defer errorhandling.CloseQuiet(podIDFD)
-		defer errorhandling.SyncQuiet(podIDFD)
-	}
-
 	if len(createOptions.Net.PublishPorts) > 0 {
 		if !createOptions.Infra {
 			return fmt.Errorf("you must have an infra container to publish port bindings to the host")
@@ -299,7 +285,7 @@ func create(cmd *cobra.Command, args []string) error {
 	}
 
 	if len(podIDFile) > 0 {
-		if err = os.WriteFile(podIDFile, []byte(response.Id), 0644); err != nil {
+		if err := util.CreateIDFile(podIDFile, response.Id); err != nil {
 			return fmt.Errorf("failed to write pod ID to file: %w", err)
 		}
 	}

pkg/domain/infra/abi/containers.go

@@ -1120,7 +1120,7 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 	}
 
 	if opts.CIDFile != "" {
-		if err := util.CreateCidFile(opts.CIDFile, ctr.ID()); err != nil {
+		if err := util.CreateIDFile(opts.CIDFile, ctr.ID()); err != nil {
 			// If you fail to create CIDFile then remove the container
 			_ = removeContainer(ctr, true)
 			return nil, err

pkg/domain/infra/tunnel/containers.go

@@ -804,7 +804,7 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 	}
 
 	if opts.CIDFile != "" {
-		if err := util.CreateCidFile(opts.CIDFile, con.ID); err != nil {
+		if err := util.CreateIDFile(opts.CIDFile, con.ID); err != nil {
 			// If you fail to create CIDFile then remove the container
 			_ = removeContainer(con.ID, true)
 			return nil, err

pkg/systemd/generate/containers.go

@@ -42,10 +42,10 @@ type containerInfo struct {
 	ExtraEnvs              []string
 	EnvVariable            string
 	AdditionalEnvVariables []string
-	ExecStartPre           string
 	ExecStart              string
 	TimeoutStartSec        uint
 	TimeoutStopSec         uint
+	ExecStartPre           string
 	ExecStop               string
 	ExecStopPost           string
 	GenerateNoHeader       bool
@@ -94,9 +94,6 @@ StartLimitBurst={{{{.StartLimitBurst}}}}
 TimeoutStartSec={{{{.TimeoutStartSec}}}}
 {{{{- end}}}}
 TimeoutStopSec={{{{.TimeoutStopSec}}}}
-{{{{- if .ExecStartPre}}}}
-ExecStartPre={{{{.ExecStartPre}}}}
-{{{{- end}}}}
 ExecStart={{{{.ExecStart}}}}
 {{{{- if .ExecStop}}}}
 ExecStop={{{{.ExecStop}}}}
@@ -316,7 +313,6 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
 		info.NotifyAccess = "all"
 		info.PIDFile = ""
 		info.ContainerIDFile = "%t/%n.ctr-id"
-		info.ExecStartPre = formatOptionsString("/bin/rm -f {{{{.ContainerIDFile}}}}")
 		info.ExecStop = formatOptionsString("{{{{.Executable}}}} stop --ignore {{{{if (ge .StopTimeout 0)}}}}-t {{{{.StopTimeout}}}}{{{{end}}}} --cidfile={{{{.ContainerIDFile}}}}")
 		info.ExecStopPost = formatOptionsString("{{{{.Executable}}}} rm -f --ignore {{{{if (ge .StopTimeout 0)}}}}-t {{{{.StopTimeout}}}}{{{{end}}}} --cidfile={{{{.ContainerIDFile}}}}")
 		// The create command must at least have three arguments:

pkg/systemd/generate/containers_test.go

@@ -282,8 +282,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman container run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -321,8 +319,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman container run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -360,8 +356,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman container run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -399,8 +393,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -438,8 +430,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -478,8 +468,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -517,8 +505,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -554,8 +540,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=102
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -594,8 +578,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=102
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -634,8 +616,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=102
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman \
 	--events-backend none \
 	--runroot /root run \
@@ -672,8 +652,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman container run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -708,8 +686,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -748,8 +724,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -787,8 +761,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -827,8 +799,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -870,8 +840,6 @@ Environment=PODMAN_SYSTEMD_UNIT=%n
 Environment=FOO=abc "BAR=my test" USER=%%a
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -940,8 +908,6 @@ Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 StartLimitBurst=42
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -976,8 +942,6 @@ RequiresMountsFor=/var/run/containers/storage
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
@@ -1014,8 +978,6 @@ Environment=PODMAN_SYSTEMD_UNIT=%n-%i
 Restart=on-failure
 StartLimitBurst=42
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/%n.ctr-id
 ExecStart=/usr/bin/podman run \
 	--name=container-foo-%i \
 	--cidfile=%t/%n.ctr-id \

pkg/systemd/generate/pods.go

@@ -61,10 +61,8 @@ type podInfo struct {
 	PodCreateCommand string
 	// EnvVariable is generate.EnvVariable and must not be set.
 	EnvVariable string
-	// ExecStartPre1 of the unit.
+	// ExecStartPre of the unit.
-	ExecStartPre1 string
+	ExecStartPre string
-	// ExecStartPre2 of the unit.
-	ExecStartPre2 string
 	// ExecStart of the unit.
 	ExecStart string
 	// TimeoutStopSec of the unit.
@@ -115,11 +113,8 @@ Restart={{{{.RestartPolicy}}}}
 RestartSec={{{{.RestartSec}}}}
 {{{{- end}}}}
 TimeoutStopSec={{{{.TimeoutStopSec}}}}
-{{{{- if .ExecStartPre1}}}}
+{{{{- if .ExecStartPre}}}}
-ExecStartPre={{{{.ExecStartPre1}}}}
+ExecStartPre={{{{.ExecStartPre}}}}
-{{{{- end}}}}
-{{{{- if .ExecStartPre2}}}}
-ExecStartPre={{{{.ExecStartPre2}}}}
 {{{{- end}}}}
 ExecStart={{{{.ExecStart}}}}
 ExecStop={{{{.ExecStop}}}}
@@ -371,8 +366,7 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions)
 		startCommand = append(startCommand, podCreateArgs...)
 		startCommand = escapeSystemdArguments(startCommand)
 
-		info.ExecStartPre1 = formatOptionsString("/bin/rm -f {{{{.PIDFile}}}} {{{{.PodIDFile}}}}")
+		info.ExecStartPre = formatOptions(startCommand)
-		info.ExecStartPre2 = formatOptions(startCommand)
 		info.ExecStart = formatOptionsString("{{{{.Executable}}}} {{{{if .RootFlags}}}}{{{{ .RootFlags}}}} {{{{end}}}}pod start --pod-id-file {{{{.PodIDFile}}}}")
 		info.ExecStop = formatOptionsString("{{{{.Executable}}}} {{{{if .RootFlags}}}}{{{{ .RootFlags}}}} {{{{end}}}}pod stop --ignore --pod-id-file {{{{.PodIDFile}}}} {{{{if (ge .StopTimeout 0)}}}} -t {{{{.StopTimeout}}}}{{{{end}}}}")
 		info.ExecStopPost = formatOptionsString("{{{{.Executable}}}} {{{{if .RootFlags}}}}{{{{ .RootFlags}}}} {{{{end}}}}pod rm --ignore -f --pod-id-file {{{{.PodIDFile}}}}")

pkg/systemd/generate/pods_test.go

@@ -258,8 +258,6 @@ Before=
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/pod-123abc.pid %t/pod-123abc.pod-id
 ExecStartPre=/usr/bin/podman pod create \
 	--infra-conmon-pidfile %t/pod-123abc.pid \
 	--pod-id-file %t/pod-123abc.pod-id \
@@ -326,8 +324,6 @@ Before=container-1.service container-2.service
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/pod-123abc.pid %t/pod-123abc.pod-id
 ExecStartPre=/usr/bin/podman pod create \
 	--infra-conmon-pidfile %t/pod-123abc.pid \
 	--pod-id-file %t/pod-123abc.pod-id \
@@ -367,8 +363,6 @@ Before=container-1.service container-2.service
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/pod-123abc.pid %t/pod-123abc.pod-id
 ExecStartPre=/usr/bin/podman \
 	--events-backend none \
 	--runroot /root pod create \
@@ -410,8 +404,6 @@ Before=container-1.service container-2.service
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/pod-123abc.pid %t/pod-123abc.pod-id
 ExecStartPre=/usr/bin/podman pod create \
 	--infra-conmon-pidfile %t/pod-123abc.pid \
 	--pod-id-file %t/pod-123abc.pod-id \
@@ -451,8 +443,6 @@ Before=container-1.service container-2.service
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=on-failure
 TimeoutStopSec=70
-ExecStartPre=/bin/rm \
-	-f %t/pod-123abc.pid %t/pod-123abc.pod-id
 ExecStartPre=/usr/bin/podman pod create \
 	--infra-conmon-pidfile %t/pod-123abc.pid \
 	--pod-id-file %t/pod-123abc.pod-id \

pkg/util/utils.go

@@ -682,18 +682,15 @@ func DefaultContainerConfig() *config.Config {
 	return containerConfig
 }
 
-func CreateCidFile(cidfile string, id string) error {
+func CreateIDFile(path string, id string) error {
-	cidFile, err := OpenExclusiveFile(cidfile)
+	idFile, err := os.Create(path)
 	if err != nil {
-		if os.IsExist(err) {
+		return fmt.Errorf("creating idfile: %w", err)
-			return fmt.Errorf("container id file exists. Ensure another container is not using it or delete %s", cidfile)
 	}
-		return fmt.Errorf("opening cidfile %s", cidfile)
+	defer idFile.Close()
+	if _, err = idFile.WriteString(id); err != nil {
+		return fmt.Errorf("writing idfile: %w", err)
 	}
-	if _, err = cidFile.WriteString(id); err != nil {
-		logrus.Error(err)
-	}
-	cidFile.Close()
 	return nil
 }
 

test/e2e/generate_systemd_test.go

@@ -570,8 +570,6 @@ var _ = Describe("Podman generate systemd", func() {
 		Expect(session.OutputToString()).To(ContainSubstring("--pod-id-file %t/pod-foo.pod-id"))
 		Expect(session.OutputToString()).To(ContainSubstring("--exit-policy=stop"))
 		Expect(session.OutputToString()).To(ContainSubstring("--name foo"))
-		Expect(session.OutputToString()).To(ContainSubstring("ExecStartPre=/bin/rm"))
-		Expect(session.OutputToString()).To(ContainSubstring("-f %t/pod-foo.pid %t/pod-foo.pod-id"))
 		Expect(session.OutputToString()).To(ContainSubstring("pod stop"))
 		Expect(session.OutputToString()).To(ContainSubstring("--ignore"))
 		Expect(session.OutputToString()).To(ContainSubstring("--pod-id-file %t/pod-foo.pod-id"))

test/system/030-run.bats

@@ -192,6 +192,10 @@ echo $rand        |   0 | $rand
     pidfile=${PODMAN_TMPDIR}/pidfile
     cidfile=${PODMAN_TMPDIR}/cidfile
 
+    # Write random content to the cidfile to make sure its content is truncated
+    # on write.
+    echo "$(random_string 120)" > $cidfile
+
     cname=$(random_string)
     run_podman run --name $cname \
                --conmon-pidfile=$pidfile \
@@ -218,12 +222,6 @@ echo $rand        |   0 | $rand
 
     # All OK. Kill container.
     run_podman rm -f $cid
-
-    # Podman must not overwrite existing cid file.
-    # (overwriting conmon-pidfile is OK, so don't test that)
-    run_podman 125 run --cidfile=$cidfile $IMAGE true
-    is "$output" "Error: container id file exists. .* delete $cidfile" \
-       "podman will not overwrite existing cidfile"
 }
 
 @test "podman run docker-archive" {