More flexible options when combining '--rm' and '--restart'

[srcshelton]

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind bug

Description

With podman-2.1.1 with patch #7895 , running a container with --rm and --restart on-failure results in the output:

Error: the --rm option conflicts with --restart, when the restartPolicy is not "" and "no"

So a couple of things:

• Should there be a value within the first set if quotes? Should the final 'and' read 'or'?

• Surely if should be possible to combine --rm and --restart on-failure (specifically) - the idea being that the container would be removed when externally killed by via podman stop (or equivalent), but would be restarted if the container's PID 1 aborts for any other internally-triggered reason (and/or results in a non-zero exit code)?

Output of podman version:

Version:      2.1.1
API Version:  2.0.0
Go Version:   go1.14.7
Git Commit:   9f6d6ba0b314d86521b66183c9ce48eaa2da1de2
Built:        Sat Oct  3 00:59:40 2020
OS/Arch:      linux/amd64

Output of podman info --debug:

host:
  arch: amd64
  buildahVersion: 1.16.1
  cgroupManager: cgroupfs
  cgroupVersion: v2
  conmon:
    package: Unknown
    path: /usr/bin/conmon
    version: 'conmon version 2.0.21, commit: 35a2fa83022e56e18af7e6a865ba5d7165fa2a4a'
  cpus: 8
  distribution:
    distribution: gentoo
    version: unknown
  eventLogger: file
  hostname: dellr330
  idMappings:
    gidmap: null
    uidmap: null
  kernel: 5.8.9-gentoo
  linkmode: dynamic
  memFree: 2429820928
  memTotal: 8063447040
  ociRuntime:
    name: crun
    package: Unknown
    path: /usr/bin/crun
    version: |-
      crun version 0.15-dirty
      commit: 56ca95e61639510c7dbd39ff512f80f626404969
      spec: 1.0.0
      +SELINUX +APPARMOR +CAP +SECCOMP +EBPF +YAJL
  os: linux
  remoteSocket:
    path: /run/podman/podman.sock
  rootless: false
  slirp4netns:
    executable: ""
    package: ""
    version: ""
  swapFree: 25387175936
  swapTotal: 25769787392
  uptime: 96h 19m 31.76s (Approximately 4.00 days)
registries:
  search:
  - docker.io
  - docker.pkg.github.com
  - quay.io
store:
  configFile: /etc/containers/storage.conf
  containerStore:
    number: 16
    paused: 0
    running: 15
    stopped: 1
  graphDriverName: overlay
  graphOptions:
    overlay.ignore_chown_errors: "false"
  graphRoot: /space/podman/storage
  graphStatus:
    Backing Filesystem: extfs
    Native Overlay Diff: "true"
    Supports d_type: "true"
    Using metacopy: "false"
  imageStore:
    number: 173
  runRoot: /space/podman/run
  volumePath: /space/podman/volumes
version:
  APIVersion: 2.0.0
  Built: 1601683180
  BuiltTime: Sat Oct  3 00:59:40 2020
  GitCommit: 9f6d6ba0b314d86521b66183c9ce48eaa2da1de2
  GoVersion: go1.14.7
  OsArch: linux/amd64
  Version: 2.1.1

Have you tested with the latest version of Podman and have you checked the Podman Troubleshooting Guide?

Yes

[zhangguanzhang]

about the value, you cloud see https://github.com/containers/podman/blob/master/test/e2e/run_test.go#L70-L91
Logically speaking, these two options should not be together in some values

$ docker run --restart on-failure --rm nginx:alpine 
docker: Conflicting options: --restart and --rm.
See 'docker run --help'.
$ docker run -d --restart no --rm nginx:alpine 
745bcf4a007c93dd8f49d7cc33a8ba344fc834334a98de331187cf30d45d9c7a

[srcshelton]

Sorry, I'm still not understanding:

restartPolicy is not "" and "no"

... is no still referring to the restartPolicy value, or something else? If so, how can restartPolicy be both an empty string and no?

Ah, but I think I see what you mean - I was assuming that if --restart isn't specified then it would default to value no, but it looks as if no and <not specified> are treated as two separate cases?

In which case, it very much seems to be an 'or' situation... how about:

		// the --rm option conflicts when the restartPolicy (from --restart) is
		// neither unspecified nor equal to "no", so the exitCode should not be 0

... or similar?

---

Why, though, can we not have --rm and a restartPolicy of on-failure? The two appear as if they should be unrelated:

• --restart on-failure says to catch the container failing (rather than being stopped) and restart it before any finalisation steps commence;
• --rm says to finally clean-up containers and volumes of stopped/crashed containers after all other steps are complete.

... so ideally, --restart on-failure should restart a failed container before --rm has chance to kick-in.

The documentation already says:

Note that the container will not be removed when it could not be created or started successfully. This allows the user to inspect the container after failure.

... so could the behaviour not be amended so that another exception is that it doesn't remove a container with a restartPolicy of 'on-failure' unless it has cleanly stopped without failing?

Additionally, the documentation doesn't seem to mention that rm and most restart options are incompatible!

It does say, though:

Restart policy will not take effect if a container is stopped via the podman kill or podman stop commands.

... which still leads me to feel that it should be possible to use rm and restart simultaneously?

[zhangguanzhang]

--restart could be:

• ""
• "no"
• "always"
• "on-failure"
• "unless-stopped"

could see this condition

	if c.Rm && c.Restart != "" && c.Restart != "no" {
		return errors.Errorf(`the --rm option conflicts with --restart, when the restartPolicy is not "" and "no"`)
	}

c.Rm && c.Restart != "" && c.Restart != "no"

[srcshelton]

To clarify, this issue is open to suggest that '--rm' and '--restart on-failure' should not be mutually exclusive:

• --restart on-failure (and indeed unless-stopped) should catch a container exiting due to PID 1 terminating with a non-zero return-code (or in any case unless specifically requested with unless-stopped), and restart the container if this happens;
• --rm should be triggered afterwards, if a container has exited (for any reason) is not to be restarted, then the container and linked resources are removed.

The latter shouldn't block the former.

[github-actions]

A friendly reminder that this issue had no activity for 30 days.