Skip to content

Mark PVC for FS resize iff it's not Block #47

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

Closed
wongma7 wants to merge 1 commit into from
Closed

Mark PVC for FS resize iff it's not Block #47

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ func (ctrl *resizeController) resizePVC(pvc *v1.PersistentVolumeClaim, pv *v1.Pe
return err
}

if fsResizeRequired {
if fsResizeRequired && !(pvc.Spec.VolumeMode != nil && *pvc.Spec.VolumeMode == v1.PersistentVolumeBlock) {
Copy link
Contributor

@gnufied gnufied Aug 13, 2019

Choose a reason for hiding this comment

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

We explicitly made a choice to not skip NodeExpandVolume if volume type is of block type. The reasoning is - the spec does not stipulate that NodeExpandVolume will not be called if volume is of block type. Only a plugin can decide whether NodeExpandVolume can be skipped or not.

This is pending on a fix in kubelet - kubernetes/kubernetes#79990 too.

Copy link
Author

@wongma7 wongma7 Aug 13, 2019

Choose a reason for hiding this comment

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

Okay, I still think there is a fix to be made here, to not put FileSystemResizePending & FileSystemResizeRequired on block volumes. NodeExpand can still be called but the user-facing things should be accurate

Copy link
Contributor

@gnufied gnufied Aug 13, 2019

Choose a reason for hiding this comment

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

Yeah I think the naming of conditions is bit wrong and pre-dates CSI spec . I will look into renaming those conditions to be more accurate.

Copy link
Author

@wongma7 wongma7 Aug 13, 2019

Choose a reason for hiding this comment

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

Hm, how on the plugin side can I decide whether to skip NodeExpandVolume? When it's called, I only get volume ID and path, it doesn't tell me if the CO intends for the volume to be used as block or not. If it's block, I'd like to no-op.

Copy link
Author

@wongma7 wongma7 Aug 13, 2019

Choose a reason for hiding this comment

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

ControllerExpandVolume won't know if the volume is block or not either, so it can't return false for node expansion required

Copy link
Collaborator

@msau42 msau42 Aug 13, 2019

Choose a reason for hiding this comment

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

I thought pd driver skipped NodeExpand based on volume capability but I was wrong: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#372

cc @davidz627

Copy link
Contributor

@gnufied gnufied Aug 13, 2019

Choose a reason for hiding this comment

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

I think I am in favour of adding supplying volume capabilities into both ControllerExpandVolume and NodeExpandVolume. NodeExpandVolume has a clear use case where we might not want to fs resize the volume even if it has file system on it - if volume is being used in block mode.

Copy link

@leakingtapan leakingtapan Aug 15, 2019

Choose a reason for hiding this comment

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

This is a similar change to the one I made during resize migration: #39 (comment)

If we want SP to decide if to do NodeExpandVolume, not sure if we still need this change here

// Resize volume succeeded and need to resize file system by kubelet, mark it as file system resizing required.
return ctrl.markPVCAsFSResizeRequired(pvc)
}
Expand Down