Repository navigation
fix(linstor): retry drbdadm secondary after mkfs on transient EBUSY - #2538
Conversation
LINSTOR satellite aborts resource initialization with "Device is held open by someone" (errno -12) when an external probe (Talos block-controller, udev, multipathd) opens the freshly-promoted DRBD device just before the post-mkfs demotion. The controller view then stays in SyncTarget / Inconsistent / DELETING indefinitely, blocking cleanup of orphan PVs in Released phase. Add a retry wrapper secondaryAfterMkfs() with exponential backoff (7 attempts, 250ms -> 2s) that triggers only on this specific error message. DrbdPrimary.close() now uses the wrapper so the retry only applies to the post-mkfs demotion path; the existing public secondary() contract is untouched. Related: LINBIT/linstor-server#268, LINBIT/drbd#74 Signed-off-by: IvanHunters <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a patch for piraeus-server that implements retry logic with exponential backoff for Changes
Sequence DiagramsequenceDiagram
actor Client as Client Code
participant DrbdAdm as DrbdAdm Service
participant Cmd as drbdadm Command
participant Device as Device/FS
Client->>DrbdAdm: secondaryAfterMkfs(drbdRscData)
loop Retry with Exponential Backoff
DrbdAdm->>Cmd: execute drbdadm secondary
Cmd->>Device: attempt state change
alt Device Available
Device-->>Cmd: success
Cmd-->>DrbdAdm: return 0
DrbdAdm-->>Client: success
else Device Held Open
Device-->>Cmd: error (device held)
Cmd-->>DrbdAdm: stderr with error message
DrbdAdm->>DrbdAdm: detect device held open error
DrbdAdm->>DrbdAdm: wait(exponential backoff)
Note over DrbdAdm: Retry or fail after limit
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
github-actions
Bot
added
area/storage
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
dosubot
Bot
added
the
size/L
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a race condition in LINSTOR where DRBD device initialization fails because external system probes hold a file descriptor open during the post-mkfs demotion process. By implementing a targeted retry strategy with exponential backoff, the change ensures that transient 'EBUSY' errors do not prevent the satellite from reporting the correct state to the controller, thereby preventing the accumulation of orphan persistent volumes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the documentation for piraeus-server patches by adding a description for a new patch, retry-secondary-after-mkfs.diff, which addresses resource initialization failures caused by external probes. However, the actual patch file referenced in the README is missing from the pull request and must be added to the patches directory.
| - **retry-adjust-after-stale-bitmap.diff** — Retry `drbdadm adjust` after detaching a stale local bitmap state | ||
| - Source PR: [#491](https://github.com/LINBIT/linstor-server/pull/491) | ||
| - Backported from commit: [`51ae50a84`](https://github.com/kvaps/linstor-server/commit/51ae50a84dcb98093f543b819652c750a94d96c9) | ||
| - **retry-secondary-after-mkfs.diff** — Retry `drbdadm secondary` after mkfs when DRBD reports the device as held open by an external probe (Talos block-controller, udev, multipathd, etc.). Without this retry, a transient `Device is held open by someone` aborts resource initialization, leaves the satellite in an intermediate state, and prevents the controller from receiving the final `UpToDate` event — orphan PVs in `Released` then cannot be cleaned up. |
There was a problem hiding this comment.
The pull request description mentions adding a fifth patch file named retry-secondary-after-mkfs.diff, and this README update references it. However, the actual .diff file is missing from the changes in this pull request. Please ensure the patch file is added to the packages/system/linstor/images/piraeus-server/patches/ directory so it can be applied during the image build.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Patch is well-scoped: rerouting only DrbdPrimary.close() to a new wrapper keeps the existing secondary() contract intact, fail-fast behavior is preserved for any non-EBUSY error, and the retry window (~7.75s of cumulative sleep) is bounded. Stacking onto retry-adjust-after-stale-bitmap.diff works correctly — line numbers and required imports (StandardCharsets, File, Platform, ProcessBuilder.Redirect, IOException, ChildProcessTimeoutException) line up. README entry follows the established format and links the relevant upstream issues.
LGTM.
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM — patch is minimal and well-scoped.
- Existing
secondary()contract is preserved; onlyDrbdPrimary.close()(the post-mkfs demotion path) is rerouted to the retrying wrapper. isDeviceHeldOpenError()gates the retry; any other failure throws fail-fast as before.- Backoff caps at 2 s, total worst case ≈ 7.75 s of sleep, comfortably above typical probe windows.
- InterruptedException correctly restores the interrupt flag.
Holding off on merge until E2E goes green.
myasnikovdaniil
added
the
kind/backport
|
Successfully created backport PR for |
4 participants
What this PR does
Adds a fifth piraeus-server patch that retries
drbdadm secondaryaftermkfswhen DRBD reports the device as held open by an external process.Problem
On Talos (and any system with active block-device probing — udev, multipathd, etc.) the kernel uevent for a freshly-promoted DRBD device causes an external probe to
open()the device for superblock inspection. If LINSTOR satellite issuesdrbdadm secondarywhile the probe still holds the fd, DRBD returns(-12) Device is held open by someoneand aborts initialization withStorageException: Failed to become secondary again after creating filesystem.The satellite never reports the final
UpToDateevent to the controller, so the controller view stays inSyncTarget(N%) / Inconsistent / DELETINGpermanently — even though the DRBD kernel state on the satellite isEstablished/UpToDate. This blocks cleanup of orphan PVs inReleasedphase and accumulates with every PVC creation.Observed in production on Talos 1.10 / LINSTOR 1.33.2: 4 occurrences in 24 hours across 3 nodes for different PVCs. Manual recovery requires
drbdadm down/upon the Primary to force the satellite to re-report state.Fix
New
secondaryAfterMkfs()wrapper inDrbdAdm.java:drbdadm secondarywith exponential backoff (250 ms → 2 s, capped)Device is held open by someone; any other failure fails fast as beforesecondary()contract is unchanged — onlyDrbdPrimary.close()(the post-mkfs demotion path) is rerouted to the new wrapperThe retry window covers ~10 s in the worst case, which comfortably exceeds typical probe durations (probes release the fd within milliseconds).
Related upstream
Validation
git apply *.diffon a freshv1.33.2checkout applies all five patches cleanly in alphabetical orderhelm template packages/system/linstorrenders without errorsRelease note
Summary by CodeRabbit
New Features
Documentation