Skip to content

daemon/c8d: Unmount container fs after unclean shutdown#46770

Merged
vvoland merged 1 commit intomoby:masterfrom
vvoland:c8d-unmount-empty-basefs
Nov 27, 2023
Merged

daemon/c8d: Unmount container fs after unclean shutdown#46770
vvoland merged 1 commit intomoby:masterfrom
vvoland:c8d-unmount-empty-basefs

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented Nov 3, 2023 •
edited
Loading

BaseFS is not serialized and is lost after an unclean shutdown. Unmount method in the containerd image service implementation will not work correctly in that case.
This patch will allow Unmount to restore the BaseFS if the target is still mounted.

The reason it works with graphdrivers is that it doesn't directly operate on BaseFS. It uses RWLayer, which is explicitly restored immediately as soon as container is loaded.

- What I did

- How I did it

- How to verify it

$ make TEST_FILTER='TestCleanupMountsAfterDaemonAndContainerKill'   TEST_IGNORE_CGROUP_CHECK=1    DOCKER_GRAPHDRIVER=overlayfs TEST_INTEGRATION_USE_SNAPSHOTTER=1    test-integration
-=== FAIL: amd64.integration-cli TestDockerDaemonSuite/TestCleanupMountsAfterDaemonAndContainerKill (1.57s)
-    docker_cli_daemon_test.go:1326: [de493fadc3745] daemon is not started
-    docker_cli_daemon_test.go:1331: assertion failed: strings.Contains(string(mountOut), id) is true: f86f0c70e20064dc73d47b3561f1946ca210aa45baafbed69ce4c4c7f8920924 is still mounted from older daemon start:
-(...)
---- FAIL: TestDockerDaemonSuite/TestCleanupMountsAfterDaemonAndContainerKill (1.57s)
-
-=== FAIL: amd64.integration-cli TestDockerDaemonSuite (292.98s)
+=== RUN   TestDockerDaemonSuite/TestCleanupMountsAfterDaemonAndContainerKill
+    docker_cli_daemon_test.go:1326: [d2a267ea1e12e] daemon is not started
+--- PASS: TestDockerDaemonSuite (1.82s)
+    --- PASS: TestDockerDaemonSuite/TestCleanupMountsAfterDaemonAndContainerKill (1.82s)

- Description for the changelog

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

@vvoland vvoland added status/2-code-review area/daemon Core Engine kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration labels Nov 3, 2023
@rumpl rumpl mentioned this pull request Nov 3, 2023
Closed
54 tasks
@vvoland
Copy link
Contributor Author

vvoland commented Nov 3, 2023

Alternative solution could be to restore the BaseFS as soon as possible like the graphdriver implementation does with the RWLayer:

moby/daemon/daemon.go

Lines 281 to 288 in ed1a61d

if accessor, ok := daemon.imageService.(layerAccessor); ok {
rwlayer, err := accessor.GetLayerByID(c.ID)
if err != nil {
logger.WithError(err).Error("failed to load container mount")
return
}
c.RWLayer = rwlayer
}

We kinda already have it, but it's peformed later - there's a funny part later which does Mount+Unmount to populate the BaseFS:

moby/daemon/daemon.go

Lines 459 to 471 in ed1a61d

if err := daemon.Mount(c); err != nil {
// The mount is unlikely to fail. However, in case mount fails
// the container should be allowed to restore here. Some functionalities
// (like docker exec -u user) might be missing but container is able to be
// stopped/restarted/removed.
// See #29365 for related information.
// The error is only logged here.
logger(c).WithError(err).Warn("failed to mount container to get BaseFs path")
} else {
if err := daemon.Unmount(c); err != nil {
logger(c).WithError(err).Warn("failed to umount container to get BaseFs path")
}
}

that's too late for the "cleanup alive containers" at startup, because the cleanup is caused by the event sent when the containerd task is killed and it gets called before the Mount+Unmount code has the chance to run.

@vvoland vvoland self-assigned this Nov 3, 2023
thaJeztah
thaJeztah reviewed Nov 3, 2023
View reviewed changes
@vvoland vvoland force-pushed the c8d-unmount-empty-basefs branch from d40467a to cbfb535 Compare November 3, 2023 17:46
thaJeztah
thaJeztah reviewed Nov 6, 2023
View reviewed changes
daemon/snapshotter/mount.go
Comment on lines +124 to +126
// Check if the refcount is non-zero.
m.rc.Increment(target)
if m.rc.Decrement(target) > 0 {
Copy link
Member

@thaJeztah thaJeztah Nov 6, 2023

Choose a reason for hiding this comment

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

Not sure if I understand this part; we're incrementing , and immediately after that decrementing.

Is this because we don't have a different way to get the active counter?

Copy link
Member

@rumpl rumpl Nov 6, 2023

Choose a reason for hiding this comment

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

Yeah, graphdriver.RefCount only has Increment and Decrement

Copy link
Member

@thaJeztah thaJeztah Nov 23, 2023

Choose a reason for hiding this comment

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

I created a ticket for this; if we want to track that in code, we could add TODO comment here so that we don't forget.

Copy link
Member

@thaJeztah thaJeztah Nov 23, 2023

Choose a reason for hiding this comment

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

Also opened a tracking issue for handling reference-counting, as I was looking at this code, and stumbled on the hard-coded `` list again 😂

@vvoland vvoland force-pushed the c8d-unmount-empty-basefs branch from cbfb535 to b2aa90c Compare November 23, 2023 10:33
@thaJeztah thaJeztah mentioned this pull request Nov 23, 2023
Open
thaJeztah
thaJeztah approved these changes Nov 23, 2023
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, but perhaps could use more eyes

@thaJeztah thaJeztah added this to the 25.0.0 milestone Nov 23, 2023
rumpl
rumpl reviewed Nov 27, 2023
View reviewed changes
@vvoland
daemon/c8d: Unmount container fs after unclean shutdown
203bac0 
BaseFS is not serialized and is lost after an unclean shutdown. Unmount
method in the containerd image service implementation will not work
correctly in that case.
This patch will allow Unmount to restore the BaseFS if the target is
still mounted.

The reason it works with graphdrivers is that it doesn't directly
operate on BaseFS. It uses RWLayer, which is explicitly restored
immediately as soon as container is loaded.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland force-pushed the c8d-unmount-empty-basefs branch from b2aa90c to 203bac0 Compare November 27, 2023 11:33
rumpl
rumpl approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

Looking great, thanks

@vvoland
Copy link
Contributor Author

vvoland commented Nov 27, 2023

Thanks for the review!

@vvoland vvoland merged commit 4cd2654 into moby:master Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@rumpl rumpl rumpl approved these changes

Assignees

@vvoland vvoland

Labels

area/daemon Core Engine containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs status/2-code-review

Projects

containerd integration
Status: Done

Milestone

25.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@vvoland @rumpl @thaJeztah