-
Notifications
You must be signed in to change notification settings - Fork 38.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix panic because VolumeSpec may be nil in volume reconstruction scenario #74493
Conversation
@@ -732,7 +732,7 @@ func (og *operationGenerator) GenerateUnmountVolumeFunc( | |||
podsDir string) (volumetypes.GeneratedOperations, error) { | |||
|
|||
var pluginName string | |||
if useCSIPlugin(og.volumePluginMgr, volumeToUnmount.VolumeSpec) { | |||
if volumeToUnmount.VolumeSpec != nil && useCSIPlugin(og.volumePluginMgr, volumeToUnmount.VolumeSpec) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should return an error in ConstructVolumeSpec() instead if we can't support reconstruction, like here.
This will cause the Unmount() to happen, which will fix the filesystem-based paths, however block-based paths will not work because UnmountDevice() is still not called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does raw block reconstruction have a similar issue? We don't have tests for raw block reconstruction: #74545
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @gnufied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed some more with @jingxu97, and she pointed out that even if we return error in ConstructVolumeSpec(), the volumeSpec will still be nil here, so we still need this check.
It looks like this check was introduced as part of CSI migration. 1) This should be feature gated, and 2) can useCSIPlugin
return false if volume spec is nil? Does that make sense for all operations? cc @davidz627
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for 1) I've submitted a PR to make sure we do the migration feature gate check before doing anything else.
For 2) I think I need to do some more investigation and address it in the CSI Migration Reconstruction design.
/lgtm We can followup on the remaining items, which I will open issues for |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cofyc, msau42 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold |
/hold cancel |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #74384
Special notes for your reviewer:
Does this PR introduce a user-facing change?: