From 42bffae5ffb8fa651416d358c7e23e7c836a47fa Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Mon, 12 Dec 2022 14:39:10 -0500 Subject: [PATCH 1/3] daemon: fix GetContainer() returning (nil, nil) GetContainer() would return (nil, nil) when looking up a container if the container was inserted into the containersReplica ViewDB but not the containers Store at the time of the lookup. Callers which reasonably assume that the returned err == nil implies returned container != nil would dereference a nil pointer and panic. Change GetContainer() so that it always returns a container or an error. Signed-off-by: Cory Snider (cherry picked from commit 00157a42d367eca1dc140a5638d41444ab7434ce) Signed-off-by: Cory Snider --- daemon/container.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/daemon/container.go b/daemon/container.go index ed850d42f3..52830f7289 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -57,7 +57,18 @@ func (daemon *Daemon) GetContainer(prefixOrName string) (*container.Container, e } return nil, errdefs.System(indexError) } - return daemon.containers.Get(containerID), nil + ctr := daemon.containers.Get(containerID) + if ctr == nil { + // Updates to the daemon.containersReplica ViewDB are not atomic + // or consistent w.r.t. the live daemon.containers Store so + // while reaching this code path may be indicative of a bug, + // it is not _necessarily_ the case. + logrus.WithField("prefixOrName", prefixOrName). + WithField("id", containerID). + Debugf("daemon.GetContainer: container is known to daemon.containersReplica but not daemon.containers") + return nil, containerNotFound(prefixOrName) + } + return ctr, nil } // checkContainer make sure the specified container validates the specified conditions From 6149c333ffb27579542ad36f973a109c82369715 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Mon, 12 Dec 2022 15:23:43 -0500 Subject: [PATCH 2/3] daemon: don't checkpoint container until registered (*Container).CheckpointTo() upserts a snapshot of the container to the daemon's in-memory ViewDB and also persists the snapshot to disk. It does not register the live container object with the daemon's container store, however. The ViewDB and container store are used as the source of truth for different operations, so having a container registered in one but not the other can result in inconsistencies. In particular, the List Containers API uses the ViewDB as its source of truth and the Container Inspect API uses the container store. The (*Daemon).setHostConfig() method is called fairly early in the process of creating a container, long before the container is registered in the daemon's container store. Due to a rogue CheckpointTo() call inside setHostConfig(), there is a window of time where a container can be included in a List Containers API response but "not exist" according to the Container Inspect API and similar endpoints which operate on a particular container. Remove the rogue call so that the caller has full control over when the container is checkpointed and update callers to checkpoint explicitly. No changes to (*Daemon).create() are needed as it checkpoints the fully-created container via (*Daemon).Register(). Fixes #44512. Signed-off-by: Cory Snider (cherry picked from commit 0141c6db8107c5dadcb37f970b5351b5f4b2cd9b) Signed-off-by: Cory Snider --- daemon/container.go | 2 +- daemon/start.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index 52830f7289..24af594590 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -235,7 +235,7 @@ func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig * runconfig.SetDefaultNetModeIfBlank(hostConfig) container.HostConfig = hostConfig - return container.CheckpointTo(daemon.containersReplica) + return nil } // verifyContainerSettings performs validation of the hostconfig and config diff --git a/daemon/start.go b/daemon/start.go index ecc0f8b8af..9d6f7812b6 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -67,9 +67,9 @@ func (daemon *Daemon) ContainerStart(name string, hostConfig *containertypes.Hos // if user has change the network mode on starting, clean up the // old networks. It is a deprecated feature and has been removed in Docker 1.12 ctr.NetworkSettings.Networks = nil - if err := ctr.CheckpointTo(daemon.containersReplica); err != nil { - return errdefs.System(err) - } + } + if err := ctr.CheckpointTo(daemon.containersReplica); err != nil { + return errdefs.System(err) } ctr.InitDNSHostConfig() } From dca58c654abc4efa46794681ad39339079984258 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Mon, 12 Dec 2022 16:04:09 -0500 Subject: [PATCH 3/3] daemon: drop side effect from registerLinks() (*Daemon).registerLinks() calling the WriteHostConfig() method of its container argument is a vestigial behaviour. In the distant past, registerLinks() would persist the container links in an SQLite database and drop the link config from the container's persisted HostConfig. This changed in Docker v1.10 (#16032) which migrated away from SQLite and began using the link config in the container's HostConfig as the persistent source of truth. registerLinks() no longer mutates the HostConfig at all so persisting the HostConfig to disk falls outside of its scope of responsibilities. Signed-off-by: Cory Snider (cherry picked from commit 388fe4aea82a37f38ff96db5594643b14b3f73e8) Signed-off-by: Cory Snider --- daemon/daemon_unix.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 68827029b6..cd602b3434 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -1309,7 +1309,8 @@ func getUnmountOnShutdownPath(config *config.Config) string { return filepath.Join(config.ExecRoot, "unmount-on-shutdown") } -// registerLinks writes the links to a file. +// registerLinks registers network links between container and other containers +// with the daemon using the specification in hostConfig. func (daemon *Daemon) registerLinks(container *container.Container, hostConfig *containertypes.HostConfig) error { if hostConfig == nil || hostConfig.NetworkMode.IsUserDefined() { return nil @@ -1353,10 +1354,7 @@ func (daemon *Daemon) registerLinks(container *container.Container, hostConfig * } } - // After we load all the links into the daemon - // set them to nil on the hostconfig - _, err := container.WriteHostConfig() - return err + return nil } // conditionalMountOnStart is a platform specific helper function during the