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.

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

Enables resizing of block volumes. #81429

Merged
merged 2 commits into from Aug 24, 2019

Conversation

@huffmanca
Copy link
Contributor

commented Aug 14, 2019

Enables resizing of block volumes.

What type of PR is this?
/kind bug

What this PR does / why we need it:
Fixes #79990

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Resolves an issue that prevented block volumes from being resized.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Hi @huffmanca. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@gnufied

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

/ok-to-test

if !fsVolume {
klog.V(5).Infof("Block mode volume needn't to check file system resize request")
return
}
if processedVolumesForFSResize.Has(string(uniqueVolumeName)) {

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 14, 2019

Member

Just to be on safe side, for in-tree drivers I think we should still skip calling fs resize on the node if volume is of type raw block.

The other thing is - we are going to need some e2e tests for this.

This comment has been minimized.

Copy link
@wongma7

wongma7 Aug 15, 2019

Contributor

What if migration is enabled?

in-tree: no Pending condition on PVC if mode Block
CSI: always Pending condition on PVC
migrated CSI: ?

This comment has been minimized.

Copy link
@wongma7

wongma7 Aug 15, 2019

Contributor

Our e2e test expects that Block volumes don't get the condition
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/testsuites/volume_expand.go#L190
I'm running the tests for aws migration.
So the test needs to know if the plugin is CSI or in-tree or migrated CSI which will be ugly.

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 15, 2019

Member

That e2e is broken for CSI raw block volumes, no matter what we do here right? Plugin already knows if it is CSI or in-tree. I guess in-tree migrated CSI is tricky. But I think this ugliness will go away once we merge - container-storage-interface/spec#381

So our options are:

  1. Keep in-tree drivers behaviour same as CSI, but since in CSI too we are moving towards skipping NodeExpandVolume for raw block devices, this seems like a step backward and will cause unnecessary churn.
  2. Live with ugliness in e2e until we container-storage-interface/spec#381 is merged and fix the in-tree <-> csi drivers.

I am leaning towards #2. And yeah - I think we should not skip calling fs resize if driver is in-tree but migration is enabled for it.

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 19, 2019

Member

I have opened #81611 to fix this problem. I think the condition checking there added very little value.

@gnufied

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

/sig storage
/kind bug
/priority important-soon

@gnufied

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

/assign

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XS labels Aug 20, 2019

@huffmanca huffmanca force-pushed the huffmanca:resize_block_volume branch 2 times, most recently from 694a0b7 to 31f65fb Aug 20, 2019

}

e1, e2 := expandVolumeFunc.OperationFunc()
if e1 != nil {

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 20, 2019

Member

This could be written as one line:

if e1 != nil || e2 != nil {
    t.Fatalf("unable to call volume expansion: %v, %v", e1, e2)
}

@huffmanca huffmanca force-pushed the huffmanca:resize_block_volume branch from 31f65fb to 3c41a07 Aug 20, 2019

@bswartz

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

This PR doesn't work for me. Kubelet is mistakenly deciding that my pods have mounted the volumes readonly and thus doesn't call the CSI NodeExpandVolume RPC.

The issue is bad logic in the mountedReadOnlyByContainer() function. This function doesn't work for raw blocks.

@@ -1649,7 +1596,7 @@ func (og *operationGenerator) GenerateExpandInUseVolumeFunc(
resizeOptions.DevicePath = volumeToMount.DevicePath
dmp, err := volumeAttacher.GetDeviceMountPath(volumeToMount.VolumeSpec)

This comment has been minimized.

Copy link
@bswartz

bswartz Aug 21, 2019

Contributor

This is definitely the wrong path for a raw block volume. It doesn't even exist.

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 22, 2019

Member

should be fixed now.

@bswartz

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

/assign

@gnufied gnufied force-pushed the huffmanca:resize_block_volume branch from 5c6a793 to 9a42cb9 Aug 22, 2019

Included resizing for CSI-based block volumes.
Perform a no-op when volume is of type raw block

Fix bug with checking volume mounts for readonly

@gnufied gnufied force-pushed the huffmanca:resize_block_volume branch from 9a42cb9 to 55be1b3 Aug 22, 2019

@gnufied

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

/retest

@gnufied gnufied force-pushed the huffmanca:resize_block_volume branch 2 times, most recently from 03ac9fd to d62d8e8 Aug 22, 2019

@gnufied

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

/assign @msau42

if processedVolumesForFSResize.Has(string(uniqueVolumeName)) {
// File system resize operation is a global operation for volume,
// so we only need to check it once if more than one pod use it.
return
}
if mountedReadOnlyByPod(podVolume, pod) {
// We need to be consistent with what we check.

This comment has been minimized.

Copy link
@msau42

msau42 Aug 22, 2019

Member

Is this a todo? I don't understand the comment

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 22, 2019

Member

This is not a todo. I was just saying - the check we were doing here was overly complex. We should use same value that we use for deciding whether to format the disk and when calling resize during mount process. There is no need for online resizing to check containers etc. I will reword this though.

if processedVolumesForFSResize.Has(string(uniqueVolumeName)) {
// File system resize operation is a global operation for volume,
// so we only need to check it once if more than one pod use it.
return
}
if mountedReadOnlyByPod(podVolume, pod) {
// We need to be consistent with what we check.
if volumeSpec.ReadOnly {

This comment has been minimized.

Copy link
@msau42

msau42 Aug 22, 2019

Member

Should we pass readonly down to the plugin and let the plugin decide if resize should be skipped? Or actually let it fail? What does it mean to resize a read only volume?

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 22, 2019

Member

yeah that is a good idea and ties into discussion we had this yesterday in CSI spec meeting. Currently NodeExpandVolumeRequest can't carry over that information and @bswartz wanted readonly volumes to be expandable on nodes too. In his case - all the plugin would do is, rescan iscsi lun.

I think, we should not be making that change right now in this PR.

This comment has been minimized.

Copy link
@msau42

msau42 Aug 22, 2019

Member

I'm wondering if volumeSpec.ReadOnly is sufficient enough, or do we also need to be checking container readonly too?

This comment has been minimized.

Copy link
@msau42

msau42 Aug 22, 2019

Member

The other question I have is more philosophical. If a user requests expansion on a read-only volume, should we actually ignore it and return success? We didn't actually expand the filesystem to what they requested.

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 22, 2019

Member

For CSI - in the long run we want the decision to be taken by the plugin(SP) and not the CO. For in-tree implementations we currently support, doing resizing of readonly volumes with filesystem on it - could result in filesystem corruption.

I'm wondering if volumeSpec.ReadOnly is sufficient enough, or do we also need to be checking container readonly too?

The container readonly is handled by CRI and is an option for bind mount. I think checking volumeSpec.Readonly should be sufficient for our use case.

This comment has been minimized.

Copy link
@msau42

msau42 Aug 22, 2019

Member

Shouldn't the resizing attempt fail if the volume is attached/mounted as read only?

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 23, 2019

Member

In many cases - expanding the filesystem does not care how volume is mounted. Expanding(resize2fs/xfsgrow) the volume sometimes just takes device path (not the mount path).

This comment has been minimized.

Copy link
@msau42

msau42 Aug 23, 2019

Member

So the PV.readOnly field is only used for determining if volumes should be attached readonly: #70505

Is that the only readOnly setting you want to skip here?

This comment has been minimized.

Copy link
@gnufied

gnufied Aug 23, 2019

Member

Yes - so attached as ReadOnly and mounted(NodePublished) as ReadOnly are two different things. Reading the linked issue, it seems like we are not setting PersistentVolume.spec.CSIPersistentVolumeSource.readOnly anywhere but once we do - this code should be updated to respect that and not resize a readonly attached volume.

Again this is with the caveat that - once we have option to pass ReadOnly to NodeExpandRequest - we will have to update the call to pass it as a flag and let SP decide, how best to handle it.

@gnufied

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

/retest

@gnufied gnufied force-pushed the huffmanca:resize_block_volume branch from d62d8e8 to 9dbe0b3 Aug 23, 2019

@bswartz

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

Okay this patch works for me. All flavors of resize are successful.

I haven't tried multi attached resize yet, I expect that to fail still, but this is good to merge IMO.

@msau42

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

/retest
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 23, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huffmanca, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gnufied

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit f105fef into kubernetes:master Aug 24, 2019

24 checks passed

cla/linuxfoundation gnufied authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.