Merge pull request #16273 from vrothberg/cidfile

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

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" {