Repository navigation
feat(linstor): bump linstor-csi to v1.10.6 with Protocol-C dual-attach fix - #2496
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates LINSTOR CSI from v1.10.5 to v1.10.6; adds best-effort replica relocation after clone/restore (configurable) and conditional DRBD protocol=C overrides to support dual-attach (RWX) with marker-based reversion logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Driver as CSI Driver
participant Client as LINSTOR Client
participant Auto as Autoplacer
participant RD as Resource Definition
Note over Driver: Clone/Restore flow (when relocate enabled)
Driver->>Driver: Check RelocateAfterClone / RelocateAfterRestore
Driver->>Client: QuerySizeInfo (current diskful nodes)
Client-->>Driver: Current placement
Driver->>Auto: Compute optimal node targets
Auto-->>Driver: Preferred targets
Driver->>Client: Create diskless targets on preferred nodes
Client-->>Driver: Diskless targets created
Driver->>Client: Trigger Migrate from current -> preferred
Client-->>Driver: Migration started / per-pair status
Driver->>RD: Set Aux/csi-relocation-triggered (idempotent)
RD-->>Driver: Property set
sequenceDiagram
participant Driver as CSI Driver
participant Client as LINSTOR Client
participant RD as Resource Definition
Note over Driver: Attach (RWX dual-attach)
Driver->>Client: GetResourceDefinition / view properties
Client-->>Driver: RD properties (protocol, rg-level fallback)
Driver->>Driver: getEffectiveDrbdProtocol(vol)
alt Protocol is A or B
Driver->>RD: Set `protocol=C` override and CSI marker
RD-->>Driver: Override applied
end
Driver->>Client: Enable allow-two-primaries
Client-->>Driver: Allow-two-primaries set
Note over Driver: Detach (when resInUse → 1)
Driver->>Client: Remove allow-two-primaries
Driver->>RD: Check CSI marker
alt Marker present
Driver->>RD: Remove `protocol` override and marker
RD-->>Driver: Reverted
else Marker absent
Note over Driver: Preserve operator configuration
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cdb32b8 to
3f2902c
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
dosubot
Bot
added
the
size/L
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request upgrades the LINSTOR CSI driver to version 1.10.6 and introduces a critical fix for KubeVirt live migrations. By dynamically applying a Protocol=C override during dual-attach operations, the driver now correctly handles DRBD resources that are configured for asynchronous replication, preventing migration failures caused by protocol mismatches. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the LINSTOR CSI version from v1.10.5 to v1.10.6 in the Makefile and Dockerfile. A review comment identifies a critical issue where patch files mentioned in the PR description are missing from the diff, which will cause the build to fail during the application step.
| FROM golang:1.25 AS builder | ||
|
|
||
| ARG VERSION=v1.10.5 | ||
| ARG VERSION=v1.10.6 |
There was a problem hiding this comment.
The pull request description mentions a new patch 002-protocol-c-override-for-dual-attach.diff and a regenerated 001-relocate-after-clone-restore.diff, but these files are missing from the pull request diff. Please ensure that all new and modified patch files are included in the packages/system/linstor/images/linstor-csi/patches/ directory. Without them, the build will fail during the git apply step or the intended fix will not be applied to the new version.
github-actions
Bot
added
area/storage
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff (1)
81-81: Optional: align Aux-marker style with patch 002 for consistency.Patch 002 defines
PropertyCsiProtocolOverride = lc.NamespcAuxiliary + "/csi-protocol-override"as an exported constant inpkg/linstor/const.go. This patch keepspropertyRelocationTriggeredas a file-local string literal"Aux/csi-relocation-triggered"inpkg/client/linstor.go. Functionally identical, but two new Aux markers introduced in the same PR with different styles is a small inconsistency.Since the PR notes upstreaming is already in flight as a draft and this hunk is "regenerated with no logic change", treat this purely as nice-to-have — best fixed in the upstream draft to avoid Cozystack-side patch churn.
🤖 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` at line 81, The Aux marker string "Aux/csi-relocation-triggered" is declared as a file-local literal (propertyRelocationTriggered) while another Aux marker (PropertyCsiProtocolOverride) is exported from pkg/linstor/const.go; make the style consistent by moving propertyRelocationTriggered into the shared constants file as an exported constant (e.g., PropertyCsiRelocationTriggered) alongside PropertyCsiProtocolOverride, update all references to use the new constant name instead of the local variable, and keep the original string value unchanged.
🤖 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/002-protocol-c-override-for-dual-attach.diff`:
- Around line 24-36: The current path in the attach flow silently skips
installing the Protocol=C override when getEffectiveDrbdProtocol(ctx, volId)
returns an error, which leaves Protocol A/B resources broken for live-migration;
update the logic in the attach code that calls getEffectiveDrbdProtocol (the
block that sets rdProps and uses s.log) to either (a) escalate the log/metric
when protoErr != nil and still install the override unconditionally (set
rdProps[linstor.PropertyDrbdNetProtocol] = "C" and
rdProps[linstor.PropertyCsiProtocolOverride] = "yes") on error, or (b) if you
prefer conditional behavior, emit a higher-severity metric/alert and return the
error so the caller can retry; ensure you reference getEffectiveDrbdProtocol,
rdProps, linstor.PropertyDrbdNetProtocol and linstor.PropertyCsiProtocolOverride
when making the change so the marker-based Detach will still remove the override
later.
- Around line 53-71: The current Detach path silently skips removing Protocol=C
and the CSI override when s.client.ResourceDefinitions.Get fails; change it so
the cleanup is best-effort: regardless of getErr, include
linstor.PropertyDrbdNetProtocol and linstor.PropertyCsiProtocolOverride in
deleteProps (since DeleteProps is a no-op for missing keys) before calling
s.client.ResourceDefinitions.Modify, or alternatively return getErr instead of
proceeding; update the logic around s.client.ResourceDefinitions.Get,
deleteProps, and the Modify call so the override + marker are always attempted
to be removed (use linestor.PropertyCsiProtocolOverride and
linstor.PropertyDrbdNetProtocol as the identifying symbols).
- Around line 79-104: Update the doc comment for getEffectiveDrbdProtocol to
explicitly state that the function only checks resource-definition and
resource-group properties and does not consult resource-connection (per-peer)
overrides; mention that connection-level DrbdOptions/Net/protocol settings have
higher precedence in LINSTOR and therefore may cause the helper to miss an
existing Protocol=C configured at the connection level (reference
getEffectiveDrbdProtocol and linstor.PropertyDrbdNetProtocol so maintainers can
locate the behavior).
---
Nitpick comments:
In
`@packages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diff`:
- Line 81: The Aux marker string "Aux/csi-relocation-triggered" is declared as a
file-local literal (propertyRelocationTriggered) while another Aux marker
(PropertyCsiProtocolOverride) is exported from pkg/linstor/const.go; make the
style consistent by moving propertyRelocationTriggered into the shared constants
file as an exported constant (e.g., PropertyCsiRelocationTriggered) alongside
PropertyCsiProtocolOverride, update all references to use the new constant name
instead of the local variable, and keep the original string value unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52954d95-461d-49da-8fac-a8433c5f312b
📒 Files selected for processing (4)
packages/system/linstor/Makefilepackages/system/linstor/images/linstor-csi/Dockerfilepackages/system/linstor/images/linstor-csi/patches/001-relocate-after-clone-restore.diffpackages/system/linstor/images/linstor-csi/patches/002-protocol-c-override-for-dual-attach.diff
| + proto, protoErr := s.getEffectiveDrbdProtocol(ctx, volId) | ||
| + if protoErr != nil { | ||
| + s.log.WithError(protoErr).WithField("volume", volId). | ||
| + Warn("failed to determine effective DRBD protocol; skipping Protocol=C override for dual-attach") | ||
| + } else if proto == "A" || proto == "B" { | ||
| + s.log.WithFields(logrus.Fields{ | ||
| + "volume": volId, | ||
| + "protocol": proto, | ||
| + }).Info("installing Protocol=C override on resource-definition for dual-attach") | ||
| + | ||
| + rdProps[linstor.PropertyDrbdNetProtocol] = "C" | ||
| + rdProps[linstor.PropertyCsiProtocolOverride] = "yes" | ||
| + } |
There was a problem hiding this comment.
Degraded path on protocol-lookup failure silently leaves Protocol=A/B resources broken for live migration.
If getEffectiveDrbdProtocol returns an error (controller hiccup, transient 5xx, etc.), Attach logs a warning and falls through, installing only allow-two-primaries=yes. For Protocol-A/B resources this is exactly the failure mode this patch is designed to fix — drbdadm adjust will fail with "Protocol C required" (errno 139) and live migration will not proceed, with no further indication that the override was skipped due to a lookup error rather than a deliberate "already C" decision.
Two minor improvements worth considering before this hits production:
- Surface the skip in metrics or a higher log level (the current
Warnwill be lost in Attach noise). - Optionally make the override unconditional when
protoErr != nil(i.e., default-safe to install Protocol=C when we can't prove it's already C). Since Detach is gated by the marker, an unnecessary override would be removed cleanly on the next Detach.
Not a blocker — the previous behavior was equally broken — but the warning-and-continue path deserves a follow-up.
🤖 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/002-protocol-c-override-for-dual-attach.diff`
around lines 24 - 36, The current path in the attach flow silently skips
installing the Protocol=C override when getEffectiveDrbdProtocol(ctx, volId)
returns an error, which leaves Protocol A/B resources broken for live-migration;
update the logic in the attach code that calls getEffectiveDrbdProtocol (the
block that sets rdProps and uses s.log) to either (a) escalate the log/metric
when protoErr != nil and still install the override unconditionally (set
rdProps[linstor.PropertyDrbdNetProtocol] = "C" and
rdProps[linstor.PropertyCsiProtocolOverride] = "yes") on error, or (b) if you
prefer conditional behavior, emit a higher-severity metric/alert and return the
error so the caller can retry; ensure you reference getEffectiveDrbdProtocol,
rdProps, linstor.PropertyDrbdNetProtocol and linstor.PropertyCsiProtocolOverride
when making the change so the marker-based Detach will still remove the override
later.
| + deleteProps := []string{linstor.PropertyAllowTwoPrimaries} | ||
| + | ||
| + // Drop the Protocol=C override only if Attach installed it (gated by | ||
| + // our marker), so an operator-set Protocol property on the | ||
| + // resource-definition is preserved. | ||
| + rd, getErr := s.client.ResourceDefinitions.Get(ctx, volId) | ||
| + if getErr != nil { | ||
| + log.WithError(getErr).Warn("failed to fetch resource-definition props; skipping Protocol=C override removal") | ||
| + } else if rd.Props[linstor.PropertyCsiProtocolOverride] != "" { | ||
| + log.Info("removing Protocol=C override from resource-definition") | ||
| + deleteProps = append(deleteProps, | ||
| + linstor.PropertyDrbdNetProtocol, | ||
| + linstor.PropertyCsiProtocolOverride, | ||
| + ) | ||
| + } | ||
| + | ||
| + err = s.client.ResourceDefinitions.Modify(ctx, volId, lapi.GenericPropsModify{ | ||
| + DeleteProps: deleteProps, | ||
| + }) |
There was a problem hiding this comment.
Detach cleanup: Get-then-Modify is fine, but consider tightening the failure path.
When s.client.ResourceDefinitions.Get fails on line 58, Detach logs a warning and proceeds with only allow-two-primaries in deleteProps. If the marker actually was present, the Protocol=C override and the Aux marker leak onto the RD until the next Attach/Detach pair — and the next Attach won't re-add anything because getEffectiveDrbdProtocol will then report proto == "C", so the override-marker pair could persist indefinitely.
Two possible refinements:
- On
getErr != nil, return the error (let the caller retry) rather than silently dropping the cleanup. - Or explicitly best-effort delete
PropertyDrbdNetProtocol+PropertyCsiProtocolOverridewhenever they're present, regardless ofGetoutcome (LINSTORDeletePropsfor a non-existent key is a no-op).
Idempotency-wise, leaving Protocol=C set is harmless for correctness (it's stricter than A/B), but it does mask any subsequent operator-driven downgrade to A/B at the RD level. Minor.
🤖 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/002-protocol-c-override-for-dual-attach.diff`
around lines 53 - 71, The current Detach path silently skips removing Protocol=C
and the CSI override when s.client.ResourceDefinitions.Get fails; change it so
the cleanup is best-effort: regardless of getErr, include
linstor.PropertyDrbdNetProtocol and linstor.PropertyCsiProtocolOverride in
deleteProps (since DeleteProps is a no-op for missing keys) before calling
s.client.ResourceDefinitions.Modify, or alternatively return getErr instead of
proceeding; update the logic around s.client.ResourceDefinitions.Get,
deleteProps, and the Modify call so the override + marker are always attempted
to be removed (use linestor.PropertyCsiProtocolOverride and
linstor.PropertyDrbdNetProtocol as the identifying symbols).
| +// getEffectiveDrbdProtocol returns the DRBD network protocol ("A", "B" or "C") | ||
| +// that LINSTOR will hand to drbdadm for the given resource. Resource-definition | ||
| +// properties take precedence; otherwise the value is inherited from the | ||
| +// resource-group. An empty string is returned when neither level sets the | ||
| +// property (LINSTOR's compiled-in default is "C"). | ||
| +func (s *Linstor) getEffectiveDrbdProtocol(ctx context.Context, volId string) (string, error) { | ||
| + rd, err := s.client.ResourceDefinitions.Get(ctx, volId) | ||
| + if err != nil { | ||
| + return "", err | ||
| + } | ||
| + | ||
| + if v, ok := rd.Props[linstor.PropertyDrbdNetProtocol]; ok { | ||
| + return v, nil | ||
| + } | ||
| + | ||
| + if rd.ResourceGroupName == "" { | ||
| + return "", nil | ||
| + } | ||
| + | ||
| + rg, err := s.client.ResourceGroups.Get(ctx, rd.ResourceGroupName) | ||
| + if err != nil { | ||
| + return "", err | ||
| + } | ||
| + | ||
| + return rg.Props[linstor.PropertyDrbdNetProtocol], nil | ||
| +} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
LINSTOR DRBD options precedence: resource-definition vs resource-connection vs resource-group for DrbdOptions/Net/protocol
💡 Result:
In LINSTOR, DRBD options like DrbdOptions/Net/protocol follow the general properties inheritance hierarchy where settings closer to the actual resource take precedence. The precedence order is: resource-connection (highest, specific to node pairs) > resource-definition > resource-group (lowest). Resource-connection options override those from resource-definition, which override resource-group. This aligns with the documented rule: "the value 'closer' to the resource or volume wins. [...] resource definition settings take precedence over resource group settings." Resource-connections, being more specific (per node pair), have highest precedence for net options like protocol, as evidenced by Piraeus documentation setting DrbdOptions/Net/protocol on node connections and LINSTOR API supporting resource_conn_modify for properties.
Citations:
- 1: https://linbit.com/drbd-user-guide/linstor-guide-1_0-en/
- 2: https://piraeus.io/docs/stable/reference/linstornodeconnection
- 3: linstor rd sp -h: Help did not list all usable keys LINBIT/linstor-client#37
- 4: https://github.com/cozystack/website/blob/e6a03717/content/en/docs/operations/stretched/linstor.md
🏁 Script executed:
# First, let's examine the patch file to understand the broader context
fd -t f '002-protocol-c-override-for-dual-attach.diff' packages/system/Repository: cozystack/cozystack
Length of output: 159
🏁 Script executed:
# Let's also check if there are any LINSTOR-related documentation or code that explains the precedence
rg -l 'DrbdOptions' packages/system/ --type patchRepository: cozystack/cozystack
Length of output: 93
🏁 Script executed:
# Search for property precedence or protocol resolution logic
rg 'precedence|override' packages/system/ -i --type patch -A 3 -B 3Repository: cozystack/cozystack
Length of output: 93
🏁 Script executed:
# Read the patch file directly to understand the context
cat packages/system/linstor/images/linstor-csi/patches/002-protocol-c-override-for-dual-attach.diffRepository: cozystack/cozystack
Length of output: 5785
Add a note to the doc comment clarifying that resource-connection level overrides are not consulted.
The helper only checks resource-definition and resource-group properties, not connection-level overrides. Per the LINSTOR precedence model, resource-connection options (set per peer) actually have higher precedence than resource-definition settings. This means if an operator has already set DrbdOptions/Net/protocol on a specific connection, that value wins over whatever the resource-definition specifies.
The current implementation could lead to installing an unnecessary Protocol=C override on the resource-definition even if some connections are already running Protocol C via connection-level configuration. Document this limitation in the helper's comment so future maintainers understand the detection only surveys RD/RG-level settings.
🤖 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/002-protocol-c-override-for-dual-attach.diff`
around lines 79 - 104, Update the doc comment for getEffectiveDrbdProtocol to
explicitly state that the function only checks resource-definition and
resource-group properties and does not consult resource-connection (per-peer)
overrides; mention that connection-level DrbdOptions/Net/protocol settings have
higher precedence in LINSTOR and therefore may cause the helper to miss an
existing Protocol=C configured at the connection level (reference
getEffectiveDrbdProtocol and linstor.PropertyDrbdNetProtocol so maintainers can
locate the behavior).
…h fix Bump linstor-csi to v1.10.6 and add an out-of-tree patch that overrides DrbdOptions/Net/protocol to C on the resource-definition whenever a volume is attached to a second node with allow-two-primaries (the live-migration flow), and reverts it on detach. Without the patch, any KubeVirt live migration of a VM whose volume sits on a Protocol-A/B resource (for example a "replicated-async" StorageClass) ends up in a permanent evacuation loop: drbdadm adjust on the satellite rejects allow-two-primaries with "Protocol C required" (errno 139), the migration fails, and KubeVirt retries until manual intervention. The override is set at the resource-definition level so it covers every connection, including diskless TieBreaker peers, and is tagged with an Aux marker so Detach reverts only the override we installed. The 001 relocate-after-clone patch was regenerated against v1.10.6 (context shift only, no logic change). Verified end-to-end on dev5: live migration of a KubeVirt VM whose PVC sits on a Protocol-A resource-group now succeeds in a single Attach. Patch is upstreamed as draft PR piraeusdatastore/linstor-csi#435. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
3f2902c to
073fb16
Compare
|
Successfully created backport PR for |
The auto-generated changelog only listed #2459/#2467 (velero-configmap Role move). Five additional PRs were backported and merged into release-1.3 between then and the v1.3.1 tag (2026-04-28): - #2471/#2524 - fix(api): IDOR in TenantNamespace Get/Watch - #2496/#2505 - feat(linstor): linstor-csi v1.10.6 (Protocol-C dual-attach) - #2462/#2511 - fix(etcd): remove destructive post-upgrade hook - #2421/#2491 - fix(kamaji): memory limits + startup probe - #2498/#2518 - build(linstor): wire linstor-gui into root build target Update the release date to match the actual tag (2026-04-28), rewrite the intro paragraph, and add @kvaps to contributors. Signed-off-by: Myasnikov Daniil <[email protected]>
2 participants
What this PR does
Bumps
linstor-csifrom v1.10.5 to v1.10.6 and ships an out-of-tree patch that fixes live migration of KubeVirt VMs whose volumes sit on a DRBD Protocol-A/B resource (e.g. areplicated-asyncStorageClass).Problem
DRBD requires Protocol C whenever
allow-two-primaries=yesis enabled. Operators commonly opt into Protocol A on a per-resource-group basis for async / WAN replication, which silently breaks every subsequent live migration of consumers of those volumes:drbdadm adjustrejects the second-attach withProtocol C required(errno 139), KubeVirt's evacuation loop retries indefinitely, and the VM stays pinned to the source node.Change
LINSTOR_CSI_VERSION1.10.5 → 1.10.6 (Makefile + Dockerfile default).002-protocol-c-override-for-dual-attach.diff: whenAttachinstallsallow-two-primaries=yeson the resource-definition during a second attach, it also installsDrbdOptions/Net/protocol=Cas an override on the resource-definition. The override applies to every connection (including diskless TieBreaker peers, where a per-pair override would still leave one connection broken). It is tagged withAux/csi-protocol-override=yessoDetachremoves only the override this driver installed, leaving any operator-setProtocolproperty on the resource-definition untouched.001-relocate-after-clone-restore.diffregenerated against v1.10.6 (context shift only, no logic change — the old patch hunks no longer aligned cleanly).Verification
make image-linstor-csibuilds successfully on linux/amd64 with both patches applied.(node) Failed to adjust DRBD resource ... Protocol C required.Upstream
Upstreamed as draft PR piraeusdatastore/linstor-csi#435.
Compatibility
Attachis a no-op once the override is installed.Release note
Summary by CodeRabbit
New Features
Chores