Fixes #40819 and Fixes #33114 #40892

robdaemon · 2017-02-02T22:48:17Z

What this PR does / why we need it:

Start looking up the virtual machine by it's UUID in vSphere again. Looking up by IP address is problematic and can either not return a VM entirely, or could return the wrong VM.

Retrieves the VM's UUID in one of two methods - either by a vm-uuid entry in the cloud config file on the VM, or via sysfs. The sysfs route requires root access, but restores the previous functionality.

Multiple VMs in a vCenter cluster can share an IP address - for example, if you have multiple VM networks, but they're all isolated and use the same address range. Additionally, flannel network address ranges can overlap.

vSphere seems to have a limitation of reporting no more than 16 interfaces from a virtual machine, so it's possible that the IP address list on a VM is completely untrustworthy anyhow - it can either be empty (because the 16 interfaces it found were veth interfaces with no IP address), or it can report the flannel IP.

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

Fixes #40819
Fixes #33114

Special notes for your reviewer:

Release note:

Reverts to looking up the current VM in vSphere using the machine's UUID, either obtained via sysfs or via the `vm-uuid` parameter in the cloud configuration file.

k8s-ci-robot · 2017-02-02T22:48:18Z

Hi @robdaemon. 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-02-02T22:48:24Z

This change is

roberthbailey · 2017-02-02T23:22:31Z

/lgtm

kerneltime · 2017-02-02T23:46:26Z

CC @BaluDontu @divyenpatel

kerneltime · 2017-02-02T23:56:35Z

Corresponding documentation needs a PR https://github.com/kubernetes/kubernetes.github.io/blob/master/docs/getting-started-guides/vsphere.md

kerneltime · 2017-02-03T00:06:06Z

We should add support for this https://github.com/kubernetes/kubernetes-anywhere/blob/master/phase1/vsphere/vSphere.jsonnet#L82

