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

vSphere Volume Plugin Implementation #24947

Merged
merged 2 commits into from
May 23, 2016
Merged

vSphere Volume Plugin Implementation #24947

merged 2 commits into from
May 23, 2016

Conversation

abithap
Copy link

@abithap abithap commented Apr 28, 2016 •
edited
Loading

This PR implements vSphere Volume plugin support in Kubernetes (ref. issue #23932).

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@k8s-bot
Copy link

k8s-bot commented Apr 28, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments
@k8s-bot
Copy link

k8s-bot commented Apr 28, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Apr 28, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Apr 28, 2016
@abithap abithap force-pushed the hpe/vsphere-volume branch from 303aef9 to 159e624 Compare April 28, 2016 21:19
@dagnello
Copy link
Contributor

hi @thockin, I have also contributed some commits to this PR how can @abithap add additional users to it? I am assuming that would also resolve the auto-cla checks.

dagnello
dagnello reviewed Apr 28, 2016
View reviewed changes
pkg/api/types.go
// Unique id of the volume used to identify the vSphere volume
VolumeID string `json:"volumeID"`
// Path that identifies vSphere volume vmdk
Path string `json:"volPath"`
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use 'volumePath' for both the variable and json name for this attribute

Copy link
Author

Choose a reason for hiding this comment

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

done

@abithap abithap force-pushed the hpe/vsphere-volume branch from e2a24d9 to c6b3c58 Compare May 10, 2016 20:56
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2016
@abithap abithap force-pushed the hpe/vsphere-volume branch from c6b3c58 to 59701bc Compare May 13, 2016 19:41
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 13, 2016
@dagnello dagnello mentioned this pull request May 15, 2016
Merged
dagnello
dagnello reviewed May 15, 2016
View reviewed changes
pkg/cloudprovider/providers/vsphere/vsphere.go
uuidWithNoHypens := strings.Replace(b.Uuid, "-", "", -1)
return uuidWithNoHypens
}
return ""
Copy link
Contributor

Choose a reason for hiding this comment

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

if disk UUID fails, we should return error with this function and caller (attach disk function) should fail and log error. Without this information, mount will also fail and would be harder to diagnose if we don't log/fail here.

Copy link
Author

Choose a reason for hiding this comment

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

disk-uuid is only used for mounting, but attach disk also returns disk-id for detaching.
Returning an error in this case will disrupt the existing functionality. So I have added a check before mount which will fail if it doesn't get a uuid. I will add a log to make it clear.

@abithap abithap force-pushed the hpe/vsphere-volume branch from 59701bc to bfedc82 Compare May 16, 2016 20:42
@abithap
Copy link
Author

abithap commented May 16, 2016

I have rebased and addressed the comments. @thockin please review this PR when you get a chance. Thanks

@k8s-bot
Copy link

k8s-bot commented May 17, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@rootfs
Copy link
Contributor

rootfs commented May 21, 2016

@abithap in pkg/api/v1/types.go you need to define vsphereVolume. See how other volume plugins are defined there. Then re-run codegen.

@abithap abithap force-pushed the hpe/vsphere-volume branch 2 times, most recently from 55e39de to 07ccfb3 Compare May 21, 2016 06:06
@k8s-github-robot k8s-github-robot added needs-ok-to-merge needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 21, 2016
@abithap abithap force-pushed the hpe/vsphere-volume branch from 07ccfb3 to ce17bca Compare May 21, 2016 18:20
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2016
@abithap abithap force-pushed the hpe/vsphere-volume branch from ce17bca to 1139765 Compare May 21, 2016 19:53
@k8s-bot
Copy link

k8s-bot commented May 21, 2016

GCE e2e build/test passed for commit 1139765.

@abithap
Copy link
Author

abithap commented May 22, 2016

@thockin @rootfs rebased and the tests are passing now. Please ok to test when you get a chance. thanks.

@pmorie pmorie added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 22, 2016
@thockin
Copy link
Member

thockin commented May 22, 2016

@rootfs LGTM?

@rootfs
Copy link
Contributor

rootfs commented May 22, 2016

@thockin LGTM, but I don't have permission to tag it

@pmorie pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

@k8s-bot test this issue: #IGNORE

Tests have been pending for 24 hours

@k8s-bot
Copy link

k8s-bot commented May 22, 2016

GCE e2e build/test passed for commit 1139765.

@k8s-bot
Copy link

k8s-bot commented May 22, 2016

GCE e2e build/test passed for commit 1139765.

@thockin
Copy link
Member

thockin commented May 23, 2016

@k8s-bot test this: github issue #IGNORE

@k8s-bot
Copy link

k8s-bot commented May 23, 2016

GCE e2e build/test passed for commit 1139765.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 23, 2016

GCE e2e build/test passed for commit 1139765.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 8b0e9c5 into kubernetes:master May 23, 2016
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request May 12, 2020
@k8s-publishing-bot
Merge pull request kubernetes#24947 from damemi/balance-preemption-e2…
5f4863b 
…e-nodes

Bug 1731263: Balance preemption e2e nodes

Origin-commit: 65834ad3365eeb49f013004248b2d93065861642
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@thockin thockin

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
9 participants
@abithap @googlebot @k8s-bot @dagnello @lavalamp @thockin @rootfs @k8s-github-robot @pmorie