From dc8996ec84ffd6f272361edbf7c19e91c52519d9 Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Fri, 6 Nov 2020 09:55:40 -0500 Subject: Allow containers to --restart on-failure with --rm Signed-off-by: Daniel J Walsh --- cmd/podman/common/createparse.go | 2 +- libpod/container_api.go | 14 ++++++++++++++ libpod/container_internal.go | 28 +++++++++++++++------------- pkg/api/handlers/libpod/containers.go | 24 ++++++++++++++++++++++++ pkg/bindings/containers/containers.go | 12 ++++++++++++ pkg/domain/infra/abi/containers.go | 15 +++++++++++++-- pkg/domain/infra/tunnel/containers.go | 25 +++++++++++++++++++------ test/e2e/run_test.go | 4 +--- 8 files changed, 99 insertions(+), 25 deletions(-) diff --git a/cmd/podman/common/createparse.go b/cmd/podman/common/createparse.go index 09ee5aa0c..3a69f11b6 100644 --- a/cmd/podman/common/createparse.go +++ b/cmd/podman/common/createparse.go @@ -9,7 +9,7 @@ import ( // by validate must not need any state information on the flag (i.e. changed) func (c *ContainerCLIOpts) validate() error { var () - if c.Rm && c.Restart != "" && c.Restart != "no" { + if c.Rm && (c.Restart != "" && c.Restart != "no" && c.Restart != "on-failure") { return errors.Errorf(`the --rm option conflicts with --restart, when the restartPolicy is not "" and "no"`) } diff --git a/libpod/container_api.go b/libpod/container_api.go index a9808a30e..6a7ddc421 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -714,3 +714,17 @@ func (c *Container) Restore(ctx context.Context, options ContainerCheckpointOpti defer c.newContainerEvent(events.Restore) return c.restore(ctx, options) } + +// Indicate whether or not the container should restart +func (c *Container) ShouldRestart(ctx context.Context) bool { + logrus.Debugf("Checking if container %s should restart", c.ID()) + if !c.batched { + c.lock.Lock() + defer c.lock.Unlock() + + if err := c.syncContainer(); err != nil { + return false + } + } + return c.shouldRestart() +} diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 108954bad..ffd0dc92f 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -206,37 +206,39 @@ func (c *Container) handleExitFile(exitFile string, fi os.FileInfo) error { return nil } -// Handle container restart policy. -// This is called when a container has exited, and was not explicitly stopped by -// an API call to stop the container or pod it is in. -func (c *Container) handleRestartPolicy(ctx context.Context) (_ bool, retErr error) { - // If we did not get a restart policy match, exit immediately. +func (c *Container) shouldRestart() bool { + // If we did not get a restart policy match, return false // Do the same if we're not a policy that restarts. if !c.state.RestartPolicyMatch || c.config.RestartPolicy == RestartPolicyNo || c.config.RestartPolicy == RestartPolicyNone { - return false, nil + return false } // If we're RestartPolicyOnFailure, we need to check retries and exit // code. if c.config.RestartPolicy == RestartPolicyOnFailure { if c.state.ExitCode == 0 { - return false, nil + return false } // If we don't have a max retries set, continue if c.config.RestartRetries > 0 { - if c.state.RestartCount < c.config.RestartRetries { - logrus.Debugf("Container %s restart policy trigger: on retry %d (of %d)", - c.ID(), c.state.RestartCount, c.config.RestartRetries) - } else { - logrus.Debugf("Container %s restart policy trigger: retries exhausted", c.ID()) - return false, nil + if c.state.RestartCount >= c.config.RestartRetries { + return false } } } + return true +} +// Handle container restart policy. +// This is called when a container has exited, and was not explicitly stopped by +// an API call to stop the container or pod it is in. +func (c *Container) handleRestartPolicy(ctx context.Context) (_ bool, retErr error) { + if !c.shouldRestart() { + return false, nil + } logrus.Debugf("Restarting container %s due to restart policy %s", c.ID(), c.config.RestartPolicy) // Need to check if dependencies are alive. diff --git a/pkg/api/handlers/libpod/containers.go b/pkg/api/handlers/libpod/containers.go index 7e6481321..14eb44831 100644 --- a/pkg/api/handlers/libpod/containers.go +++ b/pkg/api/handlers/libpod/containers.go @@ -344,3 +344,27 @@ func InitContainer(w http.ResponseWriter, r *http.Request) { } utils.WriteResponse(w, http.StatusNoContent, "") } + +func ShouldRestart(w http.ResponseWriter, r *http.Request) { + runtime := r.Context().Value("runtime").(*libpod.Runtime) + // Now use the ABI implementation to prevent us from having duplicate + // code. + containerEngine := abi.ContainerEngine{Libpod: runtime} + + name := utils.GetName(r) + report, err := containerEngine.ShouldRestart(r.Context(), name) + if err != nil { + if errors.Cause(err) == define.ErrNoSuchCtr { + utils.ContainerNotFound(w, name, err) + return + } + utils.InternalServerError(w, err) + return + + } + if report.Value { + utils.WriteResponse(w, http.StatusNoContent, "") + } else { + utils.ContainerNotFound(w, name, define.ErrNoSuchCtr) + } +} diff --git a/pkg/bindings/containers/containers.go b/pkg/bindings/containers/containers.go index b5cd2128b..4331ae6c2 100644 --- a/pkg/bindings/containers/containers.go +++ b/pkg/bindings/containers/containers.go @@ -390,3 +390,15 @@ func ContainerInit(ctx context.Context, nameOrID string) error { } return response.Process(nil) } + +func ShouldRestart(ctx context.Context, nameOrID string) (bool, error) { + conn, err := bindings.GetClient(ctx) + if err != nil { + return false, err + } + response, err := conn.DoRequest(nil, http.MethodPost, "/containers/%s/shouldrestart", nil, nil, nameOrID) + if err != nil { + return false, err + } + return response.IsSuccess(), nil +} diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 4b69ac74e..ff4277a2e 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -911,7 +911,7 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta } else { report.ExitCode = int(ecode) } - if opts.Rm { + if opts.Rm && !ctr.ShouldRestart(ctx) { if err := ic.Libpod.RemoveContainer(ctx, ctr, false, true); err != nil { if errors.Cause(err) == define.ErrNoSuchCtr || errors.Cause(err) == define.ErrCtrRemoved { @@ -992,7 +992,7 @@ func (ic *ContainerEngine) ContainerCleanup(ctx context.Context, namesOrIds []st return []*entities.ContainerCleanupReport{}, nil } - if options.Remove { + if options.Remove && !ctr.ShouldRestart(ctx) { err = ic.Libpod.RemoveContainer(ctx, ctr, false, true) if err != nil { report.RmErr = errors.Wrapf(err, "failed to cleanup and remove container %v", ctr.ID()) @@ -1015,6 +1015,7 @@ func (ic *ContainerEngine) ContainerCleanup(ctx context.Context, namesOrIds []st _, err = ic.Libpod.RemoveImage(ctx, ctrImage, false) report.RmiErr = err } + reports = append(reports, &report) } return reports, nil @@ -1314,3 +1315,13 @@ func (ic *ContainerEngine) ContainerStats(ctx context.Context, namesOrIds []stri return statsChan, nil } + +// ShouldRestart returns whether the container should be restarted +func (ic *ContainerEngine) ShouldRestart(ctx context.Context, nameOrID string) (*entities.BoolReport, error) { + ctr, err := ic.Libpod.LookupContainer(nameOrID) + if err != nil { + return nil, err + } + + return &entities.BoolReport{Value: ctr.ShouldRestart(ctx)}, nil +} diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index 8066e1c00..1aa5afbe7 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/pkg/domain/infra/tunnel/containers.go @@ -595,12 +595,20 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta // Defer the removal, so we can return early if needed and // de-spaghetti the code. defer func() { - if err := containers.Remove(ic.ClientCxt, con.ID, bindings.PFalse, bindings.PTrue); err != nil { - if errorhandling.Contains(err, define.ErrNoSuchCtr) || - errorhandling.Contains(err, define.ErrCtrRemoved) { - logrus.Warnf("Container %s does not exist: %v", con.ID, err) - } else { - logrus.Errorf("Error removing container %s: %v", con.ID, err) + shouldRestart, err := containers.ShouldRestart(ic.ClientCxt, con.ID) + if err != nil { + logrus.Errorf("Failed to check if %s should restart: %v", con.ID, err) + return + } + + if !shouldRestart { + if err := containers.Remove(ic.ClientCxt, con.ID, bindings.PFalse, bindings.PTrue); err != nil { + if errorhandling.Contains(err, define.ErrNoSuchCtr) || + errorhandling.Contains(err, define.ErrCtrRemoved) { + logrus.Warnf("Container %s does not exist: %v", con.ID, err) + } else { + logrus.Errorf("Error removing container %s: %v", con.ID, err) + } } } }() @@ -737,3 +745,8 @@ func (ic *ContainerEngine) ContainerStats(ctx context.Context, namesOrIds []stri } return containers.Stats(ic.ClientCxt, namesOrIds, &options.Stream) } + +// ShouldRestart reports back whether the containre will restart +func (ic *ContainerEngine) ShouldRestart(_ context.Context, id string) (bool, error) { + return containers.ShouldRestart(ic.ClientCxt, id) +} diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go index 5ee85efb9..0d65a3e59 100644 --- a/test/e2e/run_test.go +++ b/test/e2e/run_test.go @@ -75,11 +75,9 @@ var _ = Describe("Podman run", func() { session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) - // the --rm option conflicts with --restart, when the restartPolicy is not "" and "no" - // so the exitCode should not equal 0 session = podmanTest.Podman([]string{"run", "--rm", "--restart", "on-failure", ALPINE}) session.WaitWithDefaultTimeout() - Expect(session.ExitCode()).To(Not(Equal(0))) + Expect(session.ExitCode()).To(Equal(0)) session = podmanTest.Podman([]string{"run", "--rm", "--restart", "always", ALPINE}) session.WaitWithDefaultTimeout() -- cgit v1.2.3-54-g00ecf