kerneltime · 2017-02-03T00:16:09Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -42,6 +41,8 @@ import (
 	"github.com/vmware/govmomi/vim25/types"
 	"golang.org/x/net/context"

+	"io/ioutil"


Can this be moved along with other golang imports

kerneltime · 2017-02-03T00:17:26Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+	if cfg.Global.VMUUID != "" {
+		vmUUID = cfg.Global.VMUUID
+	} else {
+		vmUUIDbytes, err := ioutil.ReadFile("/sys/devices/virtual/dmi/id/product_uuid")


Add a comment that this needs appropriate privilidges

kerneltime · 2017-02-03T00:18:23Z

Apart from unit testing were any other tests performed? Would be good to know.

pdhamdhere · 2017-02-03T00:42:16Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -122,12 +123,15 @@ type VSphereConfig struct {
 		WorkingDir string `gcfg:"working-dir"`
 		// Soap round tripper count (retries = RoundTripper - 1)
 		RoundTripperCount uint `gcfg:"soap-roundtrip-count"`
+		// VMUUID is the virtual machine's UUID. If not set, will be fetched from the machine.


We have many UUIDs! How about following;

// VM Instance UUID of virtual machine which can be retrieved from instanceUuid property in VmConfigInfo, or also set as vc.uuid in VMX file) ?

done - merged your comment with the existing one.

pdhamdhere · 2017-02-03T00:48:40Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+	if cfg.Global.VMUUID != "" {
+		vmUUID = cfg.Global.VMUUID
+	} else {
+		vmUUIDbytes, err := ioutil.ReadFile("/sys/devices/virtual/dmi/id/product_uuid")


Can this path change in different linux distros?

Not that I'm aware of. sysfs is kernel-level, not distro-level. Someone please correct me if I'm wrong :)

kerneltime · 2017-02-03T08:49:54Z

Fixes #33114
Hopefully Github will pick up the issue else can you extend the fixes line on your initial comment to include #33114

MrTrustor · 2017-02-03T09:54:11Z

Thank you for the fix! Does this have any chance to be backported in the 1.5 branch?
cc @cvauvarin

robdaemon · 2017-02-03T19:19:58Z

@kerneltime - I tested this on a vSphere 6.0 cluster - both by specifying the ID in the config, by running it without that (as root) and running it as non-root. It behaves as expected.

Related to kubernetes/kubernetes#40892

robdaemon · 2017-02-03T19:24:53Z

@kerneltime Docs PR is kubernetes/website#2416

kerneltime · 2017-02-03T19:41:21Z

/lgtm
/approve

k8s-ci-robot · 2017-02-03T19:41:21Z

@kerneltime: you can't LGTM a PR unless you are an assignee.

In response to this comment:

/lgtm
/approve

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.

kerneltime · 2017-02-06T06:47:37Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+		vmUUID = cfg.Global.VMUUID
+	} else {
+		// This needs root privileges on the host, and will fail otherwise.
+		vmUUIDbytes, err := ioutil.ReadFile("/sys/devices/virtual/dmi/id/product_uuid")


This path can be made configurable and defaulted to this value. This way any distro specific concerns can be addressed by an optional item in the config passed in.

kerneltime · 2017-02-07T22:30:15Z

@k8s-bot ok to test

kerneltime · 2017-02-07T22:30:48Z

@roberthbailey this needs a LGTM

robdaemon · 2017-02-08T00:17:32Z

That failure looks unrelated to my change?

kerneltime · 2017-02-08T06:14:04Z

@k8s-bot gci gce e2e test this

Related to kubernetes/kubernetes#40892

roberthbailey · 2017-02-10T06:22:01Z

@kerneltime - I added you as an assignee so you should be able to lgtm now

roberthbailey · 2017-02-10T06:22:35Z

@robdaemon - we generally ask folks to squash their commits after addressing review feedback. If you feel this is ready to merge please squash it into a single commit.

Start looking up the virtual machine by it's UUID in vSphere again. Looking up by IP address is problematic and can either not return a VM entirely, or could return the wrong VM. Retrieves the VM's UUID in one of two methods - either by a `vm-uuid` entry in the cloud config file on the VM, or via sysfs. The sysfs route requires root access, but restores the previous functionality. Multiple VMs in a vCenter cluster can share an IP address - for example, if you have multiple VM networks, but they're all isolated and use the same address range. Additionally, flannel network address ranges can overlap. vSphere seems to have a limitation of reporting no more than 16 interfaces from a virtual machine, so it's possible that the IP address list on a VM is completely untrustworthy anyhow - it can either be empty (because the 16 interfaces it found were veth interfaces with no IP address), or it can report the flannel IP.

k8s-github-robot · 2017-02-10T06:55:57Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: kerneltime, robdaemon

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

~~pkg/cloudprovider/providers/vsphere/OWNERS~~ [kerneltime]

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

robdaemon · 2017-02-10T06:56:14Z

@roberthbailey - squashed! Thanks!

kerneltime · 2017-02-10T18:17:50Z

/lgtm

k8s-github-robot · 2017-02-10T21:35:41Z

Automatic merge from submit-queue (batch tested with PRs 41223, 40892, 41220, 41207, 41242)

…-k8s-release-1.5 Automatic merge from submit-queue Automated cherry pick of #40892 Cherry pick of #40892 on release-1.5. #40892: Fixes #40819

…-k8s-release-1.4 Automated cherry pick of #40892

Related to kubernetes/kubernetes#40892

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

robdaemon mentioned this pull request Feb 2, 2017

Wrong VM name is retrieved by the vSphere Cloud Provider #40819

Closed

k8s-github-robot assigned roberthbailey Feb 2, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 2, 2017

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

kerneltime reviewed Feb 3, 2017

View reviewed changes

pdhamdhere reviewed Feb 3, 2017

View reviewed changes

robdaemon changed the title ~~Fixes #40819~~ Fixes #40819 and Fixes #33114 Feb 3, 2017

robdaemon pushed a commit to hpcloud/kubernetes.github.io that referenced this pull request Feb 3, 2017

Adds documentation for new vSphere vm-uuid parameter.

fc96dc4

Related to kubernetes/kubernetes#40892

robdaemon mentioned this pull request Feb 3, 2017

Adds documentation for new vSphere vm-uuid parameter. kubernetes/website#2416

Merged

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2017

kerneltime approved these changes Feb 3, 2017

View reviewed changes

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

kerneltime reviewed Feb 6, 2017

View reviewed changes

chenopis pushed a commit to kubernetes/website that referenced this pull request Feb 9, 2017

Adds documentation for new vSphere vm-uuid parameter.

2f1ae18

Related to kubernetes/kubernetes#40892

This was referenced Feb 9, 2017

Automated cherry pick of #40892 #41217

Merged

Automated cherry pick of #40892 #41219

Merged

roberthbailey assigned kerneltime Feb 10, 2017

robdaemon force-pushed the vsphere_findbyuuid branch from 30f1c09 to 53a0093 Compare February 10, 2017 06:55

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 merged commit 93bd6b3 into kubernetes:master Feb 10, 2017

aocole mentioned this pull request Feb 14, 2017

VSphere: Ensure 'primary' IP address is always returned in NodeAddresses #40169

Closed

jessfraz added a commit that referenced this pull request Feb 23, 2017

Merge pull request #41219 from vmware/automated-cherry-pick-of-#40892…

46789c6

…-k8s-release-1.4 Automated cherry pick of #40892

tailnode pushed a commit to tailnode/kubernetes.github.io that referenced this pull request Feb 23, 2017

Adds documentation for new vSphere vm-uuid parameter.

601fd27

Related to kubernetes/kubernetes#40892

kerneltime mentioned this pull request Feb 27, 2017

Adding vsphere cloud provider support kubernetes-sigs/kubespray#1045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #40819 and Fixes #33114 #40892

Fixes #40819 and Fixes #33114 #40892

robdaemon commented Feb 2, 2017 •

edited

Loading

k8s-ci-robot commented Feb 2, 2017

k8s-reviewable commented Feb 2, 2017

roberthbailey commented Feb 2, 2017

kerneltime commented Feb 2, 2017

kerneltime commented Feb 2, 2017 •

edited

Loading

kerneltime commented Feb 3, 2017

kerneltime Feb 3, 2017

robdaemon Feb 3, 2017

kerneltime Feb 3, 2017

robdaemon Feb 3, 2017

kerneltime commented Feb 3, 2017

pdhamdhere Feb 3, 2017

robdaemon Feb 3, 2017

pdhamdhere Feb 3, 2017

robdaemon Feb 3, 2017

kerneltime commented Feb 3, 2017

MrTrustor commented Feb 3, 2017

robdaemon commented Feb 3, 2017

robdaemon commented Feb 3, 2017

kerneltime commented Feb 3, 2017

k8s-ci-robot commented Feb 3, 2017

kerneltime Feb 6, 2017

kerneltime commented Feb 7, 2017

kerneltime commented Feb 7, 2017

robdaemon commented Feb 8, 2017

kerneltime commented Feb 8, 2017

roberthbailey commented Feb 10, 2017

roberthbailey commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

robdaemon commented Feb 10, 2017

kerneltime commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

Fixes #40819 and Fixes #33114 #40892

Fixes #40819 and Fixes #33114 #40892

Conversation

robdaemon commented Feb 2, 2017 • edited Loading

k8s-ci-robot commented Feb 2, 2017

k8s-reviewable commented Feb 2, 2017

roberthbailey commented Feb 2, 2017

kerneltime commented Feb 2, 2017

kerneltime commented Feb 2, 2017 • edited Loading

kerneltime commented Feb 3, 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

kerneltime commented Feb 3, 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

kerneltime commented Feb 3, 2017

MrTrustor commented Feb 3, 2017

robdaemon commented Feb 3, 2017

robdaemon commented Feb 3, 2017

kerneltime commented Feb 3, 2017

k8s-ci-robot commented Feb 3, 2017

Choose a reason for hiding this comment

kerneltime commented Feb 7, 2017

kerneltime commented Feb 7, 2017

robdaemon commented Feb 8, 2017

kerneltime commented Feb 8, 2017

roberthbailey commented Feb 10, 2017

roberthbailey commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

robdaemon commented Feb 10, 2017

kerneltime commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

robdaemon commented Feb 2, 2017 •

edited

Loading

kerneltime commented Feb 2, 2017 •

edited

Loading