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.

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

Support retrieving the VM UUID on Windows #71147

Merged
merged 1 commit into from
Feb 16, 2019
Merged

Support retrieving the VM UUID on Windows #71147

merged 1 commit into from
Feb 16, 2019

Conversation

benmoss
Copy link
Member

@benmoss benmoss commented Nov 16, 2018

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds support to the vSphere cloud provider for retrieving a VM's UUID when running on Windows.

Special notes for your reviewer:
On linux VMs, there is a file with the UUID in it. On windows, this file does not exist, so we use the wmic CLI tool to get the same value.

$ wmic csproduct get UUID
UUID
5EE03942-CFD7-FE7D-A250-65899E639441

Does this PR introduce a user-facing change?:

vSphere cloud provider correctly retrieves the VM's UUID when running on Windows

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Nov 16, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 16, 2018
@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 16, 2018
@benmoss
Copy link
Member Author

benmoss commented Nov 16, 2018

/sig windows

@k8s-ci-robot k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Nov 16, 2018
@benmoss
Copy link
Member Author

benmoss commented Nov 16, 2018

/sig vmware

@k8s-ci-robot k8s-ci-robot added the area/provider/vmware Issues or PRs related to vmware provider label Nov 16, 2018
@benmoss
Copy link
Member Author

benmoss commented Nov 16, 2018

/check-cla

@k8s-ci-robot 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 Nov 16, 2018
@benmoss
Copy link
Member Author

benmoss commented Nov 16, 2018

/retest

@neolit123
Copy link
Member

thank you for this PR @benmoss
commits should be probably squashed.

/retest
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 17, 2018
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

the wmic csproduct get UUID usage SGTM 👍

Signed-off-by: Ben Moss <bmoss@pivotal.io>
@benmoss
Copy link
Member Author

benmoss commented Nov 17, 2018

@neolit123 squashed as you requested

@astrieanna
Copy link
Contributor

@neolit123 what needs to happen for this to get merged?

@neolit123
Copy link
Member

/assign @SandeepPissay

the project is fairly low activity before xmas and ny.
if the assigned reviewers and approvers do not respond try pinging them on slack at #sig-vmware and/or #sig-cloud-provider

@SandeepPissay
Copy link
Contributor

@benmoss, The code changes looks good to me. Have you tested that vSphere Cloud Provider works in all cases if k8s cluster is deployed as Windows VMs on vSphere?

@benmoss
Copy link
Member Author

benmoss commented Jan 16, 2019

@SandeepPissay yup, as far as we've understood the functionality everything seems to work with this.

@PatrickLang
Copy link
Contributor

cc @michmike can you take a look?

@neolit123
Copy link
Member

neolit123 commented Feb 14, 2019

/assign @frapposelli @dougm

hi, can you please take a look for approval here too?
the UUID logic on Windows looks sane.

@frapposelli
Copy link
Member

The approach is sane, I don't have the necessary Windows knowledge to validate the UUID retrieval method but

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2019
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.

/lgtm

@dvonthenen may want to review too. We should also open an issue for porting this to the external vSphere CCM.

@dvonthenen
Copy link

dvonthenen commented Feb 15, 2019

/lgtm

@dvonthenen may want to review too. We should also open an issue for porting this to the external vSphere CCM.

@dougm We use the UUID reported by the kubelet. If the kubelet populates the SystemUUID property correctly, this should automatically work for Windows systems without any code changes.

@frapposelli
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benmoss, 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 Feb 15, 2019
@benmoss
Copy link
Member Author

benmoss commented Feb 15, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit e4db147 into kubernetes:master Feb 16, 2019
@benmoss benmoss deleted the vsphere-windows-uuid branch February 16, 2019 19:09
@rhockenbury
Copy link

rhockenbury commented Feb 28, 2019

Having trouble getting windows nodes to find vm id with 1.14 beta.1:

E0228 18:07:38.491060    4324 datacenter.go:78] Unable to find VM by UUID. VM UUID: a7ff1742-06d1-549d-712b-193a270b7917
E0228 18:07:38.491060    4324 vsphere.go:1467] Cannot find VM runtime host. Get zone for node ul-win-node-5k error
E0228 18:07:38.491060    4324 kubelet_node_status.go:68] Unable to construct v1.Node object for kubelet: failed to get zone from cloud provider: No VM found

On linux, the /sys/class/dmi/id/product_serial file is used, which contains an id with a VMware prefix:

$ cat /sys/class/dmi/id/product_serial
VMware-42 17 76 c6 8e 65 8a 09-28 e9 1b 55 34 fa b9 76

To return the equivalent form on Windows, it looks like the following can be used:

$ wmic bios get serialnumber
SerialNumber
VMware-42 17 ff a7 d1 06 9d 54-71 2b 19 3a 27 0b 79 17

or

wmic csproduct get identifyingnumber
IdentifyingNumber
VMware-42 17 ff a7 d1 06 9d 54-71 2b 19 3a 27 0b 79 17

BUT the current implementation uses wmic csproduct get uuid which returns:

UUID
A7FF1742-06D1-549D-712B-193A270B7917

So is wmic csproduct get uuid the correct logic?

It appears the logic was correct at the time of opening the PR. However, in light of the change on the linux side from /sys/class/dmi/id/product_uuid to /sys/class/dmi/id/product_serial as implemented in #59519, the logic used on the windows side needs to be modified.

@rhockenbury
Copy link

/lgtm
@dvonthenen may want to review too. We should also open an issue for porting this to the external vSphere CCM.

@dougm We use the UUID reported by the kubelet. If the kubelet populates the SystemUUID property correctly, this should automatically work for Windows systems without any code changes.

On windows, the systemUUID does not currently get populated because of how cadvisor discovers it (https://github.com/google/cadvisor/blob/master/utils/sysfs/sysfs.go#L237).

@dvonthenen
Copy link

@rhockenbury Yes, the wmic csproduct get uuid is the correct implementation. The in-tree vSphere cloud provider code populates the UUID using that command. Please see below:
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/vsphere/vsphere_util_windows.go#L28

@rhockenbury
Copy link

Yes, but this makes the assumption that the uuid and serial are the same, which is only true on some vm versions. vmware-archive#450 (comment), vmware-archive#450 (comment)

@benmoss
Copy link
Member Author

benmoss commented Mar 4, 2019

Agreed, from what I remember of why I chose get uuid it was just because it required less formatting than the serial. I wasn't aware that there were instances where they disagreed. Do you want to submit a PR @rhockenbury?

@rhockenbury
Copy link

I don't have an environment set up where I could get a PR out quickly. Possibly by end of week, so if someone else is able and willing to get to it sooner, please go for it. Would this need to be in by code freeze (3/7) to make it into the 1.14 release?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.