From 221cfc68721f847ee9e41ff2634e7ed32c5b4ffe Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 24 Oct 2022 13:57:37 +0200 Subject: [PATCH] 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 --- cmd/podman/containers/create.go | 2 +- cmd/podman/pods/create.go | 16 +---------- pkg/domain/infra/abi/containers.go | 2 +- pkg/domain/infra/tunnel/containers.go | 2 +- pkg/systemd/generate/containers.go | 6 +--- pkg/systemd/generate/containers_test.go | 38 ------------------------- pkg/systemd/generate/pods.go | 16 ++++------- pkg/systemd/generate/pods_test.go | 10 ------- pkg/util/utils.go | 15 ++++------ test/e2e/generate_systemd_test.go | 2 -- test/system/030-run.bats | 10 +++---- 11 files changed, 20 insertions(+), 99 deletions(-) diff --git a/cmd/podman/containers/create.go b/cmd/podman/containers/create.go index f9e37178e9..2bd6733d8e 100644 --- a/cmd/podman/containers/create.go +++ b/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 } } diff --git a/cmd/podman/pods/create.go b/cmd/podman/pods/create.go index fc2e078948..19daa3f862 100644 --- a/cmd/podman/pods/create.go +++ b/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) } } diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 4845749511..493b07c3f7 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/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 diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index 0b573686fb..a49048ce6c 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/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 diff --git a/pkg/systemd/generate/containers.go b/pkg/systemd/generate/containers.go index ae2dac80c4..deb8ade924 100644 --- a/pkg/systemd/generate/containers.go +++ b/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: diff --git a/pkg/systemd/generate/containers_test.go b/pkg/systemd/generate/containers_test.go index 28d9b22b10..e4e12d8129 100644 --- a/pkg/systemd/generate/containers_test.go +++ b/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 \ diff --git a/pkg/systemd/generate/pods.go b/pkg/systemd/generate/pods.go index 588bfb430e..bdc7c34d8f 100644 --- a/pkg/systemd/generate/pods.go +++ b/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. - ExecStartPre1 string - // ExecStartPre2 of the unit. - ExecStartPre2 string + // ExecStartPre of the unit. + ExecStartPre string // ExecStart of the unit. ExecStart string // TimeoutStopSec of the unit. @@ -115,11 +113,8 @@ Restart={{{{.RestartPolicy}}}} RestartSec={{{{.RestartSec}}}} {{{{- end}}}} TimeoutStopSec={{{{.TimeoutStopSec}}}} -{{{{- if .ExecStartPre1}}}} -ExecStartPre={{{{.ExecStartPre1}}}} -{{{{- end}}}} -{{{{- if .ExecStartPre2}}}} -ExecStartPre={{{{.ExecStartPre2}}}} +{{{{- if .ExecStartPre}}}} +ExecStartPre={{{{.ExecStartPre}}}} {{{{- 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.ExecStartPre2 = formatOptions(startCommand) + info.ExecStartPre = 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}}}}") diff --git a/pkg/systemd/generate/pods_test.go b/pkg/systemd/generate/pods_test.go index c44ab111e1..3cde6eacb0 100644 --- a/pkg/systemd/generate/pods_test.go +++ b/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 \ diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 87e403986d..c62b9a0189 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -682,18 +682,15 @@ func DefaultContainerConfig() *config.Config { return containerConfig } -func CreateCidFile(cidfile string, id string) error { - cidFile, err := OpenExclusiveFile(cidfile) +func CreateIDFile(path string, id string) error { + idFile, err := os.Create(path) if err != nil { - if os.IsExist(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) + return fmt.Errorf("creating idfile: %w", err) } - if _, err = cidFile.WriteString(id); err != nil { - logrus.Error(err) + defer idFile.Close() + if _, err = idFile.WriteString(id); err != nil { + return fmt.Errorf("writing idfile: %w", err) } - cidFile.Close() return nil } diff --git a/test/e2e/generate_systemd_test.go b/test/e2e/generate_systemd_test.go index 01c0aefab0..bd3d01f90d 100644 --- a/test/e2e/generate_systemd_test.go +++ b/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")) diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 8de1625b5e..3440508cd2 100644 --- a/test/system/030-run.bats +++ b/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" {