Repository navigation
feat(linstor): add linstor-csi patches for clone/snapshot relocation - #2133
Merged
Andrei Kvapil (kvaps) merged 7 commits intoMar 16, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request modifies the CDI clone strategy configuration from "copy" to "csi-clone" and introduces two Linstor patches: one adds relocation logic after clone and restore operations with resource placement optimization, and another implements RWX block attachment validation with VM resolution capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Clone as Clone/Restore Operation
participant Linstor as Linstor Client
participant Autoplacer as Autoplacer
participant Resource as Resource Manager
Clone->>Linstor: Trigger clone/restore with relocation flag
Linstor->>Linstor: Check relocation parameter enabled
rect rgba(100, 150, 200, 0.5)
Linstor->>Autoplacer: Query optimal placement & storage info
Autoplacer-->>Linstor: Placement recommendations per node
end
rect rgba(150, 100, 200, 0.5)
Linstor->>Resource: Determine nodes to add/remove
Linstor->>Resource: Migrate diskless resources
Resource-->>Linstor: Migration complete
end
Linstor->>Linstor: Mark relocation as triggered
Linstor-->>Clone: Relocation done (best-effort)
sequenceDiagram
participant Controller as ControllerPublishVolume
participant Validator as RWX Validator
participant KubeAPI as Kubernetes API
participant VMResolver as VM Name Resolver
Controller->>Controller: Check if validation enabled
alt Validation not disabled
Controller->>Validator: ValidateRWXBlockAttachment(volumeID)
rect rgba(100, 150, 200, 0.5)
Validator->>KubeAPI: List pods with volume attachment
KubeAPI-->>Validator: Pod list
end
rect rgba(150, 100, 200, 0.5)
Validator->>VMResolver: GetVMNameFromPod(pod)
KubeAPI->>VMResolver: Resolve owner references
VMResolver-->>Validator: VM name or pod name
end
Validator-->>Controller: Validation result (pass/fail + owner name)
end
Controller-->>Controller: Proceed with publish
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Contributor
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 enhances the LINSTOR CSI driver by streamlining its patch management and introducing significant improvements to volume lifecycle operations. It removes an obsolete patch, integrates new logic for intelligent replica relocation following volume cloning and snapshot restoration, and optimizes snapshot restore processes through randomized node selection. These changes aim to improve resource utilization and operational efficiency within LINSTOR-backed storage environments. 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. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to relocate volume replicas to optimal nodes after clone and snapshot restore operations. It also improves the node selection logic for snapshot restores by using randomization to distribute the load. The changes look good overall. I've added a couple of comments regarding potential improvements and compatibility.
| + } | ||
| + | ||
| + // Only migrate min(remove, add) pairs. | ||
| + pairs := min(len(nodesToRemove), len(nodesToAdd)) |
Contributor
There was a problem hiding this comment.
The min function, used here, was introduced as a built-in in Go 1.21. The use of the slices package elsewhere suggests the project might be on Go 1.18+, but not necessarily 1.21+. If the target Go version is below 1.21, this will cause a compilation failure.
To ensure backward compatibility, consider replacing it with a standard comparison. For example:
pairs := len(nodesToRemove)
if len(nodesToAdd) < pairs {
pairs = len(nodesToAdd)
}| + | ||
| + var selectedNode string | ||
| + if len(candidates) > 0 { | ||
| + selectedNode = candidates[rand.Intn(len(candidates))] |
Contributor
There was a problem hiding this comment.
Using math/rand.Intn without seeding results in a predictable sequence of numbers on each program start. This could cause restores to consistently pick the same "random" node after a pod restart, negating the benefit of randomization for load distribution.
For better randomness, consider using the crypto/rand package, which is more suitable for this purpose and doesn't require seeding. Alternatively, if you stick with math/rand, ensure it is seeded once at program startup (e.g., in main()).
Andrei Kvapil (kvaps)
force-pushed
the
feat/linstor-csi-relocate-after-clone-restore
branch
from
March 2, 2026 17:43
a46621d to
bf70831
Compare
Andrei Kvapil (kvaps)
marked this pull request as ready for review
March 2, 2026 17:55
dosubot
Bot
added
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/linstor-csi/patches/001-relocate-after-clone-restore.diff`:
- Around line 171-204: The relocation marker propertyRelocationTriggered is
being set unconditionally before any migrations; change this so the code sets
propertyRelocationTriggered only after a successful migration has been started.
Remove the initial s.client.ResourceDefinitions.Modify call and instead, inside
the loop over pairs (where s.client.Resources.MakeAvailable and
s.client.Resources.Migrate are called), after a successful Migrate (or
immediately after MakeAvailable if you consider that “started”), call
s.client.ResourceDefinitions.Modify to set
OverrideProps{propertyRelocationTriggered: "true"}; ensure you set it only once
(use a boolean flag like relocationMarked), and if the Modify call fails
log/warn but do not treat it as fatal so other pairs still process.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/system/kubevirt-cdi/templates/cdi-cr.yamlpackages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diffpackages/system/linstor/images/linstor-csi/patches/001-rwx-validation.diffpackages/system/linstor/templates/volumesnapshotclass.yaml
💤 Files with no reviewable changes (1)
- packages/system/linstor/images/linstor-csi/patches/001-rwx-validation.diff
Comment on lines
+171
to
+204
| + // Mark relocation as triggered before starting migrations. | ||
| + err = s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{ | ||
| + OverrideProps: map[string]string{propertyRelocationTriggered: "true"}, | ||
| + }) | ||
| + if err != nil { | ||
| + return fmt.Errorf("failed to set relocation property: %w", err) | ||
| + } | ||
| + | ||
| + // Migrate each pair sequentially (LINSTOR serializes at the RD level anyway). | ||
| + for i := range pairs { | ||
| + fromNode := nodesToRemove[i] | ||
| + toNode := nodesToAdd[i] | ||
| + storPool := optimalPools[toNode] | ||
| + | ||
| + logger := logger.WithFields(logrus.Fields{"from": fromNode, "to": toNode, "storagePool": storPool}) | ||
| + | ||
| + logger.Info("creating diskless resource on target node") | ||
| + | ||
| + err := s.client.Resources.MakeAvailable(ctx, volID, toNode, lapi.ResourceMakeAvailable{}) | ||
| + if err != nil { | ||
| + logger.WithError(err).Warn("failed to create diskless on target, skipping this pair") | ||
| + continue | ||
| + } | ||
| + | ||
| + logger.Info("initiating migrate-disk") | ||
| + | ||
| + err = s.client.Resources.Migrate(ctx, volID, fromNode, toNode, storPool) | ||
| + if err != nil { | ||
| + logger.WithError(err).Warn("migrate-disk failed, skipping this pair") | ||
| + continue | ||
| + } | ||
| + } | ||
| + | ||
| + return nil |
Contributor
There was a problem hiding this comment.
Set the relocation marker after successful migration start, not before.
propertyRelocationTriggered is written before any migration succeeds. If all pairs fail transiently, later flows will skip relocation forever for that volume.
🔧 Proposed fix
- // Mark relocation as triggered before starting migrations.
- err = s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{
- OverrideProps: map[string]string{propertyRelocationTriggered: "true"},
- })
- if err != nil {
- return fmt.Errorf("failed to set relocation property: %w", err)
- }
-
- // Migrate each pair sequentially (LINSTOR serializes at the RD level anyway).
+ // Migrate each pair sequentially (LINSTOR serializes at the RD level anyway).
+ migrated := false
for i := range pairs {
fromNode := nodesToRemove[i]
toNode := nodesToAdd[i]
storPool := optimalPools[toNode]
@@
err = s.client.Resources.Migrate(ctx, volID, fromNode, toNode, storPool)
if err != nil {
logger.WithError(err).Warn("migrate-disk failed, skipping this pair")
continue
}
+
+ migrated = true
}
+
+ if !migrated {
+ return fmt.Errorf("relocation could not be started on any migration pair")
+ }
+
+ err = s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{
+ OverrideProps: map[string]string{propertyRelocationTriggered: "true"},
+ })
+ if err != nil {
+ return fmt.Errorf("failed to set relocation property: %w", err)
+ }📝 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.
Suggested change
| + // Mark relocation as triggered before starting migrations. | |
| + err = s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{ | |
| + OverrideProps: map[string]string{propertyRelocationTriggered: "true"}, | |
| + }) | |
| + if err != nil { | |
| + return fmt.Errorf("failed to set relocation property: %w", err) | |
| + } | |
| + | |
| + // Migrate each pair sequentially (LINSTOR serializes at the RD level anyway). | |
| + for i := range pairs { | |
| + fromNode := nodesToRemove[i] | |
| + toNode := nodesToAdd[i] | |
| + storPool := optimalPools[toNode] | |
| + | |
| + logger := logger.WithFields(logrus.Fields{"from": fromNode, "to": toNode, "storagePool": storPool}) | |
| + | |
| + logger.Info("creating diskless resource on target node") | |
| + | |
| + err := s.client.Resources.MakeAvailable(ctx, volID, toNode, lapi.ResourceMakeAvailable{}) | |
| + if err != nil { | |
| + logger.WithError(err).Warn("failed to create diskless on target, skipping this pair") | |
| + continue | |
| + } | |
| + | |
| + logger.Info("initiating migrate-disk") | |
| + | |
| + err = s.client.Resources.Migrate(ctx, volID, fromNode, toNode, storPool) | |
| + if err != nil { | |
| + logger.WithError(err).Warn("migrate-disk failed, skipping this pair") | |
| + continue | |
| + } | |
| + } | |
| + | |
| + return nil | |
| // Migrate each pair sequentially (LINSTOR serializes at the RD level anyway). | |
| migrated := false | |
| for i := range pairs { | |
| fromNode := nodesToRemove[i] | |
| toNode := nodesToAdd[i] | |
| storPool := optimalPools[toNode] | |
| logger := logger.WithFields(logrus.Fields{"from": fromNode, "to": toNode, "storagePool": storPool}) | |
| logger.Info("creating diskless resource on target node") | |
| err := s.client.Resources.MakeAvailable(ctx, volID, toNode, lapi.ResourceMakeAvailable{}) | |
| if err != nil { | |
| logger.WithError(err).Warn("failed to create diskless on target, skipping this pair") | |
| continue | |
| } | |
| logger.Info("initiating migrate-disk") | |
| err = s.client.Resources.Migrate(ctx, volID, fromNode, toNode, storPool) | |
| if err != nil { | |
| logger.WithError(err).Warn("migrate-disk failed, skipping this pair") | |
| continue | |
| } | |
| migrated = true | |
| } | |
| if !migrated { | |
| return fmt.Errorf("relocation could not be started on any migration pair") | |
| } | |
| err = s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{ | |
| OverrideProps: map[string]string{propertyRelocationTriggered: "true"}, | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to set relocation property: %w", err) | |
| } | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff`
around lines 171 - 204, The relocation marker propertyRelocationTriggered is
being set unconditionally before any migrations; change this so the code sets
propertyRelocationTriggered only after a successful migration has been started.
Remove the initial s.client.ResourceDefinitions.Modify call and instead, inside
the loop over pairs (where s.client.Resources.MakeAvailable and
s.client.Resources.Migrate are called), after a successful Migrate (or
immediately after MakeAvailable if you consider that “started”), call
s.client.ResourceDefinitions.Modify to set
OverrideProps{propertyRelocationTriggered: "true"}; ensure you set it only once
(use a boolean flag like relocationMarked), and if the Modify call fails
log/warn but do not treat it as fatal so other pairs still process.
IvanHunters
approved these changes
Mar 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff (1)
171-177:⚠️ Potential issue | 🟠 MajorMove relocation marker write to after first successful migration start.
propertyRelocationTriggeredis set before anyMigratesucceeds. If all pairs fail transiently, retries are permanently skipped for this volume.🔧 Proposed fix
- // Mark relocation as triggered before starting migrations. - err = s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{ - OverrideProps: map[string]string{propertyRelocationTriggered: "true"}, - }) - if err != nil { - return fmt.Errorf("failed to set relocation property: %w", err) - } - - // Migrate each pair sequentially (LINSTOR serializes at the RD level anyway). - for i := range pairs { + // Migrate each pair sequentially (LINSTOR serializes at the RD level anyway). + relocationMarked := false + migrated := false + for i := range pairs { fromNode := nodesToRemove[i] toNode := nodesToAdd[i] storPool := optimalPools[toNode] @@ err = s.client.Resources.Migrate(ctx, volID, fromNode, toNode, storPool) if err != nil { logger.WithError(err).Warn("migrate-disk failed, skipping this pair") continue } + + migrated = true + if !relocationMarked { + if err := s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{ + OverrideProps: map[string]string{propertyRelocationTriggered: "true"}, + }); err != nil { + logger.WithError(err).Warn("failed to set relocation marker") + } else { + relocationMarked = true + } + } } + + if !migrated { + return fmt.Errorf("relocation could not be started on any migration pair") + }Also applies to: 179-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff` around lines 171 - 177, The relocation marker (propertyRelocationTriggered) is being set before any Migrate call succeeds which causes retries to be skipped if initial migrations fail; change the flow so that s.client.ResourceDefinitions.Modify(...) that sets propertyRelocationTriggered = "true" is performed only after the first successful call to Migrate for the volume (use volID/ctx and detect success from Migrate return), not before starting the migration attempts; ensure the same fix is applied to the other similar block around lines 179-202 so the marker is only written once a migration has actually started successfully.
🤖 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/linstor-csi/patches/001-relocate-after-clone-restore.diff`:
- Around line 349-355: The switch branch currently checks for the wrong key
string (case "/relocateAfterRestore") so the VolumeSnapshotClass parameter
"relocate-after-restore" is ignored; update the switch in the parser to match
the documented parameter name by changing the case to "relocate-after-restore"
so parsed value is converted to bool and stored into p.RelocateAfterRestore
(keep the strconv.ParseBool logic and error handling intact).
---
Duplicate comments:
In
`@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff`:
- Around line 171-177: The relocation marker (propertyRelocationTriggered) is
being set before any Migrate call succeeds which causes retries to be skipped if
initial migrations fail; change the flow so that
s.client.ResourceDefinitions.Modify(...) that sets propertyRelocationTriggered =
"true" is performed only after the first successful call to Migrate for the
volume (use volID/ctx and detect success from Migrate return), not before
starting the migration attempts; ensure the same fix is applied to the other
similar block around lines 179-202 so the marker is only written once a
migration has actually started successfully.
Comment on lines
+349
to
+355
| + case "/relocateAfterRestore": | ||
| + b, err := strconv.ParseBool(v) | ||
| + if err != nil { | ||
| + return nil, err | ||
| + } | ||
| + | ||
| + p.RelocateAfterRestore = b |
Contributor
There was a problem hiding this comment.
Parse key does not match documented snapshot parameter name.
This branch handles "/relocateAfterRestore", but the documented VolumeSnapshotClass parameter is relocate-after-restore. With the current key, the intended parameter is ignored and relocation-after-restore never activates.
🔧 Proposed fix
- case "/relocateAfterRestore":
+ case "/relocate-after-restore", "/relocateAfterRestore":
b, err := strconv.ParseBool(v)
if err != nil {
return nil, err
}📝 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.
Suggested change
| + case "/relocateAfterRestore": | |
| + b, err := strconv.ParseBool(v) | |
| + if err != nil { | |
| + return nil, err | |
| + } | |
| + | |
| + p.RelocateAfterRestore = b | |
| case "/relocate-after-restore", "/relocateAfterRestore": | |
| b, err := strconv.ParseBool(v) | |
| if err != nil { | |
| return nil, err | |
| } | |
| p.RelocateAfterRestore = b |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff`
around lines 349 - 355, The switch branch currently checks for the wrong key
string (case "/relocateAfterRestore") so the VolumeSnapshotClass parameter
"relocate-after-restore" is ignored; update the switch in the parser to match
the documented parameter name by changing the case to "relocate-after-restore"
so parsed value is converted to bool and stored into p.RelocateAfterRestore
(keep the strconv.ParseBool logic and error handling intact).
Remove merged RWX validation patch and add new patch that includes: - Randomized node selection for snapshot restore - Relocate replicas to optimal nodes after clone and snapshot restore Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Move snapshot restore relocation parameter from StorageClass to VolumeSnapshotClass to avoid unwanted relocation during Velero data mover backup. Change relocateAfterClone default to false. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Use CSI clone instead of host-assisted copy for CDI DataVolume cloning. This leverages LINSTOR's native rd-clone mechanism which is significantly faster than pod-based data copying, and works together with the new relocateAfterClone parameter. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Enable relocate-after-restore on the default linstor-snapshots class so that PVCs restored from snapshots get replicas relocated to optimal nodes. Add a separate linstor-snapshots-velero class with the Velero annotation and without relocation, so Velero's temporary data mover PVCs are not relocated unnecessarily. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Andrei Kvapil (kvaps)
force-pushed
the
feat/linstor-csi-relocate-after-clone-restore
branch
from
March 10, 2026 11:42
f5cf5b1 to
37a6816
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff (1)
171-177:⚠️ Potential issue | 🟠 MajorDon’t persist the relocation marker before any migration actually starts.
If every
MakeAvailable/Migrateattempt later in the loop fails transiently, this still writesAux/csi-relocation-triggered=true, and future clone/restore flows will skip relocation forever for that volume. Move the marker write until after the first successful migration kickoff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff` around lines 171 - 177, The relocation marker is being written too early: instead of setting propertyRelocationTriggered via s.client.ResourceDefinitions.Modify(volID, ...) before any migrations, move that Modify call so it runs only after the first successful migration kickoff (i.e., after the code path that successfully calls MakeAvailable/Migrate for the first time completes). Locate the block that currently sets propertyRelocationTriggered and remove it from the pre-migration area; insert the Modify call immediately after the first successful migration start (the success branch of the MakeAvailable/Migrate loop) so transient failures do not permanently set Aux/csi-relocation-triggered=true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff`:
- Around line 171-177: The relocation marker is being written too early: instead
of setting propertyRelocationTriggered via
s.client.ResourceDefinitions.Modify(volID, ...) before any migrations, move that
Modify call so it runs only after the first successful migration kickoff (i.e.,
after the code path that successfully calls MakeAvailable/Migrate for the first
time completes). Locate the block that currently sets
propertyRelocationTriggered and remove it from the pre-migration area; insert
the Modify call immediately after the first successful migration start (the
success branch of the MakeAvailable/Migrate loop) so transient failures do not
permanently set Aux/csi-relocation-triggered=true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f04ad94-b127-4ac3-a33b-bf0566710818
📒 Files selected for processing (4)
packages/system/kubevirt-cdi/templates/cdi-cr.yamlpackages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diffpackages/system/linstor/images/linstor-csi/patches/001-rwx-validation.diffpackages/system/linstor/templates/volumesnapshotclass.yaml
💤 Files with no reviewable changes (1)
- packages/system/linstor/images/linstor-csi/patches/001-rwx-validation.diff
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/system/linstor/templates/volumesnapshotclass.yaml
Hardcode relocateAfterClone=true and relocateAfterRestore=false as defaults in the CSI driver patch instead of exposing them via StorageClass/VolumeSnapshotClass parameters. Remove the extra linstor-snapshots-ephemeral VolumeSnapshotClass (Velero) and the relocateAfterRestore parameter from the default VolumeSnapshotClass. This is a temporary measure while upstream linstor-server is deciding on the interface for the rebalance feature (see LINBIT/linstor-server#487). Once upstream provides native rebalance support, these hardcoded defaults will be replaced by the proper upstream mechanism. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Member
Author
|
Updated the branch: hardcoded This is a temporary measure while upstream linstor-server is deciding on the interface for native rebalance support (see LINBIT/linstor-server#487). Once the upstream mechanism is available, these hardcoded defaults will be replaced accordingly. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff (2)
349-355:⚠️ Potential issue | 🟠 MajorParse key does not match the JSON field naming convention.
The switch case checks for
/relocateAfterRestore(camelCase), but the JSON tag on the struct field isrelocate-after-restore(kebab-case). This inconsistency may cause the parameter to be ignored when users specify it using the expected kebab-case format in VolumeSnapshotClass.🔧 Proposed fix to accept both formats
- case "/relocateAfterRestore": + case "/relocate-after-restore", "/relocateAfterRestore": b, err := strconv.ParseBool(v) if err != nil { return nil, err } p.RelocateAfterRestore = b🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff` around lines 349 - 355, The switch branch currently matches "/relocateAfterRestore" (camelCase) but the struct JSON tag is "relocate-after-restore" (kebab-case), so update the parsing in the same switch that sets p.RelocateAfterRestore to accept the kebab-case key ("/relocate-after-restore") — ideally accept both "/relocate-after-restore" and "/relocateAfterRestore" by adding a case for the kebab form (or a second case line) before parsing with strconv.ParseBool and assigning to p.RelocateAfterRestore; ensure error handling remains the same.
171-204:⚠️ Potential issue | 🟠 MajorSet the relocation marker after at least one successful migration, not before.
The
propertyRelocationTriggeredproperty is written before any migration attempt. If all migrations fail transiently, subsequent operations will skip relocation permanently for this volume since the marker is already set.🔧 Proposed fix
- // Mark relocation as triggered before starting migrations. - err = s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{ - OverrideProps: map[string]string{propertyRelocationTriggered: "true"}, - }) - if err != nil { - return fmt.Errorf("failed to set relocation property: %w", err) - } - // Migrate each pair sequentially (LINSTOR serializes at the RD level anyway). + migrated := false for i := range pairs { fromNode := nodesToRemove[i] toNode := nodesToAdd[i] storPool := optimalPools[toNode] logger := logger.WithFields(logrus.Fields{"from": fromNode, "to": toNode, "storagePool": storPool}) logger.Info("creating diskless resource on target node") err := s.client.Resources.MakeAvailable(ctx, volID, toNode, lapi.ResourceMakeAvailable{}) if err != nil { logger.WithError(err).Warn("failed to create diskless on target, skipping this pair") continue } logger.Info("initiating migrate-disk") err = s.client.Resources.Migrate(ctx, volID, fromNode, toNode, storPool) if err != nil { logger.WithError(err).Warn("migrate-disk failed, skipping this pair") continue } + + migrated = true } + + if migrated { + err = s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{ + OverrideProps: map[string]string{propertyRelocationTriggered: "true"}, + }) + if err != nil { + logger.WithError(err).Warn("failed to set relocation property") + } + } return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff` around lines 171 - 204, The relocation flag is being set too early; remove the pre-loop call to s.client.ResourceDefinitions.Modify that writes propertyRelocationTriggered and instead set propertyRelocationTriggered = "true" only after at least one successful migrate (i.e., after a successful s.client.Resources.Migrate call inside the for loop over pairs). Track a boolean like migratedAny across the loop (use volID, nodesToRemove/nodesToAdd, optimalPools and the existing logger), and if migratedAny is true call s.client.ResourceDefinitions.Modify(ctx, volID, lapi.GenericPropsModify{OverrideProps: map[string]string{propertyRelocationTriggered: "true"}}) and handle/return any error from that Modify.
🤖 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/linstor-csi/patches/001-relocate-after-clone-restore.diff`:
- Around line 223-241: NodesAndConditionFromResources currently adds to allNodes
only when a resource has a volume with linstor.PropertyCreatedFor ==
linstor.CreatedForTemporaryDisklessAttach but still appends any res.NodeName to
abnormalNodes when res.State == nil, causing mismatch; update the abnormalNodes
append to use the same CSI-publish-target predicate (check res.Volumes with the
slices.ContainsFunc test against linstor.PropertyCreatedFor and
linstor.CreatedForTemporaryDisklessAttach) so you only record an abnormal node
if it is also included in allNodes, keeping both lists consistent.
---
Duplicate comments:
In
`@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff`:
- Around line 349-355: The switch branch currently matches
"/relocateAfterRestore" (camelCase) but the struct JSON tag is
"relocate-after-restore" (kebab-case), so update the parsing in the same switch
that sets p.RelocateAfterRestore to accept the kebab-case key
("/relocate-after-restore") — ideally accept both "/relocate-after-restore" and
"/relocateAfterRestore" by adding a case for the kebab form (or a second case
line) before parsing with strconv.ParseBool and assigning to
p.RelocateAfterRestore; ensure error handling remains the same.
- Around line 171-204: The relocation flag is being set too early; remove the
pre-loop call to s.client.ResourceDefinitions.Modify that writes
propertyRelocationTriggered and instead set propertyRelocationTriggered = "true"
only after at least one successful migrate (i.e., after a successful
s.client.Resources.Migrate call inside the for loop over pairs). Track a boolean
like migratedAny across the loop (use volID, nodesToRemove/nodesToAdd,
optimalPools and the existing logger), and if migratedAny is true call
s.client.ResourceDefinitions.Modify(ctx, volID,
lapi.GenericPropsModify{OverrideProps:
map[string]string{propertyRelocationTriggered: "true"}}) and handle/return any
error from that Modify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2600c774-d966-447b-9b56-d837dbdb93a9
📒 Files selected for processing (1)
packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff
Comment on lines
+223
to
+241
| -func NodesAndConditionFromResources(ress []lapi.Resource) ([]string, *csi.VolumeCondition) { | ||
| +func NodesAndConditionFromResources(ress []lapi.ResourceWithVolumes) ([]string, *csi.VolumeCondition) { | ||
| var allNodes, abnormalNodes []string | ||
|
|
||
| for i := range ress { | ||
| res := &ress[i] | ||
|
|
||
| - allNodes = append(allNodes, res.NodeName) | ||
| + // A resource is a CSI publish target if any of its volumes were created | ||
| + // by ControllerPublishVolume, identified by the temporary-diskless-attach property. | ||
| + if slices.ContainsFunc(res.Volumes, func(v lapi.Volume) bool { | ||
| + createdFor, ok := v.Props[linstor.PropertyCreatedFor] | ||
| + return ok && createdFor == linstor.CreatedForTemporaryDisklessAttach | ||
| + }) { | ||
| + allNodes = append(allNodes, res.NodeName) | ||
| + } | ||
|
|
||
| if res.State == nil { | ||
| abnormalNodes = append(abnormalNodes, res.NodeName) |
Contributor
There was a problem hiding this comment.
Inconsistent filtering between allNodes and abnormalNodes.
The new logic only adds nodes to allNodes if they have the temporary-diskless-attach property, but abnormalNodes still includes any resource with State == nil, regardless of whether it's a CSI publish target. This could report abnormal conditions for nodes that aren't included in allNodes.
🔧 Proposed fix to make abnormalNodes consistent
for i := range ress {
res := &ress[i]
// A resource is a CSI publish target if any of its volumes were created
// by ControllerPublishVolume, identified by the temporary-diskless-attach property.
- if slices.ContainsFunc(res.Volumes, func(v lapi.Volume) bool {
+ isCsiTarget := slices.ContainsFunc(res.Volumes, func(v lapi.Volume) bool {
createdFor, ok := v.Props[linstor.PropertyCreatedFor]
return ok && createdFor == linstor.CreatedForTemporaryDisklessAttach
- }) {
+ })
+
+ if isCsiTarget {
allNodes = append(allNodes, res.NodeName)
- }
- if res.State == nil {
- abnormalNodes = append(abnormalNodes, res.NodeName)
+ if res.State == nil {
+ abnormalNodes = append(abnormalNodes, res.NodeName)
+ }
}📝 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.
Suggested change
| -func NodesAndConditionFromResources(ress []lapi.Resource) ([]string, *csi.VolumeCondition) { | |
| +func NodesAndConditionFromResources(ress []lapi.ResourceWithVolumes) ([]string, *csi.VolumeCondition) { | |
| var allNodes, abnormalNodes []string | |
| for i := range ress { | |
| res := &ress[i] | |
| - allNodes = append(allNodes, res.NodeName) | |
| + // A resource is a CSI publish target if any of its volumes were created | |
| + // by ControllerPublishVolume, identified by the temporary-diskless-attach property. | |
| + if slices.ContainsFunc(res.Volumes, func(v lapi.Volume) bool { | |
| + createdFor, ok := v.Props[linstor.PropertyCreatedFor] | |
| + return ok && createdFor == linstor.CreatedForTemporaryDisklessAttach | |
| + }) { | |
| + allNodes = append(allNodes, res.NodeName) | |
| + } | |
| if res.State == nil { | |
| abnormalNodes = append(abnormalNodes, res.NodeName) | |
| // A resource is a CSI publish target if any of its volumes were created | |
| // by ControllerPublishVolume, identified by the temporary-diskless-attach property. | |
| isCsiTarget := slices.ContainsFunc(res.Volumes, func(v lapi.Volume) bool { | |
| createdFor, ok := v.Props[linstor.PropertyCreatedFor] | |
| return ok && createdFor == linstor.CreatedForTemporaryDisklessAttach | |
| }) | |
| if isCsiTarget { | |
| allNodes = append(allNodes, res.NodeName) | |
| if res.State == nil { | |
| abnormalNodes = append(abnormalNodes, res.NodeName) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff`
around lines 223 - 241, NodesAndConditionFromResources currently adds to
allNodes only when a resource has a volume with linstor.PropertyCreatedFor ==
linstor.CreatedForTemporaryDisklessAttach but still appends any res.NodeName to
abnormalNodes when res.State == nil, causing mismatch; update the abnormalNodes
append to use the same CSI-publish-target predicate (check res.Volumes with the
slices.ContainsFunc test against linstor.PropertyCreatedFor and
linstor.CreatedForTemporaryDisklessAttach) so you only record an abnormal node
if it is also included in allNodes, keeping both lists consistent.
Andrei Kvapil (kvaps)
deleted the
feat/linstor-csi-relocate-after-clone-restore
branch
March 16, 2026 12:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
001-rwx-validation.diff)001-relocate-after-clone-restore.diff) that includes:linstor.csi.linbit.com/relocateAfterCloneStorageClass parameter to relocate replicas to optimal nodes after clonesnap.linstor.csi.linbit.com/relocate-after-restoreVolumeSnapshotClass parameter to relocate replicas to optimal nodes after snapshot restoreUpstream PRs:
Test plan
Summary by CodeRabbit
Release Notes
New Features
Chores