Skip to content

Prevent containers from being included in List API before they are registered#44629

Merged
cpuguy83 merged 3 commits intomoby:masterfrom
corhere:fix-44512
Dec 13, 2022
Merged

Prevent containers from being included in List API before they are registered#44629
cpuguy83 merged 3 commits intomoby:masterfrom
corhere:fix-44512

Conversation

@corhere
Copy link
Contributor

@corhere corhere commented Dec 12, 2022

- What I did

  • Resolved a bug where containers were being included in the List Containers API response before they finished being created
  • Resolved a bug where calling Container APIs with the ID of a container in the process of being created would result in a nil-pointer dereference panic

- How I did it
See commit descriptions for details.

- How to verify it
Patch the daemon to widen the race window and try to reproduce the issue, as described in #44512 (comment)

- Description for the changelog

  • Resolved an issue where the List Containers API would return containers in the process of being created

- A picture of a cute animal (not mandatory but encouraged)

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 <[email protected]>
(*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 moby#44512.

Signed-off-by: Cory Snider <[email protected]>
(*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 (moby#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 <[email protected]>
@corhere corhere added area/api API kind/bugfix PR's that fix bugs labels Dec 12, 2022
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I was unsure about the reasoning behind some of the changes, but the commit messages explain them in exhaustive detail 😃

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Changes look benign.
I also checked to make sure the removed checkpoint call wasn't something where things are working because of that call to checkpoint (by coincidence), and it seems OK.

@cpuguy83
Copy link
Member

That said, perhaps we should make the move to have the inspect endpoint use the viewdb at this point.
When it was first introduced we didn't do that because we wanted to make sure it worked in the list API.

@cpuguy83 cpuguy83 merged commit 44a4ffd into moby:master Dec 13, 2022
@corhere
Copy link
Contributor Author

corhere commented Dec 13, 2022

I also checked to make sure the removed checkpoint call wasn't something where things are working because of that call to checkpoint (by coincidence), and it seems OK.

If anything, I suspect 388fe4a would be more likely to break code which worked by coincidence. Needless to say, I will be leaving that commit out of the v23.0 backport PR.

@cpuguy83
Copy link
Member

Yep, I checked that code path as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

List API includes partially-created containers which cannot be inspected

4 participants