Cinder volume attacher: use instanceID instead of NodeID when verifying attachment #39998

DukeXar · 2017-01-17T08:53:47Z

What this PR does / why we need it: Cinder volume attacher incorrectly uses NodeID instead of openstack instance id, so that reconciliation fails.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #39978

Special notes for your reviewer:

Release note:

Fixed checking of InstanceID when attaching OpenStack Cinder volume.

k8s-ci-robot · 2017-01-17T08:53:48Z

Hi @DukeXar. 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 @k8s-bot 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.

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.

k8s-reviewable · 2017-01-17T08:53:53Z

This change is

DukeXar · 2017-01-17T08:55:16Z

@k8s-ci-robot I signed it!

resouer · 2017-01-17T12:38:22Z

@k8s-bot ok to test

resouer · 2017-01-17T12:40:42Z

pkg/volume/cinder/attacher.go

+		return volumesAttachedCheck, err
+	}
+
+	attachedResult, err := attacher.cinderProvider.DisksAreAttached(volumeIDList, instanceid)


It would be better to have a unit test to verify this change.

DukeXar · 2017-01-18T08:48:53Z

pkg/volume/cinder/attacher_test.go

+			test: func(testcase *testcase) (string, error) {
+				attacher := newAttacher(testcase)
+				attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
+				return serializeAttachments(attachments), err


serializeAttachments

Does not look like the best thing to do, open for suggestions on what is idiomatic in Go

DukeXar · 2017-01-18T08:49:06Z

pkg/volume/cinder/attacher.go

+		return volumesAttachedCheck, err
+	}
+
+	attachedResult, err := attacher.cinderProvider.DisksAreAttached(volumeIDList, instanceid)


resouer · 2017-01-18T13:19:48Z

Leave rest of this to @saad-ali for final review, thanks a lot!

xsgordon · 2017-02-06T21:29:37Z

@idvoretskyi can you add the sig/openstack label?

jsafrane · 2017-02-10T12:51:23Z

no response from @saad-ali, I reviewed it by myself
/lgtm
/approve

k8s-github-robot · 2017-02-10T12:52:00Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: DukeXar, jsafrane

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

~~pkg/volume/cinder/OWNERS~~ [jsafrane]

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

jsafrane · 2017-02-10T13:44:56Z

@k8s-bot node e2e test this

k8s-github-robot · 2017-02-10T15:53:57Z

Automatic merge from submit-queue (batch tested with PRs 41246, 39998)

resouer · 2017-02-13T02:03:17Z

@DukeXar Congratulations! ㊗️ ! Enjoy the contribution.

DukeXar · 2017-02-22T23:04:19Z

@resouer Thanks. Do I need to cherry-pick it manually to get into the release 1.5?

DukeXar · 2017-02-23T00:24:40Z

@jsafrane does it need to have a milestone and a release note in order to be cherry-picked into the 1.5? I am a bit confused with the process.

jsafrane · 2017-02-23T09:02:51Z

@DukeXar, I added milestone and cherrypick-candidate label, IMO that should be enough

…8-upstream-release-1.5 Automatic merge from submit-queue Automated cherry pick of #39998 Cherry pick of #39998 on release-1.5. #39998: Cinder volume attacher: use instanceID not NodeID

k8s-cherrypick-bot · 2017-07-05T12:11:26Z

Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Automatic merge from submit-queue (batch tested with PRs 41246, 39998) Cinder volume attacher: use instanceID instead of NodeID when verifying attachment **What this PR does / why we need it**: Cinder volume attacher incorrectly uses NodeID instead of openstack instance id, so that reconciliation fails. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes kubernetes#39978 **Special notes for your reviewer**: **Release note**: ```release-note ```

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 17, 2017

DukeXar force-pushed the cinder_instance_id branch from 148bac9 to c53cf22 Compare January 17, 2017 08:58

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 17, 2017

k8s-github-robot assigned saad-ali Jan 17, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jan 17, 2017

resouer reviewed Jan 17, 2017

View reviewed changes

DukeXar force-pushed the cinder_instance_id branch from c53cf22 to d1ed363 Compare January 17, 2017 22:17

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 18, 2017

Cinder volume attacher: use instanceID not NodeID

2267588

DukeXar force-pushed the cinder_instance_id branch from 832c297 to 0842b90 Compare January 18, 2017 01:53

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 18, 2017

Add unit-tests for DisksAreAttached

084d801

DukeXar force-pushed the cinder_instance_id branch from 0842b90 to 084d801 Compare January 18, 2017 01:55

k8s-github-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 Jan 18, 2017

DukeXar commented Jan 18, 2017

View reviewed changes

k8s-github-robot assigned jsafrane and unassigned saad-ali Jan 30, 2017

apelisse assigned saad-ali Jan 31, 2017

jsafrane added the area/provider/openstack Issues or PRs related to openstack provider label Feb 7, 2017

jsafrane added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Feb 7, 2017

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2017

k8s-github-robot merged commit 6ea92b4 into kubernetes:master Feb 10, 2017

gnufied mentioned this pull request Feb 21, 2017

VerifyVolumesAreAttached is failing and looping on remounts for openstack cinder #39929

Closed

DukeXar mentioned this pull request Feb 23, 2017

Automated cherry pick of #39998 #41940

Merged

jsafrane added this to the v1.6 milestone Feb 23, 2017

jsafrane added the cherrypick-candidate label Feb 23, 2017

DukeXar deleted the cinder_instance_id branch February 23, 2017 09:04

enisoc modified the milestones: v1.5, v1.6 Mar 23, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Jul 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cinder volume attacher: use instanceID instead of NodeID when verifying attachment #39998

Cinder volume attacher: use instanceID instead of NodeID when verifying attachment #39998

DukeXar commented Jan 17, 2017 •

edited by jsafrane

Loading

k8s-ci-robot commented Jan 17, 2017

k8s-reviewable commented Jan 17, 2017

DukeXar commented Jan 17, 2017

resouer commented Jan 17, 2017

resouer Jan 17, 2017 •

edited

Loading

DukeXar Jan 18, 2017

DukeXar Jan 18, 2017

DukeXar Jan 18, 2017

resouer commented Jan 18, 2017

xsgordon commented Feb 6, 2017

jsafrane commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

jsafrane commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

resouer commented Feb 13, 2017

DukeXar commented Feb 22, 2017

DukeXar commented Feb 23, 2017

jsafrane commented Feb 23, 2017

k8s-cherrypick-bot commented Jul 5, 2017

Cinder volume attacher: use instanceID instead of NodeID when verifying attachment #39998

Cinder volume attacher: use instanceID instead of NodeID when verifying attachment #39998

Conversation

DukeXar commented Jan 17, 2017 • edited by jsafrane Loading

k8s-ci-robot commented Jan 17, 2017

k8s-reviewable commented Jan 17, 2017

DukeXar commented Jan 17, 2017

resouer commented Jan 17, 2017

resouer Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

DukeXar Jan 18, 2017

Choose a reason for hiding this comment

DukeXar Jan 18, 2017

Choose a reason for hiding this comment

DukeXar Jan 18, 2017

Choose a reason for hiding this comment

resouer commented Jan 18, 2017

xsgordon commented Feb 6, 2017

jsafrane commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

jsafrane commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

resouer commented Feb 13, 2017

DukeXar commented Feb 22, 2017

DukeXar commented Feb 23, 2017

jsafrane commented Feb 23, 2017

k8s-cherrypick-bot commented Jul 5, 2017

DukeXar commented Jan 17, 2017 •

edited by jsafrane

Loading

resouer Jan 17, 2017 •

edited

Loading