From dffe634c19ff0f78e4f108ed92553c482dcba7d7 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 11 Aug 2023 20:46:53 +0200 Subject: [PATCH 1/2] daemon: Daemon.ContainerStart(): make validateState a regular function There's no need for this to be a closure; let's just make it a regular function. While moving it out, also make some minor code-changes and add some code-comments to describe the flow / intent, which may not be trivial for people that are not familiar with these details. Signed-off-by: Sebastiaan van Stijn --- daemon/start.go | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/daemon/start.go b/daemon/start.go index 87bb8fba55..105362bbb3 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -14,6 +14,30 @@ import ( "github.com/pkg/errors" ) +// validateState verifies if the container is in a non-conflicting state. +func validateState(ctr *container.Container) error { + ctr.Lock() + defer ctr.Unlock() + + // Intentionally checking paused first, because a container can be + // BOTH running AND paused. To start a paused (but running) container, + // it must be thawed ("un-paused"). + if ctr.Paused { + return errdefs.Conflict(errors.New("cannot start a paused container, try unpause instead")) + } else if ctr.Running { + // This is not an actual error, but produces a 304 "not modified" + // when returned through the API to indicates the container is + // already in the desired state. It's implemented as an error + // to make the code calling this function terminate early (as + // no further processing is needed). + return containerNotModifiedError{running: true} + } + if ctr.RemovalInProgress || ctr.Dead { + return errdefs.Conflict(errors.New("container is marked for removal and cannot be started")) + } + return nil +} + // ContainerStart starts a container. func (daemon *Daemon) ContainerStart(ctx context.Context, name string, hostConfig *containertypes.HostConfig, checkpoint string, checkpointDir string) error { daemonCfg := daemon.config() @@ -25,26 +49,7 @@ func (daemon *Daemon) ContainerStart(ctx context.Context, name string, hostConfi if err != nil { return err } - - validateState := func() error { - ctr.Lock() - defer ctr.Unlock() - - if ctr.Paused { - return errdefs.Conflict(errors.New("cannot start a paused container, try unpause instead")) - } - - if ctr.Running { - return containerNotModifiedError{running: true} - } - - if ctr.RemovalInProgress || ctr.Dead { - return errdefs.Conflict(errors.New("container is marked for removal and cannot be started")) - } - return nil - } - - if err := validateState(); err != nil { + if err := validateState(ctr); err != nil { return err } From 80d158e0deb496cde147cfdfe48cdb5025351679 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 11 Aug 2023 21:07:19 +0200 Subject: [PATCH 2/2] daemon: remove containerNotModifiedError Removing this type, because: - containerNotModifiedError is not an actual error, and abstracting it away was hiding some of these details. It also wasn't used as a sentinel error anywhere, so doesn't have to be its own type. - Defining a type just to toggle the error-message between "not running" and "not stopped" felt a bit over-the-top, as each variant was only used once. - So "it only had one job", and it didn't even do that right; it produced capitalized error messages, which makes linters unhappy. So, let's just inline what it does in the two places it was used. Signed-off-by: Sebastiaan van Stijn --- daemon/errors.go | 13 ------------- daemon/start.go | 2 +- daemon/stop.go | 7 ++++++- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/daemon/errors.go b/daemon/errors.go index 7282e4f462..803c070f12 100644 --- a/daemon/errors.go +++ b/daemon/errors.go @@ -59,19 +59,6 @@ func (e nameConflictError) Error() string { func (nameConflictError) Conflict() {} -type containerNotModifiedError struct { - running bool -} - -func (e containerNotModifiedError) Error() string { - if e.running { - return "Container is already started" - } - return "Container is already stopped" -} - -func (e containerNotModifiedError) NotModified() {} - type invalidIdentifier string func (e invalidIdentifier) Error() string { diff --git a/daemon/start.go b/daemon/start.go index 105362bbb3..110d9c222d 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -30,7 +30,7 @@ func validateState(ctr *container.Container) error { // already in the desired state. It's implemented as an error // to make the code calling this function terminate early (as // no further processing is needed). - return containerNotModifiedError{running: true} + return errdefs.NotModified(errors.New("container is already running")) } if ctr.RemovalInProgress || ctr.Dead { return errdefs.Conflict(errors.New("container is marked for removal and cannot be started")) diff --git a/daemon/stop.go b/daemon/stop.go index b5b1506d73..4d22a81945 100644 --- a/daemon/stop.go +++ b/daemon/stop.go @@ -26,7 +26,12 @@ func (daemon *Daemon) ContainerStop(ctx context.Context, name string, options co return err } if !ctr.IsRunning() { - return containerNotModifiedError{} + // This is not an actual error, but produces a 304 "not modified" + // when returned through the API to indicates the container is + // already in the desired state. It's implemented as an error + // to make the code calling this function terminate early (as + // no further processing is needed). + return errdefs.NotModified(errors.New("container is already stopped")) } err = daemon.containerStop(ctx, ctr, options) if err != nil {