Initialize CSI client before checking publish capability - #3276
Initialize CSI client before checking publish capability#3276toughlex-donatas wants to merge 1 commit into
Conversation
|
Thank you for contributing! It looks like your commit message is missing a DCO sign-off, We require all commit messages to have a There is no need to open a new pull request, but to fix this (and make CI pass), Unfortunately, it's not possible to do so through GitHub's web UI, so this needs You can find some instructions in the output of the DCO check (which can be found Steps to do so "roughly" come down to:
Let me know if you need help or more detailed instructions! |
There was a problem hiding this comment.
🟡 Not ready to approve
The new blocks introduce non-gofmt indentation/trailing whitespace that may fail formatting/lint checks and should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes a CSI controller capability detection edge case where PublishVolume/UnpublishVolume could incorrectly no-op after a Swarm manager restart/leader change because the plugin client (and thus ControllerGetCapabilities) hadn’t been initialized yet.
Changes:
- Initialize the CSI controller client (
Client()) before checkingp.publisherinPublishVolume. - Initialize the CSI controller client (
Client()) before checkingp.publisherinUnpublishVolume.
File summaries
| File | Description |
|---|---|
| manager/csi/plugin.go | Ensures CSI controller capabilities are loaded before deciding whether to call controller-side publish/unpublish RPCs. |
Review details
Suppressed comments (1)
manager/csi/plugin.go:242
- The new error-handling block is indented with spaces (and the following blank line contains trailing whitespace), which deviates from gofmt formatting used throughout this file; this can cause gofmt/lint checks to fail and makes the code inconsistent.
c, err := p.Client(ctx)
if err != nil {
return err
}
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| c, err := p.Client(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if !p.publisher { | ||
| return nil, nil | ||
| } |
|
I see there's minor lining issue as well; |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3276 +/- ##
==========================================
- Coverage 14.73% 14.72% -0.01%
==========================================
Files 200 200
Lines 93077 93027 -50
==========================================
- Hits 13712 13699 -13
+ Misses 78019 77991 -28
+ Partials 1346 1337 -9 🚀 New features to boost your workflow:
|
Signed-off-by: Donatas Lučiūnas <donatas@toughlex.com>
82675ab to
45f47e9
Compare
|
Thank you, I fixed sign-off and whitespace |
Summary
Initialize the CSI plugin client before checking whether the plugin supports
PUBLISH_UNPUBLISH_VOLUME.Problem
The
publisherfield is populated during CSI client initialization, when SwarmKit callsControllerGetCapabilities.After a Swarm manager restart or leader change, a new plugin wrapper starts with:
However,
PublishVolumechecks this field before callingClient():At this point,
falsemay mean that the plugin has not been initialized yet, rather than that it does not support controller publish/unpublish operations.As a result, SwarmKit can incorrectly treat volume publication as successful without calling
ControllerPublishVolumeand without receiving a publish context.For the AWS EBS CSI driver, this produces the following failure sequence:
availablebecause no attach request was made.NodeStageVolumefails with:The volume then appears as
in usein Docker, while it is not attached in AWS.Fix
Call
Client()before checkingp.publisher:This ensures that plugin capabilities are loaded before SwarmKit decides whether controller-side publish operations are supported.
Reproduction
in use, while the EBS volume remainsavailable.Creating a new temporary CSI volume before scheduling the service also works around the issue because the create path initializes the CSI client and loads controller capabilities.
Expected behavior
SwarmKit should initialize the CSI plugin before checking its controller capabilities.
When the driver supports
PUBLISH_UNPUBLISH_VOLUME,ControllerPublishVolumeshould be called and its publish context should be passed to the node-side CSI operations.Testing
Tested with:
Before this change, existing EBS volumes could become stuck in an inconsistent state:
After this change, the EBS volume is attached normally and the service starts successfully.