Add restart policy for containers

[mheon]

Cooked this up after lunch. Implements most of Docker's --restart flag. We can't implement unless-stopped (we don't have a daemon and as such never experience a daemon restart), and I haven't coded up restart counts yet (another half hour or so of work), but always and on-error with infinite restarts work as advertised.

Needs tests, bash completions, and for me to stop being lazy and code up restart count.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mheon]

Added support for retry count with the on-failure policy. All that's left is manpages and tests

[mheon]

Manpages are in

[mheon]

I'm going to drop the WIP. Tests are still missing, though. Need to think about how we can test this in a way that doesn't introduce a half dozen more race conditions.

[mheon]

bot, retest this please

[rhatdan]

Restart policy is not allowed to be specified if user also specifies --rm.

[rhatdan]

Docker also supports unless-stopped

[rhatdan]

    -   **RestartPolicy** – The behavior to apply when the container exits.  The
            value is an object with a `Name` property of either `"always"` to
            always restart, `"unless-stopped"` to restart always except when
            user has manually stopped the container or `"on-failure"` to restart only when the container
            exit code is non-zero.  If `on-failure` is used, `MaximumRetryCount`
            controls the number of times to retry before giving up.
            The default is not to restart. (optional)
            An ever increasing delay (double the previous delay, starting at 100mS)
            is added before each restart to prevent flooding the server.

[mheon]

Unless-stopped is very much a "restarted the daemon" thing, so I just throw an error about it in pkg/spec if they try to pass it.

We still want to point people to systemd for cases like that, where the container will be restarted after system reboot, for example. The manpages make this clear, but it might be good to add the warning to Podman itself too.

[rhatdan]

My reading of unless-stopped, seems to be exactly what you are designing. IE If a user stops a container then it will not restart, otherwise if the container fails, it will be restarted.

[vrothberg]

It seems the interpretation depends on where we look. The man page is referring to the daemon start only while the docs state:

Similar to always, except that when the container is stopped (manually or otherwise), it is not restarted even after Docker daemon restarts

This makes me believe that we can support it.

[mheon]

From that description, it's identical to always - restart policy never triggers after the container is stopped via an API call.

[mheon]

(Minus the daemon restart aspect)

[mheon]

Unless stopped only makes sense with a Daemon. We detect it being passed in and error that it's not supported.

…

On Tue, Apr 2, 2019, 09:04 Daniel J Walsh ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In libpod/container.go <#2826 (comment)>: > @@ -102,6 +102,20 @@ func (ns LinuxNS) String() string { } } +// Valid restart policy types. +const ( + // RestartPolicyNone indicates that no restart policy has been requested + // by a container. + RestartPolicyNone = "" + // RestartPolicyNo is identical in function to RestartPolicyNone. + RestartPolicyNo = "no" + // RestartPolicyAlways unconditionally restarts the container. + RestartPolicyAlways = "always" + // RestartPolicyOnFailure restarts the container on non-0 exit code, + // with an optional maximum number of retries. + RestartPolicyOnFailure = "on-failure" Docker also supports unless-stopped ------------------------------ In libpod/container.go <#2826 (comment)>: > @@ -102,6 +102,20 @@ func (ns LinuxNS) String() string { } } +// Valid restart policy types. +const ( + // RestartPolicyNone indicates that no restart policy has been requested + // by a container. + RestartPolicyNone = "" + // RestartPolicyNo is identical in function to RestartPolicyNone. + RestartPolicyNo = "no" + // RestartPolicyAlways unconditionally restarts the container. + RestartPolicyAlways = "always" + // RestartPolicyOnFailure restarts the container on non-0 exit code, + // with an optional maximum number of retries. + RestartPolicyOnFailure = "on-failure" - **RestartPolicy** – The behavior to apply when the container exits. The value is an object with a `Name` property of either `"always"` to always restart, `"unless-stopped"` to restart always except when user has manually stopped the container or `"on-failure"` to restart only when the container exit code is non-zero. If `on-failure` is used, `MaximumRetryCount` controls the number of times to retry before giving up. The default is not to restart. (optional) An ever increasing delay (double the previous delay, starting at 100mS) is added before each restart to prevent flooding the server. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2826 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHYHCMVjeZm32hMbFQh4irBy-LiWuE14ks5vc1VCgaJpZM4cWdiC> .

[mheon]

--rm and --restart now conflict

[stevekuznetsov]

/retest

[kunalkushwaha]

I tested the PR locally with all condition.. working fine as expected.

---

One question regarding flow of control for triggering handleRestartPolicy() or container.Cleanup() function, how this is invoked when podman is not running?

[mheon]

In short, it's not.

This version of restart policy takes effect when the Podman cleanup process fires. When a Podman container exits, conmon launches the podman cleanup command to clean mounts, handle --rm, etc. The major component of this is to call Cleanup(), which has the authority to invoke restart policy.

We can't actually detect if a restart is required there, though, as in some cases (podman run specifically), we have other processes that might be checking the container's state before podman cleanup gets there. These processes may pick up the container's state change from running to stopped before podman cleanup does, so we need to store the restart policy trigger in the database. The good news is that the place where it's detected, syncContainer(), is called before Cleanup() as well (and all other Podman commands that can modify a container's state), so we're guaranteed to pick up the condition that triggers the restart, and when the cleanup command executes we'll know to perform a restart.

There are limitations to this approach - most notably, we cannot restart containers after a system reboot, because we don't have a running daemon to handle restart policy, and we don't know when a Podman process will first run. For use cases requiring that, using systemd unit files is still recommended - @baude was discussing writing a podman generate systemd command to assist in generating them.

[rhatdan]

@mheon The information you just typed, should probably be added to the man page, and perhaps a troublshoot.md (Minus the stuff about podman generate systemd, (Until it exists.)

[rhatdan]

bot, retest this please

[rhatdan]

LGTM

[mheon]

Working on tests now. Once they're in, I'll rebase to pick up Cirrus fixes and we can merge

[mheon]

Comments addressed, let's see if CI starts to behave

[mheon]

/retest

[TomSweeneyRedHat]

s/This this/If this/

[TomSweeneyRedHat]

ditto This this

[TomSweeneyRedHat]

LGTM, but would like another head nod or two.

[rhatdan]

Once this passes test, I will merge.

[mheon]

Tests are finally starting to pass

[TomSweeneyRedHat]

I'll beat @rhatdan to the punch.
/lgtm