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

Do not query for VMUUID in vsphere cloud provider if it was explicitly passed #65152

Merged
merged 1 commit into from
Jun 22, 2018
Merged

Do not query for VMUUID in vsphere cloud provider if it was explicitly passed #65152

merged 1 commit into from
Jun 22, 2018

Conversation

alvaroaleman
Copy link
Member

@alvaroaleman alvaroaleman commented Jun 15, 2018 •
edited
Loading

What this PR does / why we need it:

It allows running the kube controller-manager with the vsphere cloudprovider enabled on a machine that is not a vmware VM

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #65145

Special notes for your reviewer:

Release note:

In the vSphere cloud provider the `Global.vm-uuid` configuration option is not deprecated anymore, it can be used to overwrite the VMUUID on the controller-manager

/sig vmware

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. area/provider/vmware Issues or PRs related to vmware provider size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 15, 2018
@wgliang
Copy link
Contributor

wgliang commented Jun 16, 2018

/ok-to-test

@k8s-ci-robot 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 Jun 16, 2018
dougm
dougm approved these changes Jun 16, 2018
View reviewed changes
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Looks like the vm UUID is just used as the node instance ID. I don't see any other references to VSphereConfig.Global.VMUUID outside of the tests.. it looks your PR implements what this field was originally intended for - to avoid reading from /sys/... when set.
/approve

@dougm
Copy link
Member

dougm commented Jun 16, 2018

/assign @abrarshivani

abrarshivani
abrarshivani reviewed Jun 16, 2018
View reviewed changes
pkg/cloudprovider/providers/vsphere/vsphere.go
@@ -134,7 +134,7 @@ type VSphereConfig struct {
WorkingDir string `gcfg:"working-dir"`
// Soap round tripper count (retries = RoundTripper - 1)
RoundTripperCount uint `gcfg:"soap-roundtrip-count"`
// Deprecated as the virtual machines will be automatically discovered.
// Is required on the controller-manager if it does not run on a VMware machine
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we will need release note since the field was deprecated.

abrarshivani
abrarshivani approved these changes Jun 16, 2018
View reviewed changes
Copy link
Contributor

@abrarshivani abrarshivani left a comment

Choose a reason for hiding this comment

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

LGTM. Just a minor comment

@alvaroaleman alvaroaleman force-pushed the vsphere-control-plane-on-cloud branch from 1cb5d4e to 81e25c3 Compare June 17, 2018 09:56
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 17, 2018
@alvaroaleman
Copy link
Member Author

@abrarshivani I've added a Release note, does it match your expectations?

I've also slightly updated the PR to do the error handling for the getVMUUID() call inside the else branch

frapposelli
frapposelli approved these changes Jun 18, 2018
View reviewed changes
Copy link
Member

@frapposelli frapposelli left a comment

Choose a reason for hiding this comment

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

Makes sense also from the perspective of the out-of-tree cloud provider

/approve

@abrarshivani
Copy link
Contributor

Thanks for adding release note.
/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 Jun 18, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrarshivani, alvaroaleman, dougm, frapposelli

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 Jun 18, 2018
@k8s-github-robot
Copy link

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

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 22, 2018 •
edited
Loading

@alvaroaleman: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 81e25c3 link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-e2e-gce 81e25c3 link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 65152, 65199, 65179, 64598, 65216). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit af1450d into kubernetes:master Jun 22, 2018
@alvaroaleman alvaroaleman mentioned this pull request Oct 11, 2018
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abrarshivani abrarshivani abrarshivani approved these changes

@frapposelli frapposelli frapposelli approved these changes

@dougm dougm dougm approved these changes

@markturansky markturansky Awaiting requested review from markturansky

@pmorie pmorie Awaiting requested review from pmorie

Assignees

@abrarshivani abrarshivani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/vmware Issues or PRs related to vmware provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vshere cloud provider on the controller-manager fails to start on non-vmware hosts
7 participants
@alvaroaleman @wgliang @dougm @abrarshivani @k8s-ci-robot @k8s-github-robot @frapposelli