FC volume plugin: Support WWID for volume identifier #48741

mtanino · 2017-07-11T00:21:35Z

What this PR does / why we need it:

This PR adds World Wide Identifier (WWID) parameter to FCVolumeSource as an unique volume identifier.

Which issue this PR fixes: fixes #48639

Special notes for your reviewer:

Release note:

FC volume plugin: Support WWID for volume identifier

k8s-ci-robot · 2017-07-11T00:21:43Z

Hi @mtanino. Thanks for your PR.

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

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. I understand the commands that are listed here.

mtanino · 2017-07-11T00:22:08Z

/ok-to-test

k8s-ci-robot · 2017-07-11T00:22:22Z

@mtanino: you can't request testing unless you are a kubernetes member.

In response to this:

/ok-to-test

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.

jsafrane · 2017-07-11T08:33:39Z

pkg/api/validation/validation.go

@@ -648,15 +648,22 @@ func validateISCSIVolumeSource(iscsi *api.ISCSIVolumeSource, fldPath *field.Path

 func validateFCVolumeSource(fc *api.FCVolumeSource, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if len(fc.TargetWWNs) < 1 {
-		allErrs = append(allErrs, field.Required(fldPath.Child("targetWWNs"), ""))
+	if len(fc.TargetWWNs) < 1 && len(fc.WWIDs) < 1 {


I'd appreciate an unit test for this.

jsafrane · 2017-07-11T08:33:46Z

pkg/api/validation/validation.go

-	} else {
-		if *fc.Lun < 0 || *fc.Lun > 255 {
-			allErrs = append(allErrs, field.Invalid(fldPath.Child("lun"), fc.Lun, validation.InclusiveRangeError(0, 255)))
+	if len(fc.TargetWWNs) != 0 && len(fc.WWIDs) != 0 {


I'd appreciate an unit test for this.

jsafrane · 2017-07-11T08:34:18Z

/ok-to-test
/assign @rootfs

jsafrane · 2017-07-11T10:53:20Z

staging/src/k8s.io/api/core/v1/types.go

+	// Optional: FC volume World Wide Identifiers (WWIDs)
+	// Either WWIDs or TargetWWNs and Lun must be set, but not both simultaneously.
+	// +optional
+	WWIDs []string `json:"WWIDs,omitempty" protobuf:"bytes,5,rep,name=WWIDs"`


json fields must be lowercase, i.e. probably json:"wwids,omitempty"

'+ the same for protobuf field

jsafrane · 2017-07-11T10:54:53Z

/test pull-kubernetes-verify

jsafrane · 2017-07-11T11:36:35Z

make verify is complaining about these files:

W0711 11:24:43.198]    M pkg/apis/apps/v1beta1/zz_generated.conversion.go
W0711 11:24:43.198]    M pkg/apis/batch/v1/zz_generated.conversion.go
W0711 11:24:43.198]    M pkg/apis/extensions/v1beta1/zz_generated.conversion.go

These should be modified only when apps/v1beta1/types.go, batch/v1/types.go or extensions/v1beta1/types.go change and I do not see any such change in your PR. I guess your hack/update-all.sh regenerated them from some reason on your system. Try to run update-all.sh again or remove them manually from the patch.

mtanino · 2017-07-11T16:14:25Z

@jsafrane
Thank you for the review. I updated the patch based on your comments.

These should be modified only when apps/v1beta1/types.go, batch/v1/types.go or
extensions/v1beta1/types.go change and I do not see any such change in your PR. I guess your
hack/update-all.sh regenerated them from some reason on your system.

I'm not sure but after hack/update-all.sh again, these three files were still regenerated.
Therefore, I checkout original file for these three zz_generated.conversion.go to avoid to commit and then pushed above commit.

jsafrane · 2017-07-11T17:09:50Z

@mtanino, now it looks much better, please run hack/update-bazel.sh and that should be all from the process view. And real review can start :-)

jsafrane · 2017-07-11T17:16:09Z

pkg/volume/fc/fc_util.go

+				disk, err := io.EvalSymlinks(dev_id + name)
+				if err != nil {
+					glog.V(2).Infof("fc: failed to find a corresponding disk from symlink[%s], error %v", dev_id+name, err)
+					return "", ""


It seems to me that continue would be more appropriate here - there is a weird symlink there, but that should not stop the plugin from trying the other links in the directory.

In my understanding, fc_path is unique under /dev/by-id.
Therefore, if symlink evaluation or checkDiskExist of scsi-3600508b400105e210000900000490000 fails, checking other links seems meaning less.

Also seems strings.Contains causes unexpected behavior because there is a possibility that choose partitioned device such as sda1, sda2, etc. I think we should check whether name matches fc_path completely.

ex.

lrwxrwxrwx 1 root root 9 Jul 7 19:47 scsi-3600508b400105e210000900000490000 -> ../../sdb lrwxrwxrwx 1 root root 9 Jul 7 19:45 scsi-35000c500676fb68f -> ../../sda lrwxrwxrwx 1 root root 10 Jul 7 19:45 scsi-35000c500676fb68f-part1 -> ../../sda1 lrwxrwxrwx 1 root root 10 Jul 7 19:45 scsi-35000c500676fb68f-part2 -> ../../sda2 lrwxrwxrwx 1 root root 10 Jul 7 19:45 scsi-35000c500676fb68f-part3 -> ../../sda3 lrwxrwxrwx 1 root root 10 Jul 7 19:45 scsi-35000c500676fb68f-part4 -> ../../sda4 lrwxrwxrwx 1 root root 10 Jul 7 19:45 scsi-35000c500676fb68f-part5 -> ../../sda5 lrwxrwxrwx 1 root root 9 Jul 7 19:45 wwn-0x5000c500676fb68f -> ../../sda lrwxrwxrwx 1 root root 10 Jul 7 19:45 wwn-0x5000c500676fb68f-part1 -> ../../sda1 lrwxrwxrwx 1 root root 10 Jul 7 19:45 wwn-0x5000c500676fb68f-part2 -> ../../sda2 lrwxrwxrwx 1 root root 10 Jul 7 19:45 wwn-0x5000c500676fb68f-part3 -> ../../sda3 lrwxrwxrwx 1 root root 10 Jul 7 19:45 wwn-0x5000c500676fb68f-part4 -> ../../sda4 lrwxrwxrwx 1 root root 10 Jul 7 19:45 wwn-0x5000c500676fb68f-part5 -> ../../sda5 lrwxrwxrwx 1 root root 9 Mar 31 13:20 wwn-0x5001480000000000 -> ../../sr0

Any thought?

ok, with name == fs_path I see that return is correct here

jsafrane · 2017-07-11T17:16:22Z

pkg/volume/fc/fc_util.go

+				err = checkDiskExist(b, disk, wwid)
+				if err != nil {
+					glog.V(2).Infof("fc: failed to get wwid from disk[%s], error %v", dev_id+name, err)
+					return "", ""


dtto, continue here?

scratch the previous comment, return is correct here

nikhiljindal · 2017-07-11T20:49:53Z

/approve for federation. Al federation changes in this PR are auto generated.

mtanino · 2017-07-11T22:53:02Z

/test pull-kubernetes-node-e2e

mtanino · 2017-07-11T23:14:30Z

/test pull-kubernetes-e2e-gce-etcd3

mtanino · 2017-08-02T15:37:35Z

@rootfs @jsafrane @msau42
Just resolved merge conflict. No other changes.

Conflicting files
pkg/volume/fc/fc_util.go

mtanino · 2017-08-02T16:42:01Z

/retest

rootfs · 2017-08-02T21:31:23Z

/lgtm

mtanino · 2017-08-04T14:40:16Z

/assign @thockin
/assign @smarterclayton

thockin · 2017-08-10T06:08:11Z

/approve

This PR adds World Wide Identifier (WWID) parameter to FCVolumeSource as an unique volume identifier. fixes kubernetes#48639

mtanino · 2017-08-10T14:45:28Z

@thockin
Thank you for the approval.

@rootfs
The patch is updated to resolve the merge conflict. No other changes.

Conflicting files
staging/src/k8s.io/api/core/v1/generated.pb.go

mtanino · 2017-08-10T16:11:46Z

/retest

rootfs · 2017-08-10T16:55:36Z

/lgtm

k8s-github-robot · 2017-08-10T16:55:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, mtanino, nikhiljindal, rootfs, thockin

Associated issue: 48639

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [thockin]
~~docs/OWNERS~~ [thockin]
~~federation/OWNERS~~ [nikhiljindal,thockin]
~~pkg/api/OWNERS~~ [thockin]
~~pkg/volume/fc/OWNERS~~ [jsafrane,rootfs,thockin]
~~staging/src/k8s.io/api/core/OWNERS~~ [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

mtanino · 2017-08-10T22:57:20Z

/retest

k8s-github-robot · 2017-08-11T00:07:38Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-11T00:55:08Z

Automatic merge from submit-queue

k8s-ci-robot requested review from jsafrane, msau42 and rootfs July 11, 2017 00:21

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 11, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2017

k8s-github-robot assigned timothysc Jul 11, 2017

k8s-github-robot assigned nikhiljindal Jul 11, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2017

jsafrane reviewed Jul 11, 2017

View reviewed changes

k8s-ci-robot assigned rootfs Jul 11, 2017

jsafrane reviewed Jul 11, 2017

View reviewed changes

mtanino force-pushed the issue/48639 branch from 73e41eb to 186f47e Compare July 11, 2017 16:10

jsafrane reviewed Jul 11, 2017

View reviewed changes

mtanino force-pushed the issue/48639 branch from 186f47e to 50af163 Compare July 11, 2017 22:25

mtanino force-pushed the issue/48639 branch from c8a0f8f to 1c1d81e Compare August 2, 2017 15:35

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 2, 2017

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

k8s-ci-robot assigned smarterclayton and thockin Aug 4, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 10, 2017

mtanino added 2 commits August 10, 2017 09:59

FC plugin: Support WWID for volume identifier

03e2847

This PR adds World Wide Identifier (WWID) parameter to FCVolumeSource as an unique volume identifier. fixes kubernetes#48639

Autogenerated files

422ce03

mtanino force-pushed the issue/48639 branch from 1c1d81e to 422ce03 Compare August 10, 2017 14:43

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 10, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2017

k8s-github-robot merged commit 13b006a into kubernetes:master Aug 11, 2017

mtanino mentioned this pull request Aug 15, 2017

VolumeMode and VolumeDevice api #50457

Merged

mtanino mentioned this pull request Sep 19, 2017

Update 1.8 release notes for sig-storage kubernetes/enhancements#433

Merged

singlecheeze mentioned this pull request Oct 21, 2024

LUN Fuzzy vs. RegEx Search Infinidat/infinibox-csi-driver#118

Closed

FC volume plugin: Support WWID for volume identifier #48741

FC volume plugin: Support WWID for volume identifier #48741

Conversation

mtanino commented Jul 11, 2017

k8s-ci-robot commented Jul 11, 2017

mtanino commented Jul 11, 2017

k8s-ci-robot commented Jul 11, 2017

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

jsafrane commented Jul 11, 2017

Choose a reason for hiding this comment

jsafrane Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Jul 11, 2017

jsafrane commented Jul 11, 2017

mtanino commented Jul 11, 2017 • edited Loading

jsafrane commented Jul 11, 2017

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

nikhiljindal commented Jul 11, 2017

mtanino commented Jul 11, 2017

mtanino commented Jul 11, 2017

mtanino commented Aug 2, 2017

mtanino commented Aug 2, 2017

rootfs commented Aug 2, 2017

mtanino commented Aug 4, 2017

thockin commented Aug 10, 2017

mtanino commented Aug 10, 2017

mtanino commented Aug 10, 2017

rootfs commented Aug 10, 2017

k8s-github-robot commented Aug 10, 2017

mtanino commented Aug 10, 2017

k8s-github-robot commented Aug 11, 2017

k8s-github-robot commented Aug 11, 2017

jsafrane Jul 11, 2017 •

edited

Loading

mtanino commented Jul 11, 2017 •

edited

Loading