Allow containers to --restart on-failure with --rm

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

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"`)
 	}
 

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()
+}

libpod/container_internal.go

@@ -206,37 +206,39 @@ func (c *Container) handleExitFile(exitFile string, fi os.FileInfo) error {
 	return nil
 }
 
-// Handle container restart policy.
+func (c *Container) shouldRestart() bool {
-// This is called when a container has exited, and was not explicitly stopped by
+	// If we did not get a restart policy match, return false
-// 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.
 	// 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 {
+			if c.state.RestartCount >= c.config.RestartRetries {
-				logrus.Debugf("Container %s restart policy trigger: on retry %d (of %d)",
+				return false
-					c.ID(), c.state.RestartCount, c.config.RestartRetries)
-			} else {
-				logrus.Debugf("Container %s restart policy trigger: retries exhausted", c.ID())
-				return false, nil
 			}
 		}
 	}
+	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.

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)
+	}
+}

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
+}

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
+}

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 {
+			shouldRestart, err := containers.ShouldRestart(ic.ClientCxt, con.ID)
-				if errorhandling.Contains(err, define.ErrNoSuchCtr) ||
+			if err != nil {
-					errorhandling.Contains(err, define.ErrCtrRemoved) {
+				logrus.Errorf("Failed to check if %s should restart: %v", con.ID, err)
-					logrus.Warnf("Container %s does not exist: %v", con.ID, err)
+				return
-				} else {
+			}
-					logrus.Errorf("Error removing container %s: %v", con.ID, err)
+
+			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)
+}

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()