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.

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

Jump to bottom

fix pvc and source PVC storageclass name comparison for pvc datasource #309

Merged
merged 1 commit into from
Aug 7, 2019
Merged

fix pvc and source PVC storageclass name comparison for pvc datasource #309

merged 1 commit into from
Aug 7, 2019

Conversation

Madhu-1
Copy link
Contributor

@Madhu-1 Madhu-1 commented Jul 1, 2019 •
edited
Loading

What type of PR is this?

This fixes the issue in storageclass name comparison for PVC creation with datasource=PersistanceVolumeClaim

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:

Which issue(s) this PR fixes:

Fixes #308

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

None

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 1, 2019
@k8s-ci-robot k8s-ci-robot requested review from jsafrane and saad-ali July 1, 2019 12:59
@k8s-ci-robot
Copy link
Contributor

Hi @Madhu-1. Thanks for your PR.

I'm waiting for a kubernetes-csi or 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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 1, 2019
@Madhu-1
Copy link
Contributor Author

Madhu-1 commented Jul 1, 2019

/assign @j-griffith
/assign @msau42

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jul 1, 2019
msau42
msau42 reviewed Jul 1, 2019
View reviewed changes
pkg/controller/controller.go
@@ -661,9 +661,9 @@ func (p *csiProvisioner) getPVCSource(options controller.ProvisionOptions) (*csi
if sourcePVC.ObjectMeta.DeletionTimestamp != nil {
return nil, fmt.Errorf("the PVC DataSource %s is currently being deleted", options.PVC.Spec.DataSource.Name)
}
if sourcePVC.Spec.StorageClassName != options.PVC.Spec.StorageClassName {
if *sourcePVC.Spec.StorageClassName != *options.PVC.Spec.StorageClassName {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be nil?

Also can we add a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msau42 yes this can be nil also. updated code and added unit testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

DataSource and rquested provisioned PVC are required to be in the same storage class. Also, I'm not sure I follow, how can the sourcePVC have a "nil" storage class? Once it's provisioned it will have a storage class, even if one wasn't specified at creation, the default class would be used and the object would have a storage class assigned to it no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@j-griffith this is a kind of safety check, will it cause any issue if we do nil check?

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't hurt at all, and might as well err on the side of caution.

@msau42
Copy link
Collaborator

msau42 commented Jul 2, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 2, 2019
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 2, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 2, 2019
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 2, 2019
@msau42
Copy link
Collaborator

msau42 commented Jul 2, 2019

@j-griffith actually does storageclass need to be the same for cloning? From a user's perspective, you at least want the content to be cloned, but do you require that all the underlying storage properties are exactly the same (ie hdd vs sdd?)

@ripulpatel
Copy link

@msau42 ,

Re

@j-griffith actually does storageclass need to be the same for cloning? From a user's perspective, you at least want the content to be cloned, but do you require that all the underlying storage properties are exactly the same (ie hdd vs sdd?)

We should not require properties exactly the same as dev-test clone could very well use cheaper media like hdd over it's original source's media which could be on faster media ssd.

@Madhu-1
Copy link
Contributor Author

Madhu-1 commented Jul 9, 2019

@j-griffith I have updated some of the test cases PTAL.

@Madhu-1
Copy link
Contributor Author

Madhu-1 commented Jul 9, 2019

@j-griffith I will leave it to you. As you have more information about it

j-griffith
j-griffith suggested changes Jul 9, 2019
View reviewed changes
Copy link
Contributor

@j-griffith j-griffith left a comment

Choose a reason for hiding this comment

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

Minor nits on the tests; also if you can fix the title s/comparion/comparison/ that'd be great.

pkg/controller/controller_test.go Outdated
clientSet = fakeclientset.NewSimpleClientset(claim, pv)

// Create a fake claim as our PVC DataSource
claim := fakeClaim(srcName, "fake-claim-uid", "1Gi", pvName, v1.ClaimBound, &invalidSCName)
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of "invalidSCName" here confuses things, you have a positive test (expected not to fail) where you create a DS and a new provision request using "invalidSCName" variable). That's somewhat confusing when glancing over the tests.

pkg/controller/controller_test.go Outdated
@@ -2145,7 +2146,9 @@ func runDeleteTest(t *testing.T, k string, tc deleteTestcase) {
func TestProvisionFromPVC(t *testing.T) {
var requestedBytes int64 = 1000
invalidSCName := "invalid-sc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Initially "invalidSCName" worked nicely because it was default or invalid; but if we explicitly add the SC names (which is a good idea) we should use names that aren't misleading; just like "test-sc-1" and "test-sc-2" that way it's not confusing on what we're expecting. There's actually nothing "invalid" about the sc name here (yes part of this is my original implementation, but we should fix it up here).

pkg/controller/controller_test.go Outdated
@@ -2145,7 +2146,9 @@ func runDeleteTest(t *testing.T, k string, tc deleteTestcase) {
func TestProvisionFromPVC(t *testing.T) {
var requestedBytes int64 = 1000
invalidSCName := "invalid-sc"
fakesc := "fake-sc"
Copy link
Contributor

Choose a reason for hiding this comment

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

See my note above, we should do something like "fakesc-1" and "fakesc-2" and avoid confusion and make it clear if there is a mismatch.

pkg/controller/controller_test.go Outdated
}

if tc.restoredVolSizeBig {
controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(nil, errors.New("source volume size is bigger than requested volume size")).Times(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the extra space in "source volume..."

pkg/controller/controller_test.go Outdated
// Create a fake claim as our PVC DataSource
claim := fakeClaim(srcName, "fake-claim-uid", "1Gi", pvName, v1.ClaimBound, &invalidSCName)
// Create a fake claim with invalid PV
claimInvalidPV := fakeClaim(invalidPVC, "fake-claim-uid", "1Gi", "pv-not-present", v1.ClaimBound, &invalidSCName)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be "invalidClaim" instead of "claimInvalidPV"

pkg/controller/controller_test.go
pv := &v1.PersistentVolume{
ObjectMeta: metav1.ObjectMeta{
Name: pvName,
},
Spec: v1.PersistentVolumeSpec{
PersistentVolumeSource: v1.PersistentVolumeSource{
CSI: &v1.CSIPersistentVolumeSource{
Driver: "test-driver",
Driver: driverName,
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that, thanks!

pkg/controller/controller_test.go Outdated
volOpts: controller.ProvisionOptions{
StorageClass: &storagev1.StorageClass{
ReclaimPolicy: &deletePolicy,
Parameters: map[string]string{},
},
PVName: pvName,
PVName: "test-name",
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well use a variable for this one as well (ie replace "test-name" with a variable).

@Madhu-1
fix storageclass comparion for pvc datasource
eaefaa3 
compare storage class string, instead of comparing
the string address.
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1
Copy link
Contributor Author

Madhu-1 commented Jul 10, 2019

@j-griffith addressed review comments, PTAL

@Madhu-1 Madhu-1 changed the title fix pvc and source PVC storageclass name comparion for pvc datasource fix pvc and source PVC storageclass name comparison for pvc datasource Jul 10, 2019
@Madhu-1
Copy link
Contributor Author

Madhu-1 commented Jul 16, 2019

@j-griffith anything needed on this one?

@j-griffith
Copy link
Contributor

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2019
@j-griffith
Copy link
Contributor

@Madhu-1 Sorry, I dropped this one; looks good to me now, thanks for making those changes.

@j-griffith
Copy link
Contributor

/assign @lpabon

@j-griffith j-griffith mentioned this pull request Jul 31, 2019
Merged
@msau42
Copy link
Collaborator

msau42 commented Jul 31, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: j-griffith, Madhu-1, 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2019
@j-griffith
Copy link
Contributor

/retest

@j-griffith
Copy link
Contributor

/unassign @lpabon

@msau42
Copy link
Collaborator

msau42 commented Aug 3, 2019

/retest

3 similar comments
@j-griffith
Copy link
Contributor

/retest

@msau42
Copy link
Collaborator

msau42 commented Aug 6, 2019

/retest

@msau42
Copy link
Collaborator

msau42 commented Aug 7, 2019

/retest

@msau42
Copy link
Collaborator

msau42 commented Aug 7, 2019

/test pull-kubernetes-csi-external-provisioner-1-13-on-kubernetes-1-13

@k8s-ci-robot k8s-ci-robot merged commit dcfa978 into kubernetes-csi:master Aug 7, 2019
@j-griffith
Copy link
Contributor

Wooohooo!

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023
@k8s-ci-robot
Merge pull request kubernetes-csi#309 from pohly/prow-update-master
d6c2e72 
master: update release-tools + OWNERS
@Madhu-1 Madhu-1 mentioned this pull request Apr 3, 2024
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@j-griffith j-griffith j-griffith requested changes

@jsafrane jsafrane Awaiting requested review from jsafrane

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@j-griffith j-griffith

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
7 participants
@Madhu-1 @k8s-ci-robot @msau42 @ripulpatel @j-griffith @humblec @lpabon