Skip to content

c8d/mount: Use ref-counted mounter by default#47105

Merged
vvoland merged 1 commit intomoby:masterfrom
vvoland:c8d-mount-refcount-default
Jan 18, 2024
Merged

c8d/mount: Use ref-counted mounter by default#47105
vvoland merged 1 commit intomoby:masterfrom
vvoland:c8d-mount-refcount-default

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented Jan 18, 2024 •
edited
Loading

All commonly used filesystems should use ref-counted mounter, so make it the default instead of having to whitelist them.

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

@vvoland vvoland added area/runtime Runtime status/2-code-review containerd-integration Issues and PRs related to containerd integration labels Jan 18, 2024
@vvoland vvoland self-assigned this Jan 18, 2024
@vvoland vvoland mentioned this pull request Jan 18, 2024
Closed
rumpl
rumpl reviewed Jan 18, 2024
View reviewed changes
@vvoland
Copy link
Contributor Author

vvoland commented Jan 18, 2024

Looks like this solves the TestRestartContainerwithRestartPolicy and TestRestartStoppedContainer.

It also fixes TestCreateWithWorkdir!

@TBBle
Copy link
Contributor

TBBle commented Jan 18, 2024 •
edited
Loading

I did see it fail in

=== Failed
=== FAIL: amd64.integration.daemon TestLiveRestore/volume_references/local_volume_with_mount_options (2.34s)
    daemon_test.go:581: assertion failed: error is not nil: Error response from daemon: remove test-live-restore-volume-references-local: volume has active mounts
        --- FAIL: TestLiveRestore/volume_references/local_volume_with_mount_options (2.34s)

=== FAIL: amd64.integration.daemon TestLiveRestore/volume_references (16.91s)
    --- FAIL: TestLiveRestore/volume_references (16.91s)

=== FAIL: amd64.integration.daemon TestLiveRestore (0.00s)

but only in test (graphdriver) / integration (ubuntu-20.04, rootless).

I have no idea if that's a flake, or happens to be using the filesystem that needs to not be ref-counted, and triggers it because of how it's using mount options.

I flag it because different mount options seem like something that should produce a new mount rather than refcounting an existing mount, and I don't see any code in func (m *refCountMounter) Mount(mounts []mount.Mount, containerID string) (target string, retErr error) to ensure we don't accidentally inc-ref when passed read-write mounts when previously called with read-only mounts. Maybe that's not possible by usage, but the API appears to allow it. (In non-refcount mode, it'll mount over the top, which seems bad too...)

@vvoland
Copy link
Contributor Author

vvoland commented Jan 18, 2024 •
edited
Loading

It's a known flaky test: #46508

and it's not related here, the live restore volume mount does not happen through this ref counted mounter.

@vvoland
c8d/mount: Use ref-counted mounter by default
ae6468b 
All commonly used filesystems should use ref-counted mounter, so make it
the default instead of having to whitelist them.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland force-pushed the c8d-mount-refcount-default branch from 79557e8 to ae6468b Compare January 18, 2024 14:39
@thaJeztah thaJeztah added this to the 25.0.0 milestone Jan 18, 2024
@vvoland vvoland merged commit 7cbf01b into moby:master Jan 18, 2024
@vvoland vvoland mentioned this pull request Jun 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rumpl rumpl rumpl approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

Assignees

@vvoland vvoland

Labels

area/runtime Runtime containerd-integration Issues and PRs related to containerd integration status/2-code-review

Projects

containerd integration
Status: Done

Milestone

25.0.0

Development

Successfully merging this pull request may close these issues.

c8d: unconditionally use ref-counting, or get info from snapshotter

5 participants

@vvoland @TBBle @rumpl @dmcgowan @thaJeztah