Skip to content

fix(linstor): wait for temporary probe devices - #4100

Merged
Aleksei Sviridkin (lexfrei) merged 1 commit into
cozystack:mainfrom
yankawai:fix/linstor-probe-device-wait
Sep 18, 2026
Merged

Aleksei Sviridkin (lexfrei) merged 1 commit into
cozystack:mainfrom
yankawai:fix/linstor-probe-device-wait

Conversation

@yankawai

@yankawai europrinter (yankawai) commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

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 assemble pass on Linux amd64 with JDK 11, including 17 regression tests; 14 of those fail without this patch. checkstyleMain reports the same existing return-inside-for error at DrbdAdm.java:970 from retry-secondary-after-mkfs.diff, reproduced without this patch; no new findings.

Downstream repositories

Release note

fix(linstor): wait for ZFS probe devices before reading pool I/O properties to avoid incorrect block sizes and failed replica placement

Summary by CodeRabbit

  • Bug Fixes

    • Improved storage device probing by waiting for temporary devices to become available and retrying when necessary.
    • Improved compatibility with ZFS volumes when standard device paths are unavailable.
    • Prevented unnecessary probing of inactive shared LVM volumes.
    • Improved handling of interrupted probes and cleanup failures.
  • Documentation

    • Added documentation describing storage probing behavior and compatibility updates.

@github-actions github-actions Bot added area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/bug Categorizes issue or PR as related to a bug size/XL This PR changes 500-999 lines, ignoring generated files labels Sep 6, 2026
@coderabbitai

coderabbitai Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: f7cb29a5-df86-4247-92f5-ff719ddcbb45

📥 Commits

Reviewing files that changed from the base of the PR and between 86d62bb and f69aa8e.

📒 Files selected for processing (2)
  • packages/system/linstor/images/piraeus-server/patches/README.md
  • packages/system/linstor/images/piraeus-server/patches/fix-min-io-probe-device-wait.diff

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Minimum-I/O Probe Device Handling

Layer / File(s) Summary
Probe retry and fallback flow
packages/system/linstor/images/piraeus-server/patches/fix-min-io-probe-device-wait.diff
AbsStorageProvider waits for probe devices, retries transient failures up to three times, preserves interrupts, stops after cleanup failures, and limits fallback probing for inactive shared LVM volumes.
Probe behavior validation and documentation
packages/system/linstor/images/piraeus-server/patches/fix-min-io-probe-device-wait.diff, packages/system/linstor/images/piraeus-server/patches/README.md
ProbeVolumeDeviceWaitTest covers success, retries, failures, interrupts, cleanup, concurrency, and LVM behavior. The README documents the patch.

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
Loading

Merge Risk: ⚪ Minimal · up to f69aa

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)
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: LINSTOR now waits for temporary probe devices.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@scooby87 scooby87 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 (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.

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. 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:

  1. The new test reads BlockSizeConsts.DFLT_OPT_IO_SIZE, which fix-luks-header-size.diff adds. 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.
  2. Upstream gates cleanup on a created flag 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.delete shells out to zfs destroy with only a device-busy retry handler, so destroying a volume that was never created throws, cleaned stays false and the loop breaks on the first pass. Costs an extra zfs destroy and an extra warning. failedCreationStillCleansEveryAttempt and transientCreationFailureIsRetried pin behaviour only the mock produces.
  3. interrupted looks at exc.getCause() only. Upstream also checks Thread.currentThread().isInterrupted() and has a test for it that this patch drops. The device wait wraps InterruptedException as 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.
  4. No removal note. #528 merged as 6e55578 and 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 after retry-secondary-after-mkfs instead of the alphabetical order the rest of the file and the glob both follow.

@lexfrei Aleksei Sviridkin (lexfrei) added the kind/backport Categorizes issue or PR as requiring a backport to the current release line label Sep 18, 2026
@lexfrei
Aleksei Sviridkin (lexfrei) merged commit d71e78a into cozystack:main Sep 18, 2026
16 checks passed
@github-actions

Copy link
Copy Markdown

Successfully created backport PR for release-1.6:

myasnikovdaniil added a commit that referenced this pull request Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/backport Categorizes issue or PR as requiring a backport to the current release line kind/bug Categorizes issue or PR as related to a bug size/XL This PR changes 500-999 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants