Skip to content
This repository has been archived by the owner on Sep 7, 2022. It is now read-only.

Report InstanceID for vSphere Cloud Provider as UUID from product_serial file #452

Closed
wants to merge 1 commit into from

Conversation

abrarshivani
Copy link

@abrarshivani abrarshivani commented Feb 5, 2018

Before this fix, if VM was created on vSphere v1.6.5 then VCP was not able to find the nodes. Since VCP was finding VMs based on UUID which was reported by Kubelet as SystemUUID. Kubelet fetched SystemUUID from file /sys/class/dmi/id/product_uuid. For VMs created on vSphere 1.6.5. product_uuid is not same uuid identified by VC. Yet, /sys/class/dmi/id/product_serial contains VM uuid which is identified by VC. Hence, this PR fixes this issue by reporting the uuid fetched from product_serial.

What this PR does / why we need it:

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 #450

Special notes for your reviewer:

Release note:

vSphere Cloud Provider supports VM provision based on vSphere v1.6.5

Tested:

  1. K8s cluster using kubeadm
    a. Created Ubuntu VM with compatible with vSphere version 6.5. (Note: Installed Ubuntu from ISO)
    b. Observed following:
Master
> cat /sys/class/dmi/id/product_uuid
743F0E42-84EA-A2F9-7736-6106BB5DBF6B

> cat /sys/class/dmi/id/product_serial
VMware-42 0e 3f 74 ea 84 f9 a2-77 36 61 06 bb 5d bf 6b

Node
> cat /sys/class/dmi/id/product_uuid
956E0E42-CC9D-3D89-9757-F27CEB539B76

> cat /sys/class/dmi/id/product_serial
VMware-42 0e 6e 95 9d cc 89 3d-97 57 f2 7c eb 53 9b 76

c. With this fix controller manager was able to find the nodes.
controller manager logs

{"log":"I0205 22:43:00.106416       1 nodemanager.go:183] Found node ubuntu-node as vm=VirtualMachine:vm-95 in vc=10.161.120.115 and datacenter=vcqaDC\n","stream":"stderr","time":"2018-02-05T22:43:00.421010375Z"}

Copy link

@pshahzeb pshahzeb left a comment

Choose a reason for hiding this comment

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

LGTM.
Please ensure e2e tests pass.

if !strings.HasPrefix(uuid, UUIDPrefix) {
return "", fmt.Errorf("Failed to match Prefix, UUID read from the file is %v", uuidFromFile)
}
// Strip the prefix and while spaces and -
Copy link

Choose a reason for hiding this comment

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

nit: white spaces

}

func GetUUIDFromProviderID(providerID string) string {
return strings.TrimPrefix(providerID, ProviderPrefix)
Copy link

Choose a reason for hiding this comment

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

Confirming the need for GetUUIDFromProviderID. Is ProviderPrefix automatically added to uuid returned by func InstanceID()?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is added for all cloudproviders

@abrarshivani abrarshivani assigned BaluDontu and unassigned BaluDontu Feb 7, 2018
@abrarshivani abrarshivani force-pushed the vm_uuid_provider_id branch 2 times, most recently from b3c1da9 to 4fe36f0 Compare February 7, 2018 23:01
Copy link

@divyenpatel divyenpatel left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -43,6 +44,9 @@ const (
Folder = "Folder"
VirtualMachine = "VirtualMachine"
DummyDiskName = "kube-dummyDisk.vmdk"
UUIDPath = "/sys/class/dmi/id/product_serial"
UUIDPrefix = "VMware-"
ProviderPrefix = "vsphere://"

Choose a reason for hiding this comment

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

apply gofmt to align code.

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Feb 8, 2018
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Report InstanceID for vSphere Cloud Provider as UUID obtained from product_serial file 

**What this PR does / why we need it**:
vSphere Cloud Provider is not able to find the nodes for VMs created on vSphere v1.6.5. Kubelet fetches SystemUUID from file ```/sys/class/dmi/id/product_uuid```. vSphere Cloud Provider uses this uuid as VM identifier to get node information from vCenter. vCenter v1.6.5 doesn't recognize this uuids, as a result, nodes are not found. 

UUID present in file ```/sys/class/dmi/id/product_serial``` is recognized by vCenter. Yet,  Kubelet doesn't report this. Therefore, in this PR InstanceID is reported as UUID which is fetched from file 
```/sys/class/dmi/id/product_serial```.

**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 #58927

**Special notes for your reviewer**:
Internally review here: vmware-archive#452

Tested:
Launched K8s cluster using kubeadm (Used Ubuntu VM compatible with vSphere version 6.5.)
_**Note: Installed Ubuntu from ISO**_
Observed following:
```
Master
> cat /sys/class/dmi/id/product_uuid
743F0E42-84EA-A2F9-7736-6106BB5DBF6B

> cat /sys/class/dmi/id/product_serial
VMware-42 0e 3f 74 ea 84 f9 a2-77 36 61 06 bb 5d bf 6b

Node
> cat /sys/class/dmi/id/product_uuid
956E0E42-CC9D-3D89-9757-F27CEB539B76

> cat /sys/class/dmi/id/product_serial
VMware-42 0e 6e 95 9d cc 89 3d-97 57 f2 7c eb 53 9b 76
```
With this fix controller manager was able to find the nodes.
**controller manager logs**
```
{"log":"I0205 22:43:00.106416       1 nodemanager.go:183] Found node ubuntu-node as vm=VirtualMachine:vm-95 in vc=10.161.120.115 and datacenter=vcqaDC\n","stream":"stderr","time":"2018-02-05T22:43:00.421010375Z"}
```


**Release note**:

```release-note
vSphere Cloud Provider supports VMs provisioned on vSphere v1.6.5
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants