Prevent containers from being included in List API before they are registered#44629
Prevent containers from being included in List API before they are registered#44629cpuguy83 merged 3 commits intomoby:masterfrom
Conversation
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]>
neersighted
left a comment
There was a problem hiding this comment.
LGTM, I was unsure about the reasoning behind some of the changes, but the commit messages explain them in exhaustive detail 😃
cpuguy83
left a comment
There was a problem hiding this comment.
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.
|
That said, perhaps we should make the move to have the inspect endpoint use the viewdb at this point. |
If anything, I suspect 388fe4a would be more likely to break code which worked by coincidence. |
|
Yep, I checked that code path as well. |
- What I did
- 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
- A picture of a cute animal (not mandatory but encouraged)