fix(linstor): wait for temporary probe devices - #4100
Aleksei Sviridkin (lexfrei) merged 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe patch adds synchronized probe-volume retries, device-wait handling, interrupt preservation, cleanup safeguards, and restricted fallback probing. Tests cover these paths, including inactive shared LVM volumes. Documentation describes the updated behavior. ChangesMinimum-I/O Probe Device Handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AbsStorageProvider
participant ProbeVolume
participant DevicePath
AbsStorageProvider->>ProbeVolume: create temporary probe volume
ProbeVolume->>DevicePath: expose device path
AbsStorageProvider->>DevicePath: wait for device availability
DevicePath-->>AbsStorageProvider: usable device
AbsStorageProvider->>AbsStorageProvider: update minimum I/O properties
AbsStorageProvider->>ProbeVolume: clean up probe volume
Merge Risk: ⚪ Minimal · up to The change safely improves ZFS probe-device discovery and is ready to merge with no identified blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
f69aa8e to
fc66985
Compare
scooby87
left a comment
There was a problem hiding this comment.
LGTM (independent cozy-review pass).
Vendored Java patch to piraeus-server, origin = upstream LINBIT PR #528 / fixes #527. It applies cleanly alongside the other five patches (including the sibling luks patch that touches adjacent lines in the same file) and the result is structurally sound. The wait loop is bounded — attempts < PROBE_VLM_ATTEMPTS (3), waitUntilDeviceCreated uses a bounded device-visible timeout that throws on expiry, the 1s retry delay is interruptible, cleanup runs in a finally each iteration, worst case ~17s — no unbounded busy-wait. The LVM path is explicitly preserved (probe only on empty list or ZFS/ZFS_THIN), and a new null-path guard fixes a latent NPE.
Non-blocking: the bundled JUnit test never compiles/runs in the cozystack image build (gradle installdist has no test task), so there is no automatic regression signal here — recommend a live validation on a 4K ZFS pool.
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. Checked the committed diff against LINBIT/linstor-server#528 hunk by hunk and applied the whole patches directory to a clean v1.33.3 checkout. All six apply in the order the build globs them, this one lands at its exact offsets, no fuzz.
Business context: LINSTOR reads a ZFS probe volume's block device properties before udev publishes the device node, so 4K pools end up with no min/opt I/O size properties and replica placement fails.
Most of what differs from #528 is forced by 1.33.3. The dropped CHANGELOG.md, javadoc and LvmProviderBlockDeviceInfoTest.java hunks have nothing to attach to at this tag. The null-path guard is missing from #528 because upstream already had it, and it is needed here or haveInfo goes true on a null path and the ZFS fallback never runs. The ZFS restriction has a better reason than the README gives: upstream can probe every provider because it tries getReadOnlyProbeDevice() first, and 1.33.3 has no such method, so an unrestricted fallback would create a volume on an inactive shared LVM volume owned by another node. Two deviations that are not forced are in the list below.
No JDK 11 or Gradle on my side, so the 1026 tests, assemble and "14 of 17 fail without the patch" stay unverified. The 17 regression tests are in the diff. DrbdAdm.java:970 is the for loop in secondaryAfterMkfs, which retry-secondary-after-mkfs.diff adds and which returns from inside the loop, so that attribution holds. Nothing in CI builds this image, the four green checks are labels, size, DCO and the bot.
Four things for later, none of them blocking:
- The new test reads
BlockSizeConsts.DFLT_OPT_IO_SIZE, whichfix-luks-header-size.diffadds. It compiles today only because the glob sorts luks first. Drop luks after some future bump and this test breaks with a missing symbol nowhere near the file that went away, so the coupling is worth a line in the README entry. - Upstream gates cleanup on a
createdflag and gives up after a failed creation. Here delete always runs and the loop retries. With the real provider it never gets that far:ZfsCommands.deleteshells out tozfs destroywith only a device-busy retry handler, so destroying a volume that was never created throws,cleanedstays false and the loop breaks on the first pass. Costs an extrazfs destroyand an extra warning.failedCreationStillCleansEveryAttemptandtransientCreationFailureIsRetriedpin behaviour only the mock produces. interruptedlooks atexc.getCause()only. Upstream also checksThread.currentThread().isInterrupted()and has a test for it that this patch drops. The device wait wrapsInterruptedExceptionas the cause so the common path is covered, but an interrupt during the block size read costs an extra iteration and the final warning then blames the interrupted delay instead of the real failure.- No removal note. #528 merged as
6e55578and v1.35.1 contains that commit, so "drop when the image moves to LINSTOR 1.35.1 or newer" is nameable. It is also the only entry with no backported commit, and it sits afterretry-secondary-after-mkfsinstead of the alphabetical order the rest of the file and the glob both follow.
fc66985 to
93b46b0
Compare
Signed-off-by: Yan Bondarenko <[email protected]>
93b46b0 to
cee9065
Compare
|
Successfully created backport PR for |
What this PR does
Backports LINBIT/linstor-server#528 to the LINSTOR 1.33.3 build used by piraeus-server. A ZFS probe volume can be created before udev publishes its device path, leaving the pool without I/O properties and blocking replica placement on 4K pools.
Wait for the probe device before reading its properties, retry up to three times and report failures. Skip null paths in the existing volume list and fall back to a probe when ZFS has no usable path. Serialize probe creation and cleanup, stop after cleanup failures and preserve interrupts. Inactive LVM volumes retain the existing no-probe behavior.
Validation: all six patches apply together to v1.33.3. All 1026 tests and
assemblepass on Linux amd64 with JDK 11, including 17 regression tests; 14 of those fail without this patch.checkstyleMainreports the same existing return-inside-for error atDrbdAdm.java:970fromretry-secondary-after-mkfs.diff, reproduced without this patch; no new findings.Downstream repositories
Release note
Summary by CodeRabbit
Bug Fixes
Documentation