Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix panic because VolumeSpec may be nil in volume reconstruction scenario #74493

Merged
merged 1 commit into from
Feb 26, 2019
Merged

Fix panic because VolumeSpec may be nil in volume reconstruction scenario #74493

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/volume/util/operationexecutor/operation_generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

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.

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @gnufied

Copy link
Member

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

Copy link
Contributor

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.

pluginName = csi.CSIPluginName
} else {
pluginName = volumeToUnmount.PluginName
Expand Down