Repository navigation
linstor: update piraeus-server to v1.33.2 with selected backports - #2331
Conversation
dosubot
Bot
added
the
size/XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLINSTOR version bumped from 1.32.3 to 1.33.2; the piraeus-server patchset and README were updated — three prior patches removed, several patches rewritten or added, including toggle-disk control-flow refactor with retry, DRBD TCP port preservation, LUKS2 header/optimal I/O sizing, and DRBD attach retry for stale bitmaps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/API
participant Ctrl as Controller (ToggleDiskHandler)
participant Layer as LayerDataFactory
participant DRBD as DRBD Layer
participant Storage as Storage/Volumes
Client->>Ctrl: ToggleDisk request (add/remove)
Ctrl->>Ctrl: determineToggleDiskAction(rsc, removeDisk)
alt NOOP
Ctrl->>Client: respond NOOP
else RETRY
Ctrl->>Ctrl: handleRetryAction()
Ctrl->>Layer: copyDrbdSettings / copyDrbdTcpPortsIfExists
Ctrl->>DRBD: prepare payload & rebuild layer data
DRBD->>Storage: ensureStackDataExists (conditional)
Ctrl->>Client: respond RETRY/UPDATED
else ABORT
Ctrl->>Ctrl: clearToggleDiskFlags()
Ctrl->>Client: respond ABORTED
else NORMAL
Ctrl->>DRBD: proceed with normal toggle-disk flow
DRBD->>Storage: adjust/regenerate/resync as needed
Ctrl->>Client: respond OK
end
sequenceDiagram
participant DrbdAdm as DrbdAdm
participant OS as OS/Process
participant Drbdsetup as drbdsetup
DrbdAdm->>OS: exec ["drbdadm","attach",...] (piped)
OS-->>DrbdAdm: exitCode != 0 + stderr
alt stderr contains "bitmap leak" with minor N
DrbdAdm->>Drbdsetup: exec ["drbdsetup","detach",N]
Drbdsetup-->>DrbdAdm: success/fail
alt detach success
DrbdAdm->>OS: retry drbdadm attach ...
OS-->>DrbdAdm: exitCode
alt success
DrbdAdm->>Caller: success
else
DrbdAdm->>Caller: throw ExtCmdFailedException
end
else
DrbdAdm->>Drbdsetup: exec ["drbdsetup","detach",N,"--force"]
Drbdsetup-->>DrbdAdm: result
DrbdAdm->>OS: retry attach...
OS-->>DrbdAdm: exitCode
DrbdAdm->>Caller: success or throw ExtCmdFailedException
end
else
DrbdAdm->>Caller: throw ExtCmdFailedException
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. Comment |
daade7e to
36636d5
Compare
![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 LINSTOR to version 1.33.1 and refines the Piraeus server patches. Key changes include backporting maintainer implementations for toggle-disk retry and abort operations, preserving DRBD TCP ports during disk toggling, and introducing a new patch for consistent LUKS2 header sizing. Several obsolete patches were removed. Feedback was provided regarding the image tags in the values configuration, suggesting the use of specific version prefixes instead of "latest" to improve traceability and clarity.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/system/linstor/values.yaml (4)
The image tag for piraeusServer now uses latest as the version prefix. While the image is pinned by its digest, using the specific version 1.33.1 (as defined in the Makefile) in the tag, like 1.33.1@sha256:..., improves clarity and traceability. This makes it immediately obvious which version of linstor-server is being deployed.
tag: 1.33.1@sha256:3cb79ad7260be81223e6490e429c05be35fa15de2e4f93c7ef78a769ed9989aapackages/system/linstor/values.yaml (16)
The image tag for linstorCSI has been changed to use latest as a prefix, while the image digest remains the same. This implies the image itself was not intended to be updated. To avoid confusion and clearly indicate that the linstor-csi version is still v1.10.5, it's recommended to keep the versioned tag.
tag: v1.10.5@sha256:e80ae94d8acd8774cb35715b1f103071ae6cd3160f1ebea915e5e6ee49234448![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/system/linstor/Makefile (1)
7-25:⚠️ Potential issue | 🟠 MajorCommit the regenerated
values.yamlwith this version bump.The provided
packages/system/linstor/values.yaml:1-4snippet still pinspiraeusServer.image.tagto1.32.3@sha256:d78071fdc33220168e3f2ab112a86208be95898b75268a0c62763b8a04e145ad. Because this Makefile only rewritesvalues.yamlwhenimage-piraeus-serveris run locally, merging this change as-is would leave the checked-in chart on the old server image.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/linstor/Makefile` around lines 7 - 25, The values.yaml checked into the repo still pins piraeusServer.image.tag to the old digest; run the image-piraeus-server Makefile target (or manually apply its changes) to update piraeusServer.image.repository and piraeusServer.image.tag to the newly built image (the Makefile sets REPOSITORY="$(REGISTRY)/piraeus-server" and TAG="$(call settag,$(LINSTOR_VERSION))@$(yq e '."containerimage.digest"' images/piraeus-server.json -o json -r)"), then commit the regenerated packages/system/linstor/values.yaml so the chart matches the bumped LINSTOR_VERSION and image built by image-piraeus-server.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/system/linstor/images/piraeus-server/patches/allow-toggle-disk-retry.diff`:
- Around line 63-64: The RETRY branch bypasses precondition checks by returning
handleRetryAction(rsc, removeDisk, toggleIntoTiebreakerRef, context) before
validateToggleDiskPreconditions(rsc, removeDisk) runs; modify the switch so that
validateToggleDiskPreconditions(rsc, removeDisk) is invoked for the RETRY path
(same as other branches) and only after it succeeds call handleRetryAction(...).
Ensure you update both occurrences of the RETRY case to call
validateToggleDiskPreconditions first and then proceed to handleRetryAction with
the same arguments (rsc, removeDisk, toggleIntoTiebreakerRef, context).
---
Outside diff comments:
In `@packages/system/linstor/Makefile`:
- Around line 7-25: The values.yaml checked into the repo still pins
piraeusServer.image.tag to the old digest; run the image-piraeus-server Makefile
target (or manually apply its changes) to update piraeusServer.image.repository
and piraeusServer.image.tag to the newly built image (the Makefile sets
REPOSITORY="$(REGISTRY)/piraeus-server" and TAG="$(call
settag,$(LINSTOR_VERSION))@$(yq e '."containerimage.digest"'
images/piraeus-server.json -o json -r)"), then commit the regenerated
packages/system/linstor/values.yaml so the chart matches the bumped
LINSTOR_VERSION and image built by image-piraeus-server.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af38b719-9ac1-45de-a2c3-2090d02b303b
📒 Files selected for processing (8)
packages/system/linstor/Makefilepackages/system/linstor/images/piraeus-server/patches/README.mdpackages/system/linstor/images/piraeus-server/patches/adjust-on-resfile-change.diffpackages/system/linstor/images/piraeus-server/patches/allow-toggle-disk-retry.diffpackages/system/linstor/images/piraeus-server/patches/fix-duplicate-tcp-ports.diffpackages/system/linstor/images/piraeus-server/patches/fix-luks-header-size.diffpackages/system/linstor/images/piraeus-server/patches/force-metadata-check-on-disk-add.diffpackages/system/linstor/images/piraeus-server/patches/skip-adjust-when-device-inaccessible.diff
💤 Files with no reviewable changes (3)
- packages/system/linstor/images/piraeus-server/patches/adjust-on-resfile-change.diff
- packages/system/linstor/images/piraeus-server/patches/force-metadata-check-on-disk-add.diff
- packages/system/linstor/images/piraeus-server/patches/skip-adjust-when-device-inaccessible.diff
Signed-off-by: Andrei Kvapil <[email protected]>
Signed-off-by: Andrei Kvapil <[email protected]>
3bed6e6 to
fd6bae6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/system/linstor/images/piraeus-server/patches/retry-adjust-after-stale-bitmap.diff`:
- Around line 128-136: The catch block that handles InterruptedException should
stop the retry/backoff and not return true; after restoring the interrupt flag
(Thread.currentThread().interrupt()) return false so the caller does not
immediately perform another attach and the backoff is respected; update the
catch in the sleep around FORCE_DETACH_RETRY_WAIT_MS to return false instead of
falling through to the existing return true.
- Around line 75-89: The function extractBitmapLeakMinor currently checks for
BITMAP_LEAK_ERR_MSG anywhere in stderr then applies BITMAP_LEAK_MINOR_PATTERN to
the whole stderr, which can pick up an unrelated earlier "minor N"; change it to
first locate the specific stderr line (or substring) that contains
BITMAP_LEAK_ERR_MSG and then run BITMAP_LEAK_MINOR_PATTERN.matcher(...) only
against that line so the returned Integer (via
Integer.parseInt(matcher.group(1))) is the minor from the same bitmap-leak
message used by cleanupStaleBitmapAndRetry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce081070-cfe9-4d20-abba-d003988f1a63
📒 Files selected for processing (2)
packages/system/linstor/images/piraeus-server/patches/README.mdpackages/system/linstor/images/piraeus-server/patches/retry-adjust-after-stale-bitmap.diff
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/system/linstor/images/piraeus-server/patches/README.md
| + static @Nullable Integer extractBitmapLeakMinor(OutputData outputData) | ||
| + { | ||
| + String stderr = new String(outputData.stderrData, StandardCharsets.UTF_8); | ||
| + if (!stderr.contains(BITMAP_LEAK_ERR_MSG)) | ||
| + { | ||
| + return null; | ||
| + } | ||
| + | ||
| + Matcher matcher = BITMAP_LEAK_MINOR_PATTERN.matcher(stderr); | ||
| + if (!matcher.find()) | ||
| + { | ||
| + return null; | ||
| + } | ||
| + | ||
| + return Integer.parseInt(matcher.group(1)); |
There was a problem hiding this comment.
Parse the minor from the same bitmap-leak line.
extractBitmapLeakMinor() first checks for BITMAP_LEAK_ERR_MSG anywhere in stderr, then returns the first minor N anywhere in stderr. If DRBD prints another minor earlier in the same stderr blob, cleanupStaleBitmapAndRetry() can detach the wrong device.
Suggested fix
- Matcher matcher = BITMAP_LEAK_MINOR_PATTERN.matcher(stderr);
- if (!matcher.find())
- {
- return null;
- }
-
- return Integer.parseInt(matcher.group(1));
+ for (String line : stderr.split("\\R"))
+ {
+ if (!line.contains(BITMAP_LEAK_ERR_MSG))
+ {
+ continue;
+ }
+
+ Matcher matcher = BITMAP_LEAK_MINOR_PATTERN.matcher(line);
+ if (matcher.find())
+ {
+ return Integer.parseInt(matcher.group(1));
+ }
+ }
+ return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + static @Nullable Integer extractBitmapLeakMinor(OutputData outputData) | |
| + { | |
| + String stderr = new String(outputData.stderrData, StandardCharsets.UTF_8); | |
| + if (!stderr.contains(BITMAP_LEAK_ERR_MSG)) | |
| + { | |
| + return null; | |
| + } | |
| + | |
| + Matcher matcher = BITMAP_LEAK_MINOR_PATTERN.matcher(stderr); | |
| + if (!matcher.find()) | |
| + { | |
| + return null; | |
| + } | |
| + | |
| + return Integer.parseInt(matcher.group(1)); | |
| static `@Nullable` Integer extractBitmapLeakMinor(OutputData outputData) | |
| { | |
| String stderr = new String(outputData.stderrData, StandardCharsets.UTF_8); | |
| if (!stderr.contains(BITMAP_LEAK_ERR_MSG)) | |
| { | |
| return null; | |
| } | |
| for (String line : stderr.split("\\R")) | |
| { | |
| if (!line.contains(BITMAP_LEAK_ERR_MSG)) | |
| { | |
| continue; | |
| } | |
| Matcher matcher = BITMAP_LEAK_MINOR_PATTERN.matcher(line); | |
| if (matcher.find()) | |
| { | |
| return Integer.parseInt(matcher.group(1)); | |
| } | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/linstor/images/piraeus-server/patches/retry-adjust-after-stale-bitmap.diff`
around lines 75 - 89, The function extractBitmapLeakMinor currently checks for
BITMAP_LEAK_ERR_MSG anywhere in stderr then applies BITMAP_LEAK_MINOR_PATTERN to
the whole stderr, which can pick up an unrelated earlier "minor N"; change it to
first locate the specific stderr line (or substring) that contains
BITMAP_LEAK_ERR_MSG and then run BITMAP_LEAK_MINOR_PATTERN.matcher(...) only
against that line so the returned Integer (via
Integer.parseInt(matcher.group(1))) is the minor from the same bitmap-leak
message used by cleanupStaleBitmapAndRetry.
| + try | ||
| + { | ||
| + Thread.sleep(FORCE_DETACH_RETRY_WAIT_MS); | ||
| + } | ||
| + catch (InterruptedException ignored) | ||
| + { | ||
| + Thread.currentThread().interrupt(); | ||
| + } | ||
| + return true; |
There was a problem hiding this comment.
Stop the retry when the wait is interrupted.
Lines 130-135 restore the interrupt flag but still return true, so the caller immediately runs another attach. That skips the intended backoff and continues a state-changing operation on an interrupted thread.
Suggested fix
catch (InterruptedException ignored)
{
Thread.currentThread().interrupt();
+ return false;
}
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + try | |
| + { | |
| + Thread.sleep(FORCE_DETACH_RETRY_WAIT_MS); | |
| + } | |
| + catch (InterruptedException ignored) | |
| + { | |
| + Thread.currentThread().interrupt(); | |
| + } | |
| + return true; | |
| try | |
| { | |
| Thread.sleep(FORCE_DETACH_RETRY_WAIT_MS); | |
| } | |
| catch (InterruptedException ignored) | |
| { | |
| Thread.currentThread().interrupt(); | |
| return false; | |
| } | |
| return true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/linstor/images/piraeus-server/patches/retry-adjust-after-stale-bitmap.diff`
around lines 128 - 136, The catch block that handles InterruptedException should
stop the retry/backoff and not return true; after restoring the interrupt flag
(Thread.currentThread().interrupt()) return false so the caller does not
immediately perform another attach and the backoff is respected; update the
catch in the sleep around FORCE_DETACH_RETRY_WAIT_MS to return false instead of
falling through to the existing return true.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/system/linstor/images/piraeus-server/patches/retry-adjust-after-stale-bitmap.diff (2)
75-89:⚠️ Potential issue | 🔴 CriticalExtract the minor from the same bitmap-leak line.
Line 78 scopes the error text, but Lines 83-89 still scan the full
stderrblob for the firstminor N. If another minor appears earlier in the output,cleanupStaleBitmapAndRetry()can detach the wrong device.🔧 Suggested fix
static `@Nullable` Integer extractBitmapLeakMinor(OutputData outputData) { String stderr = new String(outputData.stderrData, StandardCharsets.UTF_8); - if (!stderr.contains(BITMAP_LEAK_ERR_MSG)) - { - return null; - } - - Matcher matcher = BITMAP_LEAK_MINOR_PATTERN.matcher(stderr); - if (!matcher.find()) + for (String line : stderr.split("\\R")) { - return null; + if (!line.contains(BITMAP_LEAK_ERR_MSG)) + { + continue; + } + + Matcher matcher = BITMAP_LEAK_MINOR_PATTERN.matcher(line); + if (matcher.find()) + { + return Integer.parseInt(matcher.group(1)); + } } - - return Integer.parseInt(matcher.group(1)); + return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/linstor/images/piraeus-server/patches/retry-adjust-after-stale-bitmap.diff` around lines 75 - 89, The extractBitmapLeakMinor function currently checks for BITMAP_LEAK_ERR_MSG in the full stderr but then applies BITMAP_LEAK_MINOR_PATTERN against the entire stderr which can pick up an unrelated "minor N" earlier; change the logic so you first locate the specific bitmap-leak line (use BITMAP_LEAK_ERR_MSG matcher to extract that line/segment) and then apply BITMAP_LEAK_MINOR_PATTERN only to that extracted line before parsing the integer; update references in extractBitmapLeakMinor and ensure cleanupStaleBitmapAndRetry continues to call extractBitmapLeakMinor unchanged.
128-136:⚠️ Potential issue | 🟠 MajorAbort the retry when the sleep is interrupted.
After Line 134 restores the interrupt flag, the method still falls through to
return trueon Line 136, so the caller immediately re-runsattach. This keeps mutating state on an interrupted thread and skips the intended backoff.🔧 Suggested fix
catch (InterruptedException ignored) { Thread.currentThread().interrupt(); + return false; } return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/linstor/images/piraeus-server/patches/retry-adjust-after-stale-bitmap.diff` around lines 128 - 136, The catch block for InterruptedException in the sleep/retry section currently resets the interrupt flag but then the method continues and returns true, causing an immediate retry; change the control flow so that when Thread.sleep(FORCE_DETACH_RETRY_WAIT_MS) throws InterruptedException you restore the interrupt flag (Thread.currentThread().interrupt()) and then abort the retry by returning false (or otherwise exiting) instead of falling through to return true; update the catch block around Thread.sleep and the method return behavior (referencing Thread.sleep, FORCE_DETACH_RETRY_WAIT_MS, the InterruptedException catch, and the method's final return) so interrupted threads do not continue to re-run attach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/system/linstor/images/piraeus-server/patches/allow-toggle-disk-retry.diff`:
- Around line 140-160: The initial ApiCallRc entry in handleRetryAction uses
ApiConsts.INFO_NOOP which implies no action even though updateAndAdjustDisk(...)
will run; change the response code passed to ApiCallRcImpl.simpleEntry to a more
accurate constant (e.g., ApiConsts.INFO or a new ApiConsts.INFO_RETRY) so the
entry reads like "Retry initiated" rather than a no-op; update the message text
if needed to match the new code and ensure callers that inspect the code treat
this as an actionable retry rather than a no-op.
In
`@packages/system/linstor/images/piraeus-server/patches/fix-luks-header-size.diff`:
- Around line 347-365: The current logic sets unalignedHeaderSizeInBytes to a
fixed LUKS2_HEADER_SIZE_IN_BYTES when cryptsetupLuks2KeyslotsSize (from
getLongOptionValue(userOptions, LUKS2_OPT_KEYSLOTS_SIZE)) is null, which ignores
a user-specified --luks2-metadata-size; change it so when
cryptsetupLuks2KeyslotsSize is null you still read cryptsetupLuks2MetadataSize
(falling back to DFLT_LUKS2_METADATA_SIZE_IN_BYTES) and then compute the
effective keyslots size the same way cryptsetup does (implement a helper like
computeKeyslotsSizeFromMetadata(cryptsetupLuks2MetadataSize) or inline the
matching calculation), and set unalignedHeaderSizeInBytes = 2 *
cryptsetupLuks2MetadataSize + computedKeyslotsSize instead of using
LUKS2_HEADER_SIZE_IN_BYTES.
- Around line 45-55: The composed internal property keys are built with an extra
'/' (StorageConstants.NAMESPACE_INTERNAL already ends with '/'), so update the
calls to updatePropIfNeeded (e.g., the invocations that set
StorageConstants.BLK_DEV_MIN_IO_SIZE and StorageConstants.BLK_DEV_OPT_IO_SIZE
using BlockSizeInfo.getBlockSize/getOptimalIoSize) to concatenate without adding
an extra '/' (use StorageConstants.NAMESPACE_INTERNAL +
StorageConstants.BLK_DEV_MIN_IO_SIZE and similarly for BLK_DEV_OPT_IO_SIZE), and
audit other uses of StorageConstants.NAMESPACE_INTERNAL in this patch to remove
the redundant '/' where present (keep the same propsChange and storPoolObj
arguments).
---
Duplicate comments:
In
`@packages/system/linstor/images/piraeus-server/patches/retry-adjust-after-stale-bitmap.diff`:
- Around line 75-89: The extractBitmapLeakMinor function currently checks for
BITMAP_LEAK_ERR_MSG in the full stderr but then applies
BITMAP_LEAK_MINOR_PATTERN against the entire stderr which can pick up an
unrelated "minor N" earlier; change the logic so you first locate the specific
bitmap-leak line (use BITMAP_LEAK_ERR_MSG matcher to extract that line/segment)
and then apply BITMAP_LEAK_MINOR_PATTERN only to that extracted line before
parsing the integer; update references in extractBitmapLeakMinor and ensure
cleanupStaleBitmapAndRetry continues to call extractBitmapLeakMinor unchanged.
- Around line 128-136: The catch block for InterruptedException in the
sleep/retry section currently resets the interrupt flag but then the method
continues and returns true, causing an immediate retry; change the control flow
so that when Thread.sleep(FORCE_DETACH_RETRY_WAIT_MS) throws
InterruptedException you restore the interrupt flag
(Thread.currentThread().interrupt()) and then abort the retry by returning false
(or otherwise exiting) instead of falling through to return true; update the
catch block around Thread.sleep and the method return behavior (referencing
Thread.sleep, FORCE_DETACH_RETRY_WAIT_MS, the InterruptedException catch, and
the method's final return) so interrupted threads do not continue to re-run
attach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3eb29766-997a-42e7-ab6d-acd57eeac684
📒 Files selected for processing (9)
packages/system/linstor/Makefilepackages/system/linstor/images/piraeus-server/patches/README.mdpackages/system/linstor/images/piraeus-server/patches/adjust-on-resfile-change.diffpackages/system/linstor/images/piraeus-server/patches/allow-toggle-disk-retry.diffpackages/system/linstor/images/piraeus-server/patches/fix-duplicate-tcp-ports.diffpackages/system/linstor/images/piraeus-server/patches/fix-luks-header-size.diffpackages/system/linstor/images/piraeus-server/patches/force-metadata-check-on-disk-add.diffpackages/system/linstor/images/piraeus-server/patches/retry-adjust-after-stale-bitmap.diffpackages/system/linstor/images/piraeus-server/patches/skip-adjust-when-device-inaccessible.diff
💤 Files with no reviewable changes (3)
- packages/system/linstor/images/piraeus-server/patches/skip-adjust-when-device-inaccessible.diff
- packages/system/linstor/images/piraeus-server/patches/force-metadata-check-on-disk-add.diff
- packages/system/linstor/images/piraeus-server/patches/adjust-on-resfile-change.diff
✅ Files skipped from review due to trivial changes (1)
- packages/system/linstor/Makefile
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/system/linstor/images/piraeus-server/patches/fix-duplicate-tcp-ports.diff
- packages/system/linstor/images/piraeus-server/patches/README.md
| + private Flux<ApiCallRc> handleRetryAction( | ||
| + Resource rscRef, | ||
| + boolean removeDisk, | ||
| + boolean toggleIntoTiebreakerRef, | ||
| + ResponseContext context | ||
| + ) | ||
| + { | ||
| + String direction = removeDisk ? "diskless" : "diskful"; | ||
| + ApiCallRcImpl responses = new ApiCallRcImpl(); | ||
| + NodeName nodeName = rscRef.getNode().getName(); | ||
| + ResourceName rscName = rscRef.getResourceDefinition().getName(); | ||
| + responses.addEntry(ApiCallRcImpl.simpleEntry( | ||
| + ApiConsts.INFO_NOOP, | ||
| + "Retrying toggle disk to " + direction + " for resource '" + rscName.displayValue + | ||
| + "' on node '" + nodeName.displayValue + "'" | ||
| + )); | ||
| + | ||
| + ctrlTransactionHelper.commit(); | ||
| + return Flux | ||
| + .<ApiCallRc>just(responses) | ||
| + .concatWith(updateAndAdjustDisk(nodeName, rscName, removeDisk, toggleIntoTiebreakerRef, context)); | ||
| + } |
There was a problem hiding this comment.
Consider using a more descriptive response code than INFO_NOOP.
INFO_NOOP semantically implies no operation will occur, but handleRetryAction proceeds to call updateAndAdjustDisk() which performs actual work. This could mislead API consumers who interpret NOOP as "nothing changed."
Consider using INFO or a distinct code that better conveys "retry initiated" rather than "no-op."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/linstor/images/piraeus-server/patches/allow-toggle-disk-retry.diff`
around lines 140 - 160, The initial ApiCallRc entry in handleRetryAction uses
ApiConsts.INFO_NOOP which implies no action even though updateAndAdjustDisk(...)
will run; change the response code passed to ApiCallRcImpl.simpleEntry to a more
accurate constant (e.g., ApiConsts.INFO or a new ApiConsts.INFO_RETRY) so the
entry reads like "Retry initiated" rather than a no-op; update the message text
if needed to match the new code and ensure callers that inspect the code treat
this as an actionable retry rather than a no-op.
| + updatePropIfNeeded( | ||
| + propsChange, | ||
| + storPoolObj, | ||
| + StorageConstants.NAMESPACE_INTERNAL + '/' + StorageConstants.BLK_DEV_MIN_IO_SIZE, | ||
| + Long.toString(BlockSizeInfo.getBlockSize(blockDevicePath)) | ||
| + ); | ||
| + updatePropIfNeeded( | ||
| + propsChange, | ||
| + storPoolObj, | ||
| + StorageConstants.NAMESPACE_INTERNAL + '/' + StorageConstants.BLK_DEV_OPT_IO_SIZE, | ||
| + Long.toString(BlockSizeInfo.getOptimalIoSize(blockDevicePath)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the patch file to verify the claims
cat -n packages/system/linstor/images/piraeus-server/patches/fix-luks-header-size.diff | head -100Repository: cozystack/cozystack
Length of output: 5436
🏁 Script executed:
# Search for the Props class to understand namespace normalization
find . -type f -name "Props.java" | head -5Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Search for namespace separator handling in the codebase
rg "NAMESPACE_INTERNAL" -A 3 -B 3 --type java | head -100Repository: cozystack/cozystack
Length of output: 211
🏁 Script executed:
# Search for Props in the repository more broadly
fd -type f "Props.java" 2>/dev/null || echo "Props.java not found"Repository: cozystack/cozystack
Length of output: 83
🏁 Script executed:
# Check if there are vendored Java sources in charts
find . -path "*/charts/*" -name "*.java" -type f | head -20Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Search for NAMESPACE_INTERNAL in all files (not just Java)
rg "NAMESPACE_INTERNAL" --no-type 2>/dev/null | head -30Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check if this is a vendored patch from an upstream linstor project
cat packages/system/linstor/images/piraeus-server/patches/fix-luks-header-size.diff | head -20Repository: cozystack/cozystack
Length of output: 1156
🏁 Script executed:
# Check if there's a git submodule for linstor
git config --file .gitmodules --name-only --get-regexp path | head -20Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check the repository structure for clues about vendoring
ls -la packages/system/linstor/ 2>/dev/null | head -20Repository: cozystack/cozystack
Length of output: 661
🏁 Script executed:
# See if there's any documentation about the patch
cat packages/system/linstor/README.md 2>/dev/null || echo "No README found"Repository: cozystack/cozystack
Length of output: 78
🏁 Script executed:
# Search for any references to the upstream LINSTOR project
rg "linstor" --type md | head -20Repository: cozystack/cozystack
Length of output: 4393
🏁 Script executed:
# Read the patches README to find info about fix-luks-header-size.diff
cat packages/system/linstor/images/piraeus-server/patches/README.mdRepository: cozystack/cozystack
Length of output: 2111
🏁 Script executed:
# Search the web for LINSTOR Props namespace behavior
# First, let's look at the upstream commit referenced in the patch
# The patch references commits ccc85fbd2 and 71b601554 from LINBIT/linstor-server
# Let's get more details from the patch about StorageConstants
cat packages/system/linstor/images/piraeus-server/patches/fix-luks-header-size.diff | grep -A 20 "StorageConstants.java"Repository: cozystack/cozystack
Length of output: 895
🌐 Web query:
LINSTOR Props namespace separator handling java
💡 Result:
In LINSTOR, properties (props) use a hierarchical namespace structure separated by forward slashes (/). The slash acts as a namespace delimiter, allowing organized key hierarchies like "Aux/foo" for auxiliary properties or "DrbdOptions/Net/protocol" for DRBD-specific settings. Key handling details: - Auxiliary properties (user-defined): Prefix with "Aux/" (e.g., linstor n sp --aux foo bar sets Aux/foo=bar). Direct "Aux/foo bar" also works for setting/querying. - Built-in properties: Defined in linstor-common/properties.json with namespace paths like ["NAMESPC_STORAGE_DRIVER", "KEY_STOR_POOL_NAME"]. - In Python client: attach_aux_prop prepends NAMESPC_AUXILIARY + '/' (Aux/) to keys when --aux flag used. - In Python API: KV class supports namespaces as UNIX-like paths (e.g., namespace='/foo/bar/'), prepending to keys. - CLI inconsistencies noted in issues: --aux foo=bar may delete instead of set; query with --prop Aux/foo but list shows Aux/ prefix. Java handling (linstor-server/api-java): Properties managed via AbsPropertiesMap or similar (inferred from Java codebase), storing full namespaced keys. No specific bugs found for separator mishandling, but general property ops use string keys with / intact. Official docs (linbit.com guides, REST API spec) confirm / as standard separator without special escaping; keys validated by regex (e.g., [a-zA-Z0-9/-]+ for some). No evidence of separator escaping or special Java parsing issues; / is literal namespace delimiter. Use full namespaced keys consistently.
Citations:
- 1: Mess with aux properties LINBIT/linstor-client#36
- 2: https://github.com/LINBIT/linstor-common/blob/master/properties.json
- 3: https://linbit.github.io/linstor-api-py/introduction.html
- 4: https://github.com/LINBIT/linstor-client/blob/master/linstor_client/commands/commands.py
- 5: https://github.com/LINBIT/linstor-server/blob/master/docs/rest_v1_openapi.yaml
- 6: https://linbit.github.io/linstor-api-py/linstor.html
🌐 Web query:
LINBIT linstor-server StorageConstants NAMESPACE_INTERNAL
💡 Result:
NAMESPACE_INTERNAL is a constant defined in LINSTOR's shared constants, used in the linstor-server (Java codebase) and client Python code. It represents an internal namespace for storage-related properties, such as in storage pool configurations. From source code analysis: - In linstor_client/commands/storpool_cmds.py (result 3): Used in constructing property keys like apiconsts.NAMESPC_STORAGE_DRIVER + '/' + apiconsts.NAMESPC_EBS + '/' + apiconsts.KEY_REMOTE for EBS storage pools. - Imported as linstor.sharedconsts as apiconsts across linstor-client Python files (results 3,7,19). - Likely defined in linstor-common/proto or generated Java constants (linstor-server build.gradle result 24 generates ApiConsts.java). - Common pattern: apiconsts.NAMESPC_STORAGE_DRIVER, apiconsts.NAMESPC_AUXILIARY, suggesting NAMESPACE_INTERNAL is a string like "internal" or "/internal" for LINSTOR's internal property namespace, distinguishing user vs internal props (properties[objname] filters 'internal':False). No exact string value found in snippets, but context indicates it's for internal storage constants in linstor-server. Official source: LINBIT/linstor-server GitHub repo (result 1).
Citations:
- 1: https://github.com/LINBIT/linstor-server
- 2: https://github.com/LINBIT/linstor-client/blob/master/linstor_client/commands/storpool_cmds.py
- 3: https://github.com/LINBIT/linstor-client/blob/master/linstor_client/commands/commands.py
- 4: https://github.com/LINBIT/linstor-api-py/blob/master/linstor/resource.py
- 5: https://github.com/LINBIT/linstor-server/blob/master/build.gradle
🏁 Script executed:
# Verify the exact definition by examining more of the patch
grep -A 2 "NAMESPACE_INTERNAL" packages/system/linstor/images/piraeus-server/patches/fix-luks-header-size.diffRepository: cozystack/cozystack
Length of output: 1104
Remove the extra / when composing internal property keys.
StorageConstants.NAMESPACE_INTERNAL already ends with / (defined as NAMESPACE_STOR_DRIVER + "/internal/"). Concatenating it with '/' on lines 48 and 54 creates malformed keys with double slashes (internal//minIoSize instead of internal/minIoSize). Since LINSTOR properties use literal / separators without normalization, these keys won't match read operations, causing property values to silently default.
Suggested fix
- StorageConstants.NAMESPACE_INTERNAL + '/' + StorageConstants.BLK_DEV_MIN_IO_SIZE,
+ StorageConstants.NAMESPACE_INTERNAL + StorageConstants.BLK_DEV_MIN_IO_SIZE,
...
- StorageConstants.NAMESPACE_INTERNAL + '/' + StorageConstants.BLK_DEV_OPT_IO_SIZE,
+ StorageConstants.NAMESPACE_INTERNAL + StorageConstants.BLK_DEV_OPT_IO_SIZE,Also applies to other uses of this pattern in the patch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + updatePropIfNeeded( | |
| + propsChange, | |
| + storPoolObj, | |
| + StorageConstants.NAMESPACE_INTERNAL + '/' + StorageConstants.BLK_DEV_MIN_IO_SIZE, | |
| + Long.toString(BlockSizeInfo.getBlockSize(blockDevicePath)) | |
| + ); | |
| + updatePropIfNeeded( | |
| + propsChange, | |
| + storPoolObj, | |
| + StorageConstants.NAMESPACE_INTERNAL + '/' + StorageConstants.BLK_DEV_OPT_IO_SIZE, | |
| + Long.toString(BlockSizeInfo.getOptimalIoSize(blockDevicePath)) | |
| updatePropIfNeeded( | |
| propsChange, | |
| storPoolObj, | |
| StorageConstants.NAMESPACE_INTERNAL + StorageConstants.BLK_DEV_MIN_IO_SIZE, | |
| Long.toString(BlockSizeInfo.getBlockSize(blockDevicePath)) | |
| ); | |
| updatePropIfNeeded( | |
| propsChange, | |
| storPoolObj, | |
| StorageConstants.NAMESPACE_INTERNAL + StorageConstants.BLK_DEV_OPT_IO_SIZE, | |
| Long.toString(BlockSizeInfo.getOptimalIoSize(blockDevicePath)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/linstor/images/piraeus-server/patches/fix-luks-header-size.diff`
around lines 45 - 55, The composed internal property keys are built with an
extra '/' (StorageConstants.NAMESPACE_INTERNAL already ends with '/'), so update
the calls to updatePropIfNeeded (e.g., the invocations that set
StorageConstants.BLK_DEV_MIN_IO_SIZE and StorageConstants.BLK_DEV_OPT_IO_SIZE
using BlockSizeInfo.getBlockSize/getOptimalIoSize) to concatenate without adding
an extra '/' (use StorageConstants.NAMESPACE_INTERNAL +
StorageConstants.BLK_DEV_MIN_IO_SIZE and similarly for BLK_DEV_OPT_IO_SIZE), and
audit other uses of StorageConstants.NAMESPACE_INTERNAL in this patch to remove
the redundant '/' where present (keep the same propsChange and storPoolObj
arguments).
| + @Nullable Long cryptsetupLuks2KeyslotsSize = getLongOptionValue( | ||
| + userOptions, | ||
| + LUKS2_OPT_KEYSLOTS_SIZE | ||
| + ); | ||
| + if (cryptsetupLuks2KeyslotsSize == null) | ||
| + { | ||
| + unalignedHeaderSizeInBytes = LUKS2_HEADER_SIZE_IN_BYTES; | ||
| + } | ||
| + else | ||
| + { | ||
| + @Nullable Long cryptsetupLuks2MetadataSize = getLongOptionValue( | ||
| + userOptions, | ||
| + LUKS2_OPT_METADATA_SIZE | ||
| + ); | ||
| + cryptsetupLuks2MetadataSize = cryptsetupLuks2MetadataSize == null ? | ||
| + DFLT_LUKS2_METADATA_SIZE_IN_BYTES : | ||
| + cryptsetupLuks2MetadataSize; | ||
| + | ||
| + unalignedHeaderSizeInBytes = 2 * cryptsetupLuks2MetadataSize + cryptsetupLuks2KeyslotsSize; |
There was a problem hiding this comment.
❓ Verification inconclusive
For cryptsetup luksFormat, can --luks2-metadata-size be specified without --luks2-keyslots-size, and if so how should it change the effective LUKS2 header/payload offset calculation?
Yes, --luks2-metadata-size can be specified without --luks2-keyslots-size. The man pages list both options independently under LUKS2 format options for cryptsetup luksFormat, with no requirement that they must be used together (2,3,11,12,61). Examples in issues and discussions use --luks2-metadata-size alone or with specific combinations, confirming independent usage (6). When only --luks2-metadata-size is specified, the keyslots size defaults to a value calculated by the storage parameter function LUKS2_hdr_get_storage_params to fit the available space up to the data offset, typically aiming for a reasonable number of keyslots (up to 32 max) while respecting alignment and device constraints (9, LUKS2 code). The default without options is 16 MiB total header (metadata + keyslots), but specifying metadata size adjusts the JSON area to the given value (valid sizes: 16k to 4096k per spec), and keyslots takes the remainder or a computed default (16,18,20). The effective LUKS2 header/payload offset calculation follows the on-disk layout: Primary binary header (4096 bytes) + primary JSON area (hdr_size - 4096, set by metadata size) + keyslots area (size from config or computed) + alignment padding to secondary header at fixed offsets (16k,32k,...,4M based on metadata size per Table 1 in LUKS2 spec) + secondary binary header + secondary JSON + remaining keyslots space if any. The payload offset is explicitly set in the JSON segments object as a 4096-byte aligned value (multiple of 8 512B sectors), computed as the end of all header areas including both metadata copies and full keyslots area, ensuring space for up to the max keyslots (7,8,9,23). Without --offset, cryptsetup auto-computes the minimal aligned offset accommodating the specified (or default) metadata and computed keyslots sizes (6,16). Use --offset to override for custom alignment, but it must fit the areas.
Citations:
- 1: https://manpages.ubuntu.com/manpages/plucky/en/man8/cryptsetup-luksFormat.8.html
- 2: https://man.archlinux.org/man/cryptsetup-luksFormat.8.en
- 3: https://gitlab.com/cryptsetup/cryptsetup/-/issues/791
- 4: https://vhs.codeberg.page/post/external-backup-drive-encryption/assets/luks2_doc_wip.pdf
- 5: https://gitlab.com/cryptsetup/cryptsetup/raw/v2.0.0/docs/LUKS2-format.txt
- 6: https://gitlab.com/cryptsetup/cryptsetup/raw/master/lib/luks2/luks2.h
- 7: https://man7.org/linux/man-pages/man8/cryptsetup-luksFormat.8.html
- 8: https://manpages.debian.org/testing/cryptsetup-bin/cryptsetup-luksFormat.8.en.html
- 9: https://superuser.com/questions/1557750/why-does-cryptsetup-fail-with-container-10m-in-size/1557798
- 10: https://www.spinics.net/lists/dm-crypt/msg08261.html
- 11: https://cdn.kernel.org/pub/linux/utils/cryptsetup/v2.1/v2.1.0-ReleaseNotes
- 12: https://security.stackexchange.com/questions/227359/how-to-determine-start-and-end-bytes-of-luks-header
- 13: https://manpages.ubuntu.com/manpages/plucky/man8/cryptsetup-luksFormat.8.html
--luks2-metadata-size changes effective header size but code ignores it when keyslots size is absent.
The code unconditionally falls back to a fixed 16 MiB header (line 351) whenever --luks2-keyslots-size is absent. However, --luks2-metadata-size can be specified independently, and when it is, cryptsetup computes a corresponding keyslots size rather than using a fixed value. The code's fixed fallback does not account for this computed size, causing the header offset calculation to mismatch cryptsetup's actual on-disk layout. When metadata size is explicitly set without keyslots size, the code under-sizes the header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/linstor/images/piraeus-server/patches/fix-luks-header-size.diff`
around lines 347 - 365, The current logic sets unalignedHeaderSizeInBytes to a
fixed LUKS2_HEADER_SIZE_IN_BYTES when cryptsetupLuks2KeyslotsSize (from
getLongOptionValue(userOptions, LUKS2_OPT_KEYSLOTS_SIZE)) is null, which ignores
a user-specified --luks2-metadata-size; change it so when
cryptsetupLuks2KeyslotsSize is null you still read cryptsetupLuks2MetadataSize
(falling back to DFLT_LUKS2_METADATA_SIZE_IN_BYTES) and then compute the
effective keyslots size the same way cryptsetup does (implement a helper like
computeKeyslotsSizeFromMetadata(cryptsetupLuks2MetadataSize) or inline the
matching calculation), and set unalignedHeaderSizeInBytes = 2 *
cryptsetupLuks2MetadataSize + computedKeyslotsSize instead of using
LUKS2_HEADER_SIZE_IN_BYTES.
All existing patches (allow-toggle-disk-retry, fix-duplicate-tcp-ports, fix-luks-header-size, retry-adjust-after-stale-bitmap) apply cleanly on the new version without modifications. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Andrei Kvapil (kvaps)
changed the title
Andrei Kvapil (kvaps)
added
kind/backport
|
Successfully created backport PR for |
2 participants
Summary
packages/system/linstortolinstor-serverv1.33.1piraeus-serverbackports needed for this updateSelected backports
3d97f71c95a493588d3d521c63eac4d846935fb379d6375c55d6181b35a7b7f0fe8dbdfb86e126cdandbcc89902f4f61ac1589dd07ebb7f5aae1935370dccc85fbd2c65f0b97c52403fa80f1efdb886ec4eplus the requiredoptimal_io_sizedependency from71b601554f41bcb50cd5bd06989c5b0d3a814acdVerification
v1.33.1withgit apply --check./gradlew --no-daemon compileJavamake imageinpackages/system/linstorsuccessfullySummary by CodeRabbit
Chores
Bug Fixes / Reliability
Storage / Compatibility