Skip to content

Commit

Permalink
daemon: fix restoring containers with name matching an ID
Browse files Browse the repository at this point in the history
This patch fixes a bug in the daemon's restore step on startup, where
a container with a name matching another container's ID would not be
restored.

`Daemon.registerName` is used during startup as part of the daemon's
container restore code
https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/daemon.go#L331-L344

In that process, it first registers the containers names through
[`Daemon.registerName()`][1], then registers the container's ID through
[`Daemon.Register()`][1], which calls `Daemon.containers.Add()` under the
hood.

Restoring containers is done in a goroutine, and at this stage of the daemon's
lifecycle, not all containers may be restored yet. However, `Daemon.registerName()`
has some safeguard to prevent the same container from being restored _twice_
through [`Daemon.Exists()`][3]. If a duplicate is found, an error is logged, and
the container is not restored (but kept on disk).

While it's disputable if this logic is needed at all, perhaps a panic would be
more appropriate (duplicate containers were stored on disk), there's also a
flaw in the current implementation of this check.

The [`Daemon.Exists()`][3] function uses [`Daemon.GetContainer()`][4] to look
up the container. This function performs fuzzy matching on the given reference,
first trying to match containers on their full ID, which _should_ not give a
match at this stage, before falling back to matching containers by name and
partial prefix.

This last part can be problematic in situations where a container exists that
uses the container to restore's ID as name. In such cases, the container will
be considered "already present", and not restored.

Create a container, then create a number of containers, each of which using
the ID of the previous container as name.

    docker create --name one hello-world
    d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab

    docker create --name d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab hello-world
    217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d
    docker create --name 217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d hello-world
    b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4
    docker create --name b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 hello-world

The daemon should now have a number of containers where the ID and name
conflict:

    docker ps -a --no-trunc --format 'table {{.ID}}\t{{.Names}}'
    CONTAINER ID                                                       NAMES
    f59e8e4044471c45d4c9841d11a2c586cbfa4703b1344035fd51a15e15899ea7   b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4
    b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4   217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d
    217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d   d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab
    d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab   one

Restart the daemon. Depending on the order in which containers are restored,
a conflict may happen, and the conflicting container will not be restored.
Logs below are from the daemon with debug enabled;

    INFO[2024-10-15T11:13:38.770744797Z] Loading containers: start.
    DEBU[2024-10-15T11:13:38.771152214Z] processing event stream                       module=libcontainerd namespace=moby
    DEBU[2024-10-15T11:13:38.771599797Z] loaded container                              container=d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab paused=false running=false
    DEBU[2024-10-15T11:13:38.771637464Z] loaded container                              container=217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d paused=false running=false
    DEBU[2024-10-15T11:13:38.771672714Z] loaded container                              container=bbe03a6554867810c2d7464ed3cb853865c755bae797b8d1f4caf60fb3f9fa04 paused=false running=false
    DEBU[2024-10-15T11:13:38.771765297Z] loaded container                              container=f59e8e4044471c45d4c9841d11a2c586cbfa4703b1344035fd51a15e15899ea7 paused=false running=false
    DEBU[2024-10-15T11:13:38.771780839Z] loaded container                              container=b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 paused=false running=false
    ERRO[2024-10-15T11:13:38.772114505Z] failed to register container name: /217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d  container=b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4 error="container is already loaded"

And the conflicting container (`217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d`)
is not present:

    docker ps -a --no-trunc --format 'table {{.ID}}\t{{.Names}}'
    CONTAINER ID                                                       NAMES
    f59e8e4044471c45d4c9841d11a2c586cbfa4703b1344035fd51a15e15899ea7   b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4
    b125af485d6d1875b586b314f93af1b49d5baaa94cec4199ae4ef4c6da05e7e4   217c53b9826eb7875ca2620596864d039848470befeb5f963b3ebffe509e7a6d
    d54301b7560f3c3544acc2d9c9dd55a194d6db37c2af64fe83fa34238c7ce6ab   one

[1]: https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/names.go#L22-L38
[2]: https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/container.go#L106-L121
[3]: https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/container.go#L71-L76
[4]: https://github.com/moby/moby/blob/97b1233a15c86afa627ffbb60748b530bc3d19bb/daemon/container.go#L30-L69

Signed-off-by: Sebastiaan van Stijn <[email protected]>
  • Loading branch information
thaJeztah committed Oct 15, 2024
1 parent 71977a8 commit a602054
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion daemon/names.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ func (daemon *Daemon) registerName(container *container.Container) error {
if container.ID == "" {
return fmt.Errorf("invalid empty id")
}
if daemon.Exists(container.ID) {
if daemon.containers.Get(container.ID) != nil {
// TODO(thaJeztah): should this be a panic (duplicate IDs due to invalid state on disk?)
// TODO(thaJeztah): should this also check for container.ID being a prefix of another container's ID? (daemon.containersReplica.GetByPrefix); only should happen due to corruption / truncated ID.
return fmt.Errorf("container is already loaded")
}
if container.Name == "" {
Expand Down

0 comments on commit a602054

Please sign in to comment.