Skip to content

fix: make unmount handling robust for busy/disconnected mounts - #772

Open
qinfustu wants to merge 1 commit into
containerd:mainfrom
qinfustu:fix/umount-robust-busy-disconnected
Open

fix: make unmount handling robust for busy/disconnected mounts#772
qinfustu wants to merge 1 commit into
containerd:mainfrom
qinfustu:fix/umount-robust-busy-disconnected

Conversation

@qinfustu

Copy link
Copy Markdown

Overview

Make mount.Umount (and WaitUntilUnmounted) robust against busy and
disconnected mountpoints, so stale nydusd FUSE mounts are always torn down
instead of being left behind.

Related Issues

Related #771

Change Details

Previously Umount had two problems that leave residual mountpoints under
registry pressure / after a nydusd crash:

  1. Bails out on ENOTCONN. When the backing nydusd is dead, the FUSE mount
    becomes disconnected and IsMountpoint (via os.Stat) fails with ENOTCONN.
    The old code returned that error and never even attempted to unmount — i.e.
    the mountpoint that most needs cleaning up was skipped.
  2. Bare unmount gives up on EBUSY. syscall.Unmount(target, 0) had no
    fallback, so a busy mountpoint (old process still holding it) was left mounted.
    Changes:
  • Umount now treats an ENOTCONN/ESTALE mountpoint as "needs unmount" and
    proceeds instead of returning early; a genuinely-not-mounted path now returns
    nil (previously returned a misleading "not mounted" error).
  • New unmountWithFallback: plain unmount → MNT_FORCE (abort in-flight FUSE
    requests) → MNT_DETACH (lazy detach for busy mounts). EINVAL is treated as
    already unmounted.
  • WaitUntilUnmounted no longer reports a disconnected mountpoint as an error
    (it would otherwise spin for ~1s and log a misleading failure during teardown).
  • MNT_FORCE/MNT_DETACH are platform-specific, so they live in
    mount_linux.go / mount_darwin.go (darwin has no lazy detach and degrades to
    force; that build exists only for local dev — nydusd mounts are Linux-only at
    runtime).
  • Added small test seams (syscallUnmount, isMountpoint) to enable unit tests.
    The only production caller of mount.Mounter.Umount is daemon.ClearVestige, so
    the blast radius is small.

Test Results

New unit tests in pkg/utils/mount/mount_test.go:

  • TestIsDisconnectedENOTCONN/ESTALE detection, including through
    pkg/errors wrapping.
  • TestUnmountWithFallback — plain success, EINVAL short-circuit,
    EBUSY → force, force → lazy detach, and all-fail returns error.
  • TestUmount — disconnected mount still unmounts; non-mountpoint returns nil
    without unmounting; other errors propagate.
  • TestWaitUntilUnmountedIgnoresDisconnectedMountpoint — disconnected mount
    returns immediately without retrying.

Change Type

Please select the type of change your pull request relates to:

  • Bug Fix
  • Feature Addition
  • Documentation Update
  • Code Refactoring
  • Performance Improvement
  • Other (please describe)

Self-Checklist

Before submitting a pull request, please ensure you have completed the following:

  • I have run a code style check and addressed any warnings/errors.
  • I have added appropriate comments to my code (if applicable).
  • I have updated the documentation (if applicable).
  • I have written appropriate unit tests.

@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 26.29%. Comparing base (d16caad) to head (9a35551).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #772      +/-   ##
==========================================
+ Coverage   25.42%   26.29%   +0.87%     
==========================================
  Files         133      134       +1     
  Lines       12639    12888     +249     
==========================================
+ Hits         3213     3389     +176     
- Misses       9036     9088      +52     
- Partials      390      411      +21     
Files with missing lines Coverage Δ
pkg/utils/mount/mount.go 50.90% <100.00%> (+50.90%) ⬆️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Zephyrcf Zephyrcf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@qinfustu
qinfustu force-pushed the fix/umount-robust-busy-disconnected branch from 4c51cb0 to 9a35551 Compare July 29, 2026 08:21
@imeoer

imeoer commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

The force/lazy fallback needs a call from the maintainers, because unmountWithFallback returns nil after MNT_DETACH succeeds. Lazy unmount only detaches from the namespace — the mount stays live until the last reference goes away, so ClearVestige moves on and cleans up backing resources while a container may still be using it. MNT_FORCE similarly aborts in-flight FUSE requests and the container sees I/O errors. If we want this behaviour, it would be better to signal "unmounted, but not cleanly" back to the caller instead of a plain nil.

WaitUntilUnmounted returning nil on ENOTCONN looks wrong to me. A disconnected mountpoint is still mounted, the server is just gone. We're telling the caller the unmount finished when nothing was unmounted.

Two smaller ones:

  • Mounter.Umount now returns nil instead of errors.New("not mounted") for a non-mountpoint. That's a contract change on an exported method, please mention it in the description.
  • mount_darwin.go maps umountDetach to MNT_FORCE, so the fallback fires MNT_FORCE twice with different intent. We don't run on darwin, so I'd just drop the file rather than carry the extra surface.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants