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

Add check for volumecontent source in CreateVolume #148

Merged

k8s-ci-robot merged 2 commits into kubernetes-csi:master from Madhu-1:add-parent-check

Feb 13, 2020

Contributor

Madhu-1 commented Feb 12, 2020

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:
If the volume is already found during create volume request,we also need to check the volume content source id valid or not.
If the returning error message doesn't require formatting, use errors.New().

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot added release-note-none cncf-cla: yes needs-ok-to-test labels

Contributor

k8s-ci-robot commented Feb 12, 2020

Hi @Madhu-1. Thanks for your PR.

I'm waiting for a kubernetes-csi 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 requested review from saad-ali and vladimirvivien

February 12, 2020 07:48

k8s-ci-robot added the size/M label

Contributor Author

Madhu-1 commented Feb 12, 2020

/assign @jsafrane
/assign @msau42

k8s-ci-robot assigned jsafrane and msau42

Madhu-1 force-pushed the add-parent-check branch from abfad08 to f0d3ade Compare

February 12, 2020 08:11

Contributor

jsafrane commented Feb 12, 2020

/ok-to-test

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels

jsafrane reviewed

View reviewed changes

pkg/hostpath/controllerserver.go Outdated

-              		}
-              		if srcVolume := contentSource.GetVolume(); srcVolume != nil {
-              			err = loadFromVolume(srcVolume.GetVolumeId(), path)

Contributor

jsafrane Feb 12, 2020

Why extra space?

jsafrane reviewed

View reviewed changes

pkg/hostpath/controllerserver.go Outdated

+              					}
+              				default:
+              					err = fmt.Errorf("%v not a proper volume source", volumeSource)

Contributor

jsafrane Feb 12, 2020

nit: extra blank line

Madhu-1 force-pushed the add-parent-check branch 2 times, most recently from 30e5e00 to 87399b0 Compare

February 12, 2020 08:58

Contributor Author

Madhu-1 commented Feb 12, 2020

@jsafrane removed extra blank lines

jsafrane reviewed

View reviewed changes

pkg/hostpath/controllerserver.go Outdated

               		// request
               		if exVol.VolSize >= int64(req.GetCapacityRange().GetRequiredBytes()) {
               			// exisiting volume is compatible with new request and should be reused.

Contributor

jsafrane Feb 12, 2020

nit: extra blank line?

pkg/hostpath/controllerserver.go Outdated

+              				default:
+              					err = fmt.Errorf("%v not a proper volume source", volumeSource)
+              				}
+              				return nil, status.Error(codes.InvalidArgument, err.Error())

Contributor

jsafrane Feb 12, 2020

This always returns error even if VolumeContentSource was fine and err == nil

Contributor Author

Madhu-1 Feb 12, 2020

yes correct, fixed now

pkg/hostpath/controllerserver.go Outdated Show resolved Hide resolved

Madhu-1 force-pushed the add-parent-check branch 3 times, most recently from 3054761 to f16a8b2 Compare

February 12, 2020 09:12

jsafrane reviewed

View reviewed changes

Contributor

jsafrane left a comment

I should have spotted this earlier, but can you fix the nested ifs?

pkg/hostpath/controllerserver.go Outdated

+              					err = fmt.Errorf("%v not a proper volume source", volumeSource)
+              				}
+              				if err != nil {
+              					return nil, status.Error(codes.InvalidArgument, err.Error())

Contributor

jsafrane Feb 12, 2020

In spirit of container-storage-interface/spec#412, the first two cases should return AlreadyExists, while the default branch should return InvalidArgument. Moving return to the cases would be IMO better.

Contributor Author

Madhu-1 Feb 12, 2020

updated code to return already exists

pkg/hostpath/controllerserver.go Outdated

Comment on lines 130 to 132

               		if exVol.VolSize >= int64(req.GetCapacityRange().GetRequiredBytes()) {
               			// exisiting volume is compatible with new request and should be reused.
+              			if req.GetVolumeContentSource() != nil {

Contributor

jsafrane Feb 12, 2020

I am not happy with nested ifs here.

IMO it would be better to do negative conditions:

if (size does not match){
  return AlreadyExists
}
if (source is not empty)
  ... return AlreadyExists / InvalidArgument on mismatch ...
}

// everything matches
return &csi.CreateVolumeResponse{}

Contributor Author

Madhu-1 Feb 12, 2020

as I need to check

content source is empty or not
if not need empty check is it snapshot or volume or not
if it is a snapshot or volume check id is matching
nothing content source is neither snapshot nor volume return error

updated with if else if and else check

Contributor Author

Madhu-1 Feb 12, 2020

now i got it, making changes

Madhu-1 force-pushed the add-parent-check branch 5 times, most recently from 443ca59 to b9ac9cb Compare

February 12, 2020 09:56

k8s-ci-robot added the needs-rebase label

Madhu-1 force-pushed the add-parent-check branch from b9ac9cb to bad2de2 Compare

February 12, 2020 10:16

k8s-ci-robot removed the needs-rebase label

Madhu-1 force-pushed the add-parent-check branch from bad2de2 to 5fe37e0 Compare

February 12, 2020 10:18

humblec reviewed

View reviewed changes

pkg/hostpath/controllerserver.go Outdated Show resolved Hide resolved

Madhu-1 force-pushed the add-parent-check branch from 5fe37e0 to 9a41fb9 Compare

February 13, 2020 05:38

Contributor Author

Madhu-1 commented Feb 13, 2020

@jsafrane @humblec PTAL

jsafrane reviewed

View reviewed changes

pkg/hostpath/controllerserver.go Outdated

+              			switch volumeSource.Type.(type) {
+              			case *csi.VolumeContentSource_Snapshot:
+              				if volumeSource.GetSnapshot() != nil && exVol.ParentSnapID != volumeSource.GetSnapshot().GetSnapshotId() {
+              					err = status.Error(codes.AlreadyExists, "existing volume source snapshot id not matching")

Contributor

jsafrane Feb 13, 2020

nit: return right here, no need to pass err around. This applies to all cases + default.

Contributor Author

Madhu-1 Feb 13, 2020

done.

Madhu-1 added 2 commits

February 13, 2020 14:53


          Replace fmt.Errorf with errors.New

c099734

If the returning error message doesnot
require formatting, use errors.New().

the error message should not be capitalized,
replace it with smaller case


          Add check for volumecontent source in CreateVolume

366d752

If the volume is already found,we also need to
check the volumecountent source id valid or not.

Madhu-1 force-pushed the add-parent-check branch from 9a41fb9 to 366d752 Compare

February 13, 2020 09:23

Contributor

jsafrane commented Feb 13, 2020

/lgtm
/approve
Thanks for the patience :-)

k8s-ci-robot added the lgtm label

Contributor

k8s-ci-robot commented Feb 13, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, Madhu-1

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

k8s-ci-robot added the approved label

Contributor Author

Madhu-1 commented Feb 13, 2020

/retest

k8s-ci-robot merged commit b9f759c into kubernetes-csi:master

msau42 mentioned this pull request

release 1.3.0 preparations #155

Merged

Madhu-1 mentioned this pull request

REQUEST: New membership for Madhu-1 kubernetes/org#4867

Closed

9 tasks

TerryHowe pushed a commit to TerryHowe/csi-driver-host-path that referenced this pull request


          Merge pull request kubernetes-csi#148 from dobsonj/add-checkpathcmd-t…

7b96bea

…o-prow

prow.sh: enable -csi.checkpathcmd option in csi-sanity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cncf-cla: yes lgtm ok-to-test release-note-none size/M