Skip to content

Replace mount fork hack with CLONE_FS #7513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dmcgowan merged 1 commit into from
Nov 7, 2022
Merged

Replace mount fork hack with CLONE_FS #7513

dmcgowan merged 1 commit into from
Nov 7, 2022

Conversation

@cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Oct 11, 2022

This change spins up a new goroutine, locks it to a thread, then unshares CLONE_FS which allows us to Chdir from inside the thread without affecting the rest of the program.

The thread is no longer usable after unshare so it leaves the thread locked to prevent go from returning the thread to the thread pool. (see https://pkg.go.dev/runtime#UnlockOSThread)

Thanks @corhere for working on a bunch of this stuff in moby and pointing out that we should be able to do this instead.

fuweid
fuweid approved these changes Oct 12, 2022
View reviewed changes
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM on green!

Thanks!

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Oct 12, 2022

Would it be possible to have some unit tests?

fuweid
fuweid reviewed Oct 12, 2022
View reviewed changes
@cpuguy83
Copy link
Member Author

cpuguy83 commented Oct 12, 2022

Would it be possible to have some unit tests?

Test added.

@cpuguy83
Copy link
Member Author

cpuguy83 commented Oct 12, 2022

Interesting error I've not seen before: https://github.com/containerd/containerd/actions/runs/3237587338/jobs/5304881990#step:9:56393
Eventually it panics.

@cpuguy83 cpuguy83 mentioned this pull request Oct 12, 2022
Closed
kzys
kzys approved these changes Oct 12, 2022
View reviewed changes
mount/mount_linux.go
// If the thread is unlocked go will try to use it for other goroutines.
// However it is not possible to restore the thread state after CLONE_FS.
//
// Once the goroutine exits the thread should eventually be terminated by go.
Copy link
Member

@kzys kzys Oct 12, 2022

Choose a reason for hiding this comment

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

https://pkg.go.dev/runtime#LockOSThread

If the calling goroutine exits without unlocking the thread, the thread will be terminated.

So it will be fine.

AkihiroSuda
AkihiroSuda previously approved these changes Oct 12, 2022
View reviewed changes
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Oct 12, 2022 


=== Failed
=== FAIL: snapshots/overlay TestOverlay/no_opt/RemoveIntermediateSnapshot (unknown)
    helpers_unix.go:33: unmount /tmp/snapshot-suite-Overlay-179260178/work/base

=== FAIL: snapshots/overlay TestOverlay/no_opt/RootPermission (unknown)
    helpers_unix.go:33: unmount /tmp/snapshot-suite-Overlay-136791402/work/preparing

=== FAIL: snapshots/overlay TestOverlay/no_opt/Remove (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/DirectoryPermissionOnCommit (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/TestOverlayOverlayRead (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/TestOverlayView (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/Rename (unknown)
    issues.go:118: rename test still fails on some kernels with overlay

=== FAIL: snapshots/overlay TestOverlay/no_opt/StatInWalk (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/RemoveDirectoryInLowerLayer (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/MoveFileFromLowerLayer (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/Basic (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/Walk (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/Update (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/Chown (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/CloseTwice (unknown)

=== FAIL: snapshots/overlay TestOverlay/no_opt/128LayersMount (unknown)

https://github.com/containerd/containerd/actions/runs/3238000261/jobs/5305789503

@AkihiroSuda AkihiroSuda dismissed their stale review October 12, 2022 23:06

CI failing

@fuweid

This comment was marked as duplicate.

Sign in to view
@corhere corhere mentioned this pull request Oct 15, 2022
Closed
@cpuguy83 cpuguy83 force-pushed the nix_mount_fork branch 2 times, most recently from bee7db3 to 4a0fb1c Compare November 2, 2022 16:04
@kzys
Copy link
Member

kzys commented Nov 2, 2022

Wow. We have found a Go bug :)

@cpuguy83
Copy link
Member Author

cpuguy83 commented Nov 2, 2022

Should be fixed now with 1.19.3.
Last push passed everything except one weird devicemapper failure in one of the runners. Re-pushed so I can try to run it again.

@cpuguy83
Copy link
Member Author

cpuguy83 commented Nov 2, 2022

Windows failure looks unrelated.
The rockylinux test had an issue, the results don't seem to be showing up anymore but I recall it was something cgroup related which also seems unrelated... that said I've not noticed either of these failures before.

@estesp
Copy link
Member

estesp commented Nov 2, 2022

Should we get a separate PR for the usual "upgrade the world" to 1.19.3 versus just the one GH Actions CI setting? Should be able to get that in quickly and then rebase this

@cpuguy83 cpuguy83 mentioned this pull request Nov 2, 2022
Merged
@cpuguy83
Copy link
Member Author

cpuguy83 commented Nov 2, 2022

@estesp Good idea, #7620

@cpuguy83
Replace mount fork hack with CLONE_FS
a24ef09 
This change spins up a new goroutine, locks it to a thread, then
unshares CLONE_FS which allows us to `Chdir` from inside the thread
without affecting the rest of the program.

The thread is no longer usable after unshare so it leaves the thread
locked to prevent go from returning the thread to the thread pool.

Signed-off-by: Brian Goff <[email protected]>
@dmcgowan dmcgowan merged commit 816f708 into containerd:main Nov 7, 2022
@cpuguy83 cpuguy83 deleted the nix_mount_fork branch November 7, 2022 17:35
@fuweid fuweid mentioned this pull request Nov 20, 2022
Merged
@dmcgowan dmcgowan mentioned this pull request Dec 10, 2022
Merged
@fuweid fuweid mentioned this pull request Dec 16, 2022
Merged
4 tasks
@dmcgowan dmcgowan mentioned this pull request Jan 3, 2023
Merged
@dmcgowan dmcgowan mentioned this pull request Jan 28, 2023
Merged
@dmcgowan dmcgowan mentioned this pull request Feb 15, 2023
Merged
This was referenced Feb 25, 2023
Merged
Merged
This was referenced Mar 7, 2023
Merged
Merged
Merged
@tonistiigi tonistiigi mentioned this pull request Mar 29, 2023
Closed
@fuweid fuweid mentioned this pull request Apr 27, 2023
Merged
@Burning1020 Burning1020 mentioned this pull request May 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corhere corhere corhere left review comments

@kzys kzys kzys approved these changes

@fuweid fuweid fuweid approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

Assignees

No one assigned

Labels

impact/changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@cpuguy83 @AkihiroSuda @fuweid @kzys @estesp @corhere @dmcgowan