Add volumeMode checks to clone operations #410

j-griffith · 2020-02-21T23:02:44Z

/kind cleanup

What this PR does / why we need it:

This PR add checks against the volume mode between source and newly created
clone of a pvc. If the source PVC does not match the requested volume mode of
the newly created clone the create operation will fail.

NONE

ShyamsundarR · 2020-02-22T14:19:13Z

pkg/controller/controller.go

@@ -819,6 +819,23 @@ func (p *csiProvisioner) getPVCSource(options controller.ProvisionOptions) (*csi
 		return nil, fmt.Errorf("claim in dataSource not bound or invalid")
 	}

+	if options.PVC.Spec.VolumeName == "my-block-pvc" {


remove debug logs

Resolved by the latest update

ShyamsundarR · 2020-02-23T12:07:57Z

pkg/controller/controller_test.go

@@ -3444,6 +3597,55 @@ func TestProvisionFromPVC(t *testing.T) {
 			},
 		}

+		nilModePV := &v1.PersistentVolume{


nilModePV is the same as pv above, and pv can be reused instead.

Yeah, I'll go through these again and verify the modes again; thanks.

Resolved by the latest update

pkg/controller/controller_test.go

ShyamsundarR · 2020-02-23T12:27:52Z

pkg/controller/controller_test.go

+		}
+
+		//clonePVName: "pvSrcBlockMode",
+		blkModePV := &v1.PersistentVolume{


(nit) blkModePV can be instead defined as,

blkModePV := pv.DeepCopy() blkModePV.Name = pvSrcBlockMode blkModePV.Spec.VolumeMode = &volumeModeBlock

And corresponding test cases can be updated to reflect tc.clonePVName as pvSrcBlockMode

This also removes the need to add blockModeClaim below.

Actually I noticed some bad behavior in the tests (that I didn't track down). It seems that we're missing cleanup steps in the tests and the result of trying to do something like you suggest above is things like invalid claims, and unexpected status on PVs. I'll take a look again, maybe it was something else I had incorrect, or maybe I can more easily identify the problem. Using debug statements in the catches though, I did notice that our tests don't always fail for the reasons we're expecting them to (sort of a problem), we set expectErr: true and let it rip, but it's not always failing for what we expect.

I think we should open an issue against the tests, and not necessarily failing for what we expect. I verified this when trying to copy the pv object into a block object, I suspect we might be doing this in other places.

I agree, there were a few flakes and incorrect errors, I have put up this PR as an initial cleanup for the tests. It does not ensure the errors are what is expected as per each test case though.

ShyamsundarR · 2020-02-24T15:09:33Z

Barring the issue discussed in this comment, which needs a separate resolution anyway, this PR is good to go, IMO.

msau42 · 2020-02-25T00:38:17Z

pkg/controller/controller_test.go

+			cloneSupport:   true,
+			expectErr:      true,
+		},
+		"provision with pvc source is block request nil mode": {


Can you also add more positive and negative test cases with nil mode?

jsafrane · 2020-02-27T09:32:04Z

@j-griffith, can you please rebase and rework the unit tests to the new style? It should be easier now.

xing-yang · 2020-02-28T15:26:18Z

pkg/controller/controller.go

+
+		}
+	}
+


These checks look good to me.

This PR add checks against the volume mode between source and newly created clone of a pvc. If the source PVC does not match the requested volume mode of the newly created clone the create operation will fail.

jsafrane · 2020-03-02T08:55:19Z

/lgtm
/approve

k8s-ci-robot · 2020-03-02T08:55:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [jsafrane]

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

…go_modules/google.golang.org/grpc-1.52.3 Bump google.golang.org/grpc from 1.52.1 to 1.52.3

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 21, 2020

k8s-ci-robot requested review from jsafrane and msau42 February 21, 2020 23:02

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 21, 2020

ShyamsundarR mentioned this pull request Feb 22, 2020

Cleanup and fix, clone unit tests #411

Merged

ShyamsundarR reviewed Feb 23, 2020

View reviewed changes

j-griffith force-pushed the add_mode_fs_checks_to_clone branch from 52382af to 552ea88 Compare February 24, 2020 00:41

ShyamsundarR approved these changes Feb 24, 2020

View reviewed changes

msau42 reviewed Feb 25, 2020

View reviewed changes

ShyamsundarR mentioned this pull request Feb 26, 2020

Add more tests for provsionsing from PVC as DataSource #409

Merged

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

xing-yang reviewed Feb 28, 2020

View reviewed changes

pkg/controller/controller.go

}

}

Copy link

Contributor

xing-yang Feb 28, 2020

Choose a reason for hiding this comment

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

These checks look good to me.

Add volumeMode checks to clone operations

08252d6

This PR add checks against the volume mode between source and newly created clone of a pvc. If the source PVC does not match the requested volume mode of the newly created clone the create operation will fail.

j-griffith force-pushed the add_mode_fs_checks_to_clone branch from 552ea88 to 08252d6 Compare February 28, 2020 22:52

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

k8s-ci-robot assigned jsafrane Mar 2, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2020

k8s-ci-robot merged commit efcaee7 into kubernetes-csi:master Mar 2, 2020

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

Merge pull request kubernetes-csi#410 from kubernetes-csi/dependabot/…

553a354

…go_modules/google.golang.org/grpc-1.52.3 Bump google.golang.org/grpc from 1.52.1 to 1.52.3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add volumeMode checks to clone operations #410

Add volumeMode checks to clone operations #410

j-griffith commented Feb 21, 2020

ShyamsundarR Feb 22, 2020

ShyamsundarR Feb 24, 2020

ShyamsundarR Feb 23, 2020

j-griffith Feb 23, 2020

ShyamsundarR Feb 24, 2020

ShyamsundarR Feb 23, 2020

j-griffith Feb 23, 2020

j-griffith Feb 24, 2020

ShyamsundarR Feb 24, 2020 •

edited

Loading

ShyamsundarR commented Feb 24, 2020

msau42 Feb 25, 2020

jsafrane commented Feb 27, 2020

xing-yang Feb 28, 2020

jsafrane commented Mar 2, 2020

k8s-ci-robot commented Mar 2, 2020

Add volumeMode checks to clone operations #410

Add volumeMode checks to clone operations #410

Conversation

j-griffith commented Feb 21, 2020

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

ShyamsundarR Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

ShyamsundarR commented Feb 24, 2020

Choose a reason for hiding this comment

jsafrane commented Feb 27, 2020

Choose a reason for hiding this comment

jsafrane commented Mar 2, 2020

k8s-ci-robot commented Mar 2, 2020

ShyamsundarR Feb 24, 2020 •

edited

Loading