Implement CSI migration logic for volume resize #39

leakingtapan · 2019-04-25T22:46:10Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This is a continuation of the CSI migration for volume resizing. It implements migration logic for in-tree plugin to use CSI driver for volume resize:

updated external-resizer to support resizing in-tree volumes types as well as CSI volume.
updated external-resizer to handle raw block volume

See kubernetes/community#3624 for WIP migration design for volume resize.

Feature tracking issue: kubernetes/enhancements#625

/cc @ddebroy @davidz627 @gnufied @msau42

Which issue(s) this PR fixes:

Fixes #41

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Implement CSI migration logic for volume resizing

pkg/resizer/csi_resizer.go

davidz627 · 2019-05-29T18:20:51Z

@leakingtapan is there anything I can do to help unblock this PR?

ddebroy

A couple of comments added. Further, for looking up secrets around resizing, I think we need to call something along the lines of TranslateInTreeStorageClassToCSI to make sure any in-tree secrets (if necessary) is converted to the CSI equivalents. The storage class parameters names csi.storage.k8s.io/resizer-secret-name and csi.storage.k8s.io/resizer-secret-namespace are not expected to be present for the in-tree storage classes.

pkg/resizer/csi_resizer.go

leakingtapan · 2019-06-09T23:14:32Z

Finished the rest of the logic and implemented unit test. PTAL

davidz627

small testing and comment comments. Implementation LGTM

pkg/resizer/csi_resizer.go

pkg/resizer/csi_resizer_test.go

ddebroy

Added a suggestion around code structure. LGTM otherwise (modulo David's comments)

pkg/resizer/csi_resizer.go

leakingtapan · 2019-06-14T00:45:50Z

/hold

leakingtapan · 2019-06-14T04:38:31Z

After testing the new change with synchronization, I found a design flaw of using resizer key from annotation. When the new resizer key volume.kubernetes.io/storage-resizer is used, since both in-tree resizer and external resizer is trying to resize the volume at the same time, the external-resizer won't be able to read the resizer key since it's not even written by in-tree resizer yet.

The current workaround is fallback to provisioner key. WDYT?

/cc @davidz627 @ddebroy @gnufied

leakingtapan · 2019-06-23T22:49:19Z

@gnufied addressed comment and updated the condition to add PVC into work queue when the reszier annotation is added from update event. PTAL

gnufied

Mostly looks good. Just requested some minor changes in tests.

pkg/resizer/csi_resizer_test.go

leakingtapan · 2019-06-24T17:53:49Z

Updated PR

leakingtapan · 2019-06-24T17:54:23Z

/hold cancel

gnufied · 2019-06-24T17:56:09Z

/hold cancel
/lgtm

gnufied · 2019-06-24T18:04:34Z

/assign @msau42

msau42 · 2019-06-24T18:10:53Z

pkg/controller/controller.go

+	//
+	// We add the PVC into work queue when the new size is larger then the old size
+	// or when the resizer name annotation is missing from old PVC
+	// but present in the new PVC. This is needed for CSI migration


Can you describe more in detail why it is needed for migration? Even when migration is enabled, wouldn't the new and old sizes be different?

the problem is - when first time a migrated PVC is expanded, it does not yet have the annotation because annotation is only added by in-tree resizer when it receives a volume expansion request. So first update event that will be received by external-resizer will be ignored because it won't know how to support resizing of a "un-annotated" in-tree PVC.

When in-tree resizer does add the annotation, a second update even will be received and we add the pvc to workqueue. If annotation matches the registered driver name in csi_resizer object, we proceeds with expansion internally or we discard the PVC.

To determine if we should process the PVC, should we be comparing spec/status of size, instead of new/old PVC size? How do we retry resizing operation if it fails?

Regardless, if this is the way it needs to be, let's explain it in a comment.

Thx @gnufied for detailed explanation, added you reply into the comment.

We do compare spec/status size later - https://github.com/kubernetes-csi/external-resizer/blob/master/pkg/controller/controller.go#L213 but having new vs old size check in update prevents processing of events which aren't size related updates for PVC.

pkg/resizer/csi_resizer.go

msau42 · 2019-06-24T18:14:59Z

pkg/resizer/csi_resizer.go

+		return *resource.NewQuantity(newSizeBytes, resource.BinarySI), nodeResizeRequired, err
+	}
+
+	return *resource.NewQuantity(newSizeBytes, resource.BinarySI), false, err


Are there any block plugins that only implement node resize?

Not that Im aware of, could it be a case for local volume plugin? And how is this going to affect this highlighted line?

There is nothing in the csi spec that limits node expansion to only filesystem volumes. For a block plugin that only supports node resize, then is setting nodeResizeRequired=false going to cause nodeResize to never be called?

Should we instead always call node expansion (based off of nodeResizeRequired)? If the plugin gets a node expand call for a block device, can they just return success immediately?

I agree that the responsibility of determining whether node expansion is needed or not should be delegated to the plugin, but in control-plane the plugin may not have the necessary information to know that volume is being used in block device mode.

For now - I think it makes sense to return whatever nodeResizeRequired value was returned by the plugin. In future I am thinking we may want to update CSI spec to pass volume capability with ControllerExpandVolume request.

I think the question here is more related to node expand. It looks like we don't pass volume capability to node expansion, and maybe we need to add that?

I guess what a plugin could do now is check if the volume has a filesystem on it. If not, then assume it's raw block, otherwise assume it's filesystem.

Shouldn't a plugin be able to determine that given target_path in NodeExpandVolume RPC call is a device or a mounted filesystem?

agree with @gnufied here, ControllerExpand should have volume capability in the future so the driver can determine nodeResizeRequired better.

NodeExpandVolume may be determinable by the target_path but it might be nice to know the volume capability too.

Either way, for this PR I think just returning the plugins nodeResizeRequired makes sense instead of overwriting with false.

Unsure if we should be dynamically determining whether or not to call node expand. It is simpler to base it strictly on the plugin's capabilities. Especially across controller and node services, this makes testing/supporting version skew more complicated.

pkg/resizer/csi_resizer.go

leakingtapan · 2019-06-24T18:44:20Z

Added more comments @msau42

pkg/resizer/csi_resizer.go

pkg/controller/controller.go

* Using PVC annotation for sychronication between in-tree resizer and extternal resizer * Modify enqueue condition for resizer migration * Add unit tests

msau42 · 2019-06-25T22:40:36Z

/lgtm
/approve

k8s-ci-robot · 2019-06-25T22:40:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leakingtapan, 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:

~~OWNERS~~ [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 25, 2019

k8s-ci-robot requested review from davidz627, ddebroy, gnufied and msau42 April 25, 2019 22:46

leakingtapan mentioned this pull request May 3, 2019

Perform resizing of migrated PVCs #41

Closed

gnufied reviewed May 20, 2019

View reviewed changes

pkg/resizer/csi_resizer.go Outdated Show resolved Hide resolved

pkg/resizer/csi_resizer.go Show resolved Hide resolved

ddebroy reviewed Jun 3, 2019

View reviewed changes

pkg/resizer/csi_resizer.go Outdated Show resolved Hide resolved

pkg/resizer/csi_resizer.go Show resolved Hide resolved

davidz627 reviewed Jun 3, 2019

View reviewed changes

pkg/resizer/csi_resizer.go Outdated Show resolved Hide resolved

leakingtapan force-pushed the resize-migration branch from 586e2e0 to 8f75a0f Compare June 9, 2019 23:07

leakingtapan changed the title ~~[WIP] Implement CSI migration logic for volume resize~~ Implement CSI migration logic for volume resize Jun 9, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2019

davidz627 reviewed Jun 11, 2019

View reviewed changes

pkg/resizer/csi_resizer.go Outdated Show resolved Hide resolved

pkg/resizer/csi_resizer_test.go Outdated Show resolved Hide resolved

pkg/resizer/csi_resizer_test.go Outdated Show resolved Hide resolved

ddebroy reviewed Jun 12, 2019

View reviewed changes

pkg/resizer/csi_resizer.go Show resolved Hide resolved

leakingtapan force-pushed the resize-migration branch from 8f75a0f to 11eb8af Compare June 13, 2019 23:33

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2019

k8s-ci-robot requested review from davidz627, ddebroy and gnufied June 14, 2019 04:38

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2019

leakingtapan force-pushed the resize-migration branch from 83d5dff to 59fb87a Compare June 23, 2019 22:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2019

gnufied reviewed Jun 24, 2019

View reviewed changes

pkg/resizer/csi_resizer_test.go Outdated Show resolved Hide resolved

pkg/resizer/csi_resizer_test.go Outdated Show resolved Hide resolved

pkg/resizer/csi_resizer_test.go Outdated Show resolved Hide resolved

leakingtapan force-pushed the resize-migration branch from 59fb87a to b1b48db Compare June 24, 2019 17:53

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2019

k8s-ci-robot assigned gnufied Jun 24, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2019

k8s-ci-robot assigned msau42 Jun 24, 2019

msau42 reviewed Jun 24, 2019

View reviewed changes

leakingtapan force-pushed the resize-migration branch from b1b48db to 0870251 Compare June 24, 2019 18:44

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2019

msau42 reviewed Jun 24, 2019

View reviewed changes

pkg/resizer/csi_resizer.go Show resolved Hide resolved

gnufied reviewed Jun 24, 2019

View reviewed changes

pkg/controller/controller.go Outdated Show resolved Hide resolved

leakingtapan force-pushed the resize-migration branch from 0870251 to 74df227 Compare June 24, 2019 23:21

Implement CSI migration logic for volume resize

a380b39

* Using PVC annotation for sychronication between in-tree resizer and extternal resizer * Modify enqueue condition for resizer migration * Add unit tests

leakingtapan force-pushed the resize-migration branch from 74df227 to a380b39 Compare June 25, 2019 22:29

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2019

k8s-ci-robot merged commit 24780ba into kubernetes-csi:master Jun 25, 2019

leakingtapan mentioned this pull request Aug 15, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CSI migration logic for volume resize #39

Implement CSI migration logic for volume resize #39

leakingtapan commented Apr 25, 2019 •

edited

Loading

davidz627 commented May 29, 2019

ddebroy left a comment

leakingtapan commented Jun 9, 2019

davidz627 left a comment

ddebroy left a comment •

edited

Loading

leakingtapan commented Jun 14, 2019

leakingtapan commented Jun 14, 2019

leakingtapan commented Jun 23, 2019 •

edited

Loading

gnufied left a comment

leakingtapan commented Jun 24, 2019

leakingtapan commented Jun 24, 2019

gnufied commented Jun 24, 2019

gnufied commented Jun 24, 2019

msau42 Jun 24, 2019

gnufied Jun 24, 2019

msau42 Jun 24, 2019

leakingtapan Jun 24, 2019

gnufied Jun 24, 2019

msau42 Jun 24, 2019

leakingtapan Jun 24, 2019

msau42 Jun 24, 2019

gnufied Jun 24, 2019

msau42 Jun 24, 2019

gnufied Jun 24, 2019

davidz627 Jun 24, 2019

msau42 Jun 24, 2019

leakingtapan commented Jun 24, 2019

msau42 commented Jun 25, 2019

k8s-ci-robot commented Jun 25, 2019

Implement CSI migration logic for volume resize #39

Implement CSI migration logic for volume resize #39

Conversation

leakingtapan commented Apr 25, 2019 • edited Loading

davidz627 commented May 29, 2019

ddebroy left a comment

Choose a reason for hiding this comment

leakingtapan commented Jun 9, 2019

davidz627 left a comment

Choose a reason for hiding this comment

ddebroy left a comment • edited Loading

Choose a reason for hiding this comment

leakingtapan commented Jun 14, 2019

leakingtapan commented Jun 14, 2019

leakingtapan commented Jun 23, 2019 • edited Loading

gnufied left a comment

Choose a reason for hiding this comment

leakingtapan commented Jun 24, 2019

leakingtapan commented Jun 24, 2019

gnufied commented Jun 24, 2019

gnufied commented Jun 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leakingtapan commented Jun 24, 2019

msau42 commented Jun 25, 2019

k8s-ci-robot commented Jun 25, 2019

leakingtapan commented Apr 25, 2019 •

edited

Loading

ddebroy left a comment •

edited

Loading

leakingtapan commented Jun 23, 2019 •

edited

Loading