Extend allowed DataSources #220

j-griffith · 2019-01-29T00:42:52Z

This PR extends the supported DataSources handled by the
external-provisioner to include PVCs (Clone) and Populators (external
CRDs).

It removes the restriction of of the provisioner erroring when a
DataSource.Kind of anything other than VolumeSnapshot is supplied (the
Kubernetes API currently enforces this anyway via it's verifictaion
checks) and adds handling for a DataSource.Kind of
PersistentVolumeClaim.

Addresses: Issue #172

Does this PR introduce a user-facing change?:

Adds the ability to handle PVC as a DataSource to enable cloning for plugins that support it.

pkg/controller/controller.go

humblec · 2019-03-19T08:35:09Z

pkg/controller/controller_test.go

+	//var timeNow = time.Now().UnixNano()
+	//var metaTimeNowUnix = &metav1.Time{
+	//		Time: time.Unix(0, timeNow),
+	//	}


@j-griffith can we remove this commented code ?

Oops, yes sorry!

j-griffith · 2019-04-11T21:07:49Z

Cleaned up with the rebase, should be good to go.

msau42 · 2019-04-17T17:12:25Z

/assign @saad-ali

humblec · 2019-04-20T12:05:53Z

pkg/controller/controller.go

+	if err != nil {
+		return nil, fmt.Errorf("error getting PVC %s from api server: %v", options.PVC.Spec.DataSource.Name, err)
+	}
+	if string(sourcePVC.Status.Phase) != "Bound" {


@j-griffith I dont know whether this can land on a race , that said, by the time getPVCSource returns its available/ready , someone released it or vice versa. @jsafrane thoughts?

Yes, I think that's possible, I guess you'd want to add a shared mutex for the source PVC here?

humblec · 2019-04-20T12:06:44Z

pkg/controller/controller.go

-		if *(options.PVC.Spec.DataSource.APIGroup) != snapshotAPIGroup {
-			return nil, fmt.Errorf("the PVC source does not belong to the right APIGroup. Expected %s, Got %s", snapshotAPIGroup, *(options.PVC.Spec.DataSource.APIGroup))
+
+		switch options.PVC.Spec.DataSource.Kind {


@j-griffith we already have a getVolumeContentSource, can we make use of it?

Hi @humblec thanks for pointing that out. Actually you'll notice I do utilize that helper function, I added the fetch for PVC to it and call it when we actually pull the content.

This particular check in this section was a here to validate the options and "fail fast" if they were invalid, as well as set a variable so that the provision stage later would have a flag to "getContentSource" or not. If we want to consolidate things into a single helper, that's certainly possible, but it seemed like following the flow we already have for Snapshots was a good idea here.

humblec · 2019-04-20T12:10:46Z

pkg/controller/controller.go

+	}
+	capacity := options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
+	requestedSize := capacity.Value()
+	if requestedSize < int64(sourcePVC.Spec.Size()) {


@j-griffith There could be scenarios where resizer is processing an expansion request in the backend. So, may be checking current PVC's size and capacity could help as well.

Sorry, i'm not sure I follow; what do you mean the "current PVC's size and capacity"? The destination has yet to be created at this stage, so the only object that's relevant is the sourcePVC and the requested size; do you mean a resize in progress on the specified source volume?

do you mean a resize in progress on the specified source volume?

Exactly! thats what I meant, in short, the resizer took the request and working with the driver to get it into desired state.

Shouldn't checking sourcePVC.Spec alone be sufficient since it will be the largest possible value (it will be the desired size)? Validation prevents setting that value to a smaller value right?

@j-griffith There could be scenarios where resizer is processing an expansion request in the backend. So, may be checking current PVC's size and capacity could help as well.

@humblec so I've thought about this a bit; and yes there's certainly a potential for a race here. That being said, we have that condition across other operations already (snapshots for example) and the challenge that's been discussed is coordinating any sort of locking/checks across the different control planes. Not to mention as @jsafrane pointed out, even across CSI provisioner nodes.

Note that this PR does fetch the source PVC object and check its capacity before processing/issuing the provision call, so I think it's doing basically what you're suggesting.

IMO this is a larger problem that needs solved independent of the cloning implementation. Whether that's new phases on the PVC, some other event tracking, or (as I think we've done thus far) the "storage device plugins problem". It sounds like this is something that's been brought up in other context and is being discussed, but we have some long term work to do here I think.

j-griffith · 2019-05-31T03:43:02Z

/retest

saad-ali

/lgtm
/approve
/hold
To give @humblec a chance to review

k8s-ci-robot · 2019-06-10T23:32:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: j-griffith, saad-ali

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~~ [saad-ali]

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

humblec · 2019-06-11T02:42:08Z

@j-griffith as discussed in above review comments (#220 (comment)) i see possibilities of couple of race conditions ( pvc status/capacity..etc) , however these are not really specific to this PR rather its a general problem which we have to address with a thorough design. Lets attack it later in a different discussion/PR. LGTM from my side on this PR . Thanks !!

@saad-ali I am good to get this in and thanks for giving chance for final review from my side!

j-griffith · 2019-06-11T03:07:30Z

@j-griffith as discussed in above review comments (#220 (comment)) i see possibilities of couple of race conditions ( pvc status/capacity..etc) , however these are not really specific to this PR rather its a general problem which we have to address with a thorough design. Lets attack it later in a different discussion/PR. LGTM from my side on this PR . Thanks !!

@saad-ali I am good to get this in and thanks for giving chance for final review from my side!

Thanks @humblec I agree with you, we'll need to consider these in a broader scope.

msau42 · 2019-06-12T17:08:49Z

/hold cancel

j-griffith · 2019-06-12T18:15:05Z

/retest

This PR extends the supported DataSources handled by the external-provisioner to include PVCs (Clone). It removes the restriction of of the provisioner erroring when a DataSource.Kind of anything other than VolumeSnapshot is supplied and adds handling for a DataSource.Kind of PersistentVolumeClaim. This implements the ability to specify a clone, and pass that dataSource on to plugins that report support for the capability. Addresses: Issue kubernetes-csi#172

j-griffith · 2019-06-12T18:58:28Z

Had to update the call I added to NewCSIProvisioner in the test file after rebase.

msau42 · 2019-06-12T20:04:50Z

/lgtm

msau42 · 2019-06-20T01:25:31Z

/kind feature

6613c3980 Merge pull request kubernetes-csi#223 from sunnylovestiramisu/update 0e7ae993d Update k8s image repo url 77e47cce8 Merge pull request kubernetes-csi#222 from xinydev/fix-dep-version 155854b09 Fix dep version mismatch 8f839056a Merge pull request kubernetes-csi#221 from sunnylovestiramisu/go-update 1d3f94dd5 Update go version to 1.20 to match k/k v1.27 e322ce5e5 Merge pull request kubernetes-csi#220 from andyzhangx/fix-golint-error b74a51209 test: fix golint error aa61bfd0c Merge pull request kubernetes-csi#218 from xing-yang/update_csi_driver 7563d1963 Update CSI_PROW_DRIVER_VERSION to v1.11.0 a2171bef0 Merge pull request kubernetes-csi#216 from msau42/process cb9878261 Merge pull request kubernetes-csi#217 from msau42/owners a11216e47 add new reviewers and remove inactive reviewers dd9867540 Add step for checking builds b66c08249 Merge pull request kubernetes-csi#214 from pohly/junit-fixes b9b6763bd filter-junit.go: fix loss of testcases when parsing Ginkgo v2 JUnit d4277839f filter-junit.go: preserve system error log 38e11468f prow.sh: publish individual JUnit files as separate artifacts git-subtree-dir: release-tools git-subtree-split: 6613c3980d1e418bebb7bc49d64c977cfff85671

Add CHANGELOG for v2.2.0

6613c398 Merge pull request kubernetes-csi#223 from sunnylovestiramisu/update 0e7ae993 Update k8s image repo url 77e47cce Merge pull request kubernetes-csi#222 from xinydev/fix-dep-version 155854b0 Fix dep version mismatch 8f839056 Merge pull request kubernetes-csi#221 from sunnylovestiramisu/go-update 1d3f94dd Update go version to 1.20 to match k/k v1.27 e322ce5e Merge pull request kubernetes-csi#220 from andyzhangx/fix-golint-error b74a5120 test: fix golint error aa61bfd0 Merge pull request kubernetes-csi#218 from xing-yang/update_csi_driver 7563d196 Update CSI_PROW_DRIVER_VERSION to v1.11.0 a2171bef Merge pull request kubernetes-csi#216 from msau42/process cb987826 Merge pull request kubernetes-csi#217 from msau42/owners a11216e4 add new reviewers and remove inactive reviewers dd986754 Add step for checking builds b66c0824 Merge pull request kubernetes-csi#214 from pohly/junit-fixes b9b6763b filter-junit.go: fix loss of testcases when parsing Ginkgo v2 JUnit d4277839 filter-junit.go: preserve system error log 38e11468 prow.sh: publish individual JUnit files as separate artifacts git-subtree-dir: release-tools git-subtree-split: 6613c3980d1e418bebb7bc49d64c977cfff85671

k8s-ci-robot requested review from davidz627 and sbezverk January 29, 2019 00:43

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 29, 2019

j-griffith force-pushed the extend_datasources branch from 0627c2b to 979102b Compare January 29, 2019 01:20

j-griffith force-pushed the extend_datasources branch from 979102b to b9148ef Compare February 8, 2019 16:52

j-griffith mentioned this pull request Feb 8, 2019

Propose KEP to extend allowed DataSource entries to include PVC kubernetes/enhancements#642

Merged

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2019

j-griffith force-pushed the extend_datasources branch from 63c64c3 to b5e553f Compare February 15, 2019 16:43

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 15, 2019

msau42 mentioned this pull request Feb 15, 2019

REQUEST: New membership for j-griffith kubernetes/org#495

Closed

6 tasks

rollandf reviewed Feb 19, 2019

View reviewed changes

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

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

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

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

j-griffith force-pushed the extend_datasources branch 2 times, most recently from 4d5f58b to 0ef237e Compare February 21, 2019 20:45

nixpanic mentioned this pull request Mar 11, 2019

[DNM] [WIP] Add support for smart cloning ceph/ceph-csi#248

Closed

humblec reviewed Mar 19, 2019

View reviewed changes

j-griffith force-pushed the extend_datasources branch from 0ef237e to 839edba Compare April 11, 2019 23:54

k8s-ci-robot assigned saad-ali Apr 17, 2019

humblec reviewed Apr 20, 2019

View reviewed changes

j-griffith force-pushed the extend_datasources branch from 839edba to 33ea9a5 Compare May 30, 2019 18:39

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label May 30, 2019

j-griffith force-pushed the extend_datasources branch from 33ea9a5 to 1dd93e3 Compare May 30, 2019 19:48

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

j-griffith force-pushed the extend_datasources branch from 1dd93e3 to 47c105a Compare May 31, 2019 02:47

saad-ali approved these changes Jun 10, 2019

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 10, 2019

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

j-griffith mentioned this pull request Jun 11, 2019

Implement Cloning for csi-hostpath driver kubernetes-csi/csi-driver-host-path#58

Merged

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 12, 2019

j-griffith force-pushed the extend_datasources branch from 47c105a to c7b783a Compare June 12, 2019 18:40

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

k8s-ci-robot assigned msau42 Jun 12, 2019

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

k8s-ci-robot merged commit 22bb640 into kubernetes-csi:master Jun 12, 2019

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 20, 2019

msau42 mentioned this pull request Jun 20, 2019

External provisioner misses VolumeSource support for CLONE_VOLUMES #172

Closed

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023

Merge pull request kubernetes-csi#220 from jsafrane/changelog-2.2

97411fa

Add CHANGELOG for v2.2.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend allowed DataSources #220

Extend allowed DataSources #220

j-griffith commented Jan 29, 2019 •

edited by msau42

Loading

humblec Mar 19, 2019

j-griffith Apr 22, 2019

j-griffith commented Apr 11, 2019 •

edited

Loading

msau42 commented Apr 17, 2019

humblec Apr 20, 2019

j-griffith Apr 22, 2019

humblec Apr 20, 2019

j-griffith Apr 22, 2019

humblec Apr 20, 2019

j-griffith Apr 22, 2019

humblec May 2, 2019 •

edited

Loading

saad-ali May 31, 2019

j-griffith May 31, 2019 •

edited

Loading

j-griffith commented May 31, 2019

saad-ali left a comment

k8s-ci-robot commented Jun 10, 2019

humblec commented Jun 11, 2019

j-griffith commented Jun 11, 2019

msau42 commented Jun 12, 2019

j-griffith commented Jun 12, 2019

j-griffith commented Jun 12, 2019

msau42 commented Jun 12, 2019

msau42 commented Jun 20, 2019

Extend allowed DataSources #220

Extend allowed DataSources #220

Conversation

j-griffith commented Jan 29, 2019 • edited by msau42 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-griffith commented Apr 11, 2019 • edited Loading

msau42 commented Apr 17, 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

humblec May 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-griffith May 31, 2019 • edited Loading

Choose a reason for hiding this comment

j-griffith commented May 31, 2019

saad-ali left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 10, 2019

humblec commented Jun 11, 2019

j-griffith commented Jun 11, 2019

msau42 commented Jun 12, 2019

j-griffith commented Jun 12, 2019

j-griffith commented Jun 12, 2019

msau42 commented Jun 12, 2019

msau42 commented Jun 20, 2019

j-griffith commented Jan 29, 2019 •

edited by msau42

Loading

j-griffith commented Apr 11, 2019 •

edited

Loading

humblec May 2, 2019 •

edited

Loading

j-griffith May 31, 2019 •

edited

Loading