Skip to content

fix(kubernetes): log kubevirt-csi temp NFS mount dir removal failure - #4562

Merged
IvanHunters merged 1 commit into
mainfrom
fix/kubevirt-csi-nfs-tmpdir-error
Oct 5, 2026
Merged

IvanHunters merged 1 commit into
mainfrom
fix/kubevirt-csi-nfs-tmpdir-error

Conversation

@lexfrei

@lexfrei Aleksei Sviridkin (lexfrei) commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

What this PR does

The kubevirt-csi node plugin now logs a warning when it cannot remove the temp dir it uses for NFS publish. Before, the error was dropped, so a dir left behind after a failed unmount was never reported.

The publish path temp-mounts the export root to create /data, then removes the temp dir in a defer. The failure is logged, the same way the unmount failure next to it already is, and not returned. A leftover temp dir does not break the volume, and failing a publish that otherwise worked only makes kubelet retry it.

A new unit test publishes an NFS volume through the fake mounter, where the removal always fails, and checks that a warning naming the dir is logged.

This is independent of #4451. That PR touches the same Go module but not node.go.

A failed unmount still leaves the NFS root mounted. That is older than this change and is tracked in #4561.

Closes #4466

Screenshots

No UI change.

Downstream repositories

Release note

fix(kubernetes): the kubevirt-csi node plugin now logs a warning when it cannot remove its temporary NFS mount dir instead of dropping the error

Summary by CodeRabbit

  • Bug Fixes
    • Failed cleanup of a temporary mount directory now produces a warning with the directory path, making the issue easier to identify. The volume publish operation continues to succeed when cleanup fails.

The NFS publish path temp-mounts the export root to create /data and
migrate old files, then removes the temp dir in a defer that dropped
the error. When the unmount before it fails, the removal fails on a
busy mount point and the leftover dir under the node's temp dir went
unreported.

The failure is logged rather than returned: a leftover temp dir does
not make the volume unusable, and failing an otherwise good publish
would only make kubelet retry it.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <[email protected]>
@github-actions github-actions Bot added size/M This PR changes 30-99 lines, ignoring generated files area/kubernetes Issues or PRs related to the tenant Kubernetes app kind/bug Categorizes issue or PR as related to a bug labels Sep 29, 2026
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: d1c63bd7-e821-450b-a507-8db6bb672919

📥 Commits

Reviewing files that changed from the base of the PR and between b314514 and 3e91da9.

📒 Files selected for processing (2)
  • packages/apps/kubernetes/images/kubevirt-csi-driver/node.go
  • packages/apps/kubernetes/images/kubevirt-csi-driver/node_test.go

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

NodePublishVolume now logs a warning if it cannot remove its temporary mount directory. A test checks the warning and confirms that the publish call still succeeds.

Changes

NFS temporary mount cleanup

Layer / File(s) Summary
Cleanup error logging and test
packages/apps/kubernetes/images/kubevirt-csi-driver/node.go, packages/apps/kubernetes/images/kubevirt-csi-driver/node_test.go
Deferred cleanup logs a warning when temporary directory removal fails. The test checks for the warning and confirms that NodePublishVolume returns no error.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 3e91d

This change adds a warning when the temporary NFS mount directory cannot be removed. Publishing behaves as before. Merge risk is minimal.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: logging kubevirt-csi temporary NFS mount directory removal failures.
Linked Issues check ✅ Passed The change satisfies #4466. NodePublishVolume now checks os.Remove(tmpMount) in a deferred function and logs a warning with the temporary directory and error. The publish result remains unchanged.…
Out of Scope Changes check ✅ Passed The changed files contain only the cleanup-error logging required by #4466 and its focused unit test. No unrelated production behavior or repository changes are present in the reviewed diff.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

Clean, well-scoped fix. The temp-dir removal error used to be dropped (defer os.Remove(tmpMount)); now it is logged with the path and the error, the same log-not-return treatment the unmount defer right below it already uses. That asymmetry is the right call: migration failure returns because it can hide user data, while a leftover temp dir cannot break the volume, and returning would only make kubelet retry a publish that otherwise worked. The comment says as much.

A couple of things made me confident the fix is sound and not just plausible.

  • The warning stands on its own: the temp dir name embeds the volume id (nfs-init-<volumeID>-...), so the logged path already identifies the volume, no separate field needed.
  • Defer order holds up. The unmount defer is registered after the remove defer, so LIFO unmounts first and removes second, which is the only order where removal can succeed on a real mount.

The test holds the behavior rather than the happy path: revert to a bare defer os.Remove(tmpMount) and it fails. Build, vet and the full package test pass, including against current main merged into the head.

Nothing blocking. The mount leak on a failed unmount is older than this change and already tracked in #4561.

@IvanHunters
IvanHunters merged commit c7b8009 into main Oct 5, 2026
20 checks passed
@IvanHunters
IvanHunters deleted the fix/kubevirt-csi-nfs-tmpdir-error branch October 5, 2026 11:40
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Oct 7, 2026
…out (#4584)

<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->

## What this PR does

The kubevirt-csi node plugin now unmounts its temporary NFS mount with a
timeout, and retries with `umount -f` when the timeout runs out.

For an NFS publish the plugin mounts the export root into an
`nfs-init-*` temp dir to create `/data`. It unmounted that dir in a
defer with a plain `umount`, which has no deadline. Before this change,
if the NFS server was gone by then, the publish could hang on that
`umount` forever and the temp mount stayed.

The force-unmount helper in `k8s.io/mount-utils` doesn't work for this
case. `UnmountWithForce` retries with `-f` only after its first `umount`
returns. That `umount` forks the `umount.nfs` helper. When the timeout
kills `umount`, the helper stays stuck on the dead server and keeps the
output pipe open, and `CombinedOutput` waits for that pipe, so the `-f`
retry never runs. The plugin now runs `umount` itself with `WaitDelay`
set, so the pipe is closed a second after the kill, and then runs
`umount -f` under its own timeout. A "not mounted" answer counts as
done, same as in mount-utils. `umount2` detaches the mount before it
blocks on tearing down the superblock, so the `-f` may find nothing
left.

Some umounts can't be killed at all, for example one stuck in
uninterruptible sleep. So the whole cleanup also waits at most 30s, well
inside the 2m deadline kubelet sets for NodePublishVolume (`csiTimeout`
in `pkg/volume/csi/csi_plugin.go`). Once the budget runs out the publish
goes on, and the unmount keeps running in the background and logs its
result when it ends. The temp dir is removed by that background unmount
once it succeeds, so an unmount that finishes late still cleans up after
itself. A mounter other than the exec-based one keeps its plain
`Unmount`, under the same budget.

The temp mount now uses `nosharecache`. A forced unmount runs
`nfs_umount_begin`, which kills every RPC in flight on the superblock,
and NFS shares a superblock between mounts of the same export with the
same options. Without this option, `-f` on the temp mount could fail I/O
on the pod mount this same publish creates, and on other pods on the
node that mount the same volume. The cost is a separate cache: pods that
already have `/data` mounted may not see what the migration moves in
until their attribute cache expires. The migration is a one-time
backfill, so I'm fine with that.

The leak only lives until the container restarts, because `/tmp` in the
node plugin container has no hostPath and the mounts go away with the
container's mount namespace. So this change stops leaked mounts from
piling up inside one container. For the same reason a sweep of
`nfs-init-*` dirs at startup would never find anything, so I did not add
one.

Two cases stay as they were. An `umount` that fails fast, for example
with `target is busy`, gets no force retry, is logged, and the dir stays
because `rmdir` refuses a mount point. And since the temp mount is a
hard mount, a server that disappears before the unmount still hangs the
mount and migration calls earlier in this path. That second case is
#4581.

The tests run the real exec-based mounter against a fake `umount` on
`PATH`. The fake covers a plain unmount that succeeds, one that fails
fast and gets no `-f`, and one that hangs while an orphaned child holds
its output pipe. The last one has to end in `umount -f` inside the
budget. A "not mounted" from `-f` counts as done, and any other `-f`
failure is reported and keeps the dir. Fake mounters cover the
background path: giving up on a stalled unmount, removing the dir after
a late unmount, and logging the late result. Other tests check
`nosharecache` on the temp mount and the temp dir after a failed mount.
I haven't reproduced this against a real NFS server that went away, so
it is not checked whether `umount -f` in the almalinux image gets as far
as `umount2(MNT_FORCE)` there. The tests are in a new file, so this PR
does not overlap with #4562.

Closes #4561

### Screenshots

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

No UI change.

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
- [ ] [cozystack/community](https://github.com/cozystack/community) -
follow-up:

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
fix(kubernetes): the kubevirt-csi node plugin no longer hangs an NFS volume publish forever when unmounting its temporary mount after the NFS server went away; the unmount now times out and is retried with force
```




<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Temporary NFS mounts use isolated caching to reduce interference with
pod mounts.
* Cleanup waits up to 30 seconds for unmounting. If the limit is
reached, the publish operation returns an error while unmounting
continues in the background.
* Temporary directories are removed after a successful unmount or a
failed mount, and retained if unmounting fails.
* When a standard unmount times out, cleanup retries with a forced
unmount when available.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/kubernetes Issues or PRs related to the tenant Kubernetes app kind/bug Categorizes issue or PR as related to a bug size/M This PR changes 30-99 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kubevirt-csi node: temp NFS mount dir removal error is ignored

2 participants