From f8363690ca19ef03a990b789569b1497b282209d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 14 Aug 2023 13:04:15 +0200 Subject: [PATCH 1/2] daemon: Daemon.handleContainerExit(): reduce ambiguity in error handling This goroutine was added in c458bca6dc25c25d3345b093592167f2bd5e4af9, and looks for errors from the wait channel. If no error is returned, it attempts to start the container, and *updates* the error if a failure happened while doing so, so that the code below it can update the container's status, and perform auto-remove (if set for the container). However, due to the formatting of the code, it was easy to overlook that the "err" variable was not local to the "if" statement. This patch breaks up the if-statement in an attempt to make it clearer that this is not a local "err" variable, and adds a code-comment explaining the logic. Signed-off-by: Sebastiaan van Stijn --- daemon/monitor.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/daemon/monitor.go b/daemon/monitor.go index d8ed3417a9..d604e4835d 100644 --- a/daemon/monitor.go +++ b/daemon/monitor.go @@ -121,7 +121,11 @@ func (daemon *Daemon) handleContainerExit(c *container.Container, e *libcontaine // So to avoid panic at startup process, here must wait util daemon restore done. daemon.waitForStartupDone() cfg := daemon.config() // Apply the most up-to-date daemon config to the restarted container. - if err = daemon.containerStart(context.Background(), cfg, c, "", "", false); err != nil { + + // update the error if we fail to start the container, so that the cleanup code + // below can handle updating the container's status, and auto-remove (if set). + err = daemon.containerStart(context.Background(), cfg, c, "", "", false) + if err != nil { log.G(ctx).Debugf("failed to restart container: %+v", err) } } From 18a0ff2b2b16e40b5c702dac3aecbdf3263aa922 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 14 Aug 2023 15:17:43 +0200 Subject: [PATCH 2/2] daemon: Daemon.handleContainerExit(): rename err-var for clarity The "cpErr" naming was a bit confusing; give it a more descriptive name. Signed-off-by: Sebastiaan van Stijn --- daemon/monitor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/monitor.go b/daemon/monitor.go index d604e4835d..5a4aeb4eb0 100644 --- a/daemon/monitor.go +++ b/daemon/monitor.go @@ -108,7 +108,7 @@ func (daemon *Daemon) handleContainerExit(c *container.Container, e *libcontaine defer c.Unlock() // needs to be called before autoRemove daemon.setStateCounter(c) - cpErr := c.CheckpointTo(daemon.containersReplica) + checkpointErr := c.CheckpointTo(daemon.containersReplica) daemon.LogContainerEventWithAttributes(c, "die", attributes) @@ -143,7 +143,7 @@ func (daemon *Daemon) handleContainerExit(c *container.Container, e *libcontaine }() } - return cpErr + return checkpointErr } // ProcessEvent is called by libcontainerd whenever an event occurs