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

Use GetVersion() API instead of ver command #55143

Merged
merged 1 commit into from Nov 22, 2017

Conversation

feiskyer
Copy link
Member

@feiskyer feiskyer commented Nov 6, 2017

What this PR does / why we need it:

Should use GetVersion vs Shelling out to ver.

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

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 6, 2017
@feiskyer
Copy link
Member Author

feiskyer commented Nov 6, 2017

Note that with this PR the kernel verson is in different format as before, e.g. on my local windows 10 laptop

  • ver command would output 10.0.16299.19
  • while GetVersion would got 16299.15.amd64fre.rs3_release.170928-1534

/cc @taylorb-microsoft is this what we expected?

@thecloudtaylor
Copy link

@feiskyer - thanks for coding this up so quickly!

  • For windows versions the kernel matches usermode so there is only one version.
  • Explanation of fields from ver:
    • The first two fields (10.0) specific the OS Major version, which is unlikely to change but you can get it from GetVersionEX (https://msdn.microsoft.com/en-us/library/windows/desktop/ms724833(v=vs.85).aspx)
    • The third field (16299) is the buildNumber - that number is important as it increments with every full build.
    • The fourth field (19) is the UBR number, which is really what is required from the registry. The UBR number specifies what updates have been applied, each new Windows update increments this number.
  • Expiation of fields from BuildLabEx
    • First field is the (16299) is the buildNumber (you can also query the CurrentBuildNumber key vs parsing)
    • Second field is the UBR number ( you can also query the UBR key vs parsing)
    • Third field is the architecture
    • Forth field is the build Branch ( you can also query the BuildBranch key vs parsing)
    • Fifth field is the time stamp (not overly helpful unless your part of the Windows team)


KVI.kvi = windows.UTF16ToString(buf[:])

// Important - docker.exe MUST be manifested for this API to return

Choose a reason for hiding this comment

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

Ideally should update comment to proper binary name.

@thecloudtaylor
Copy link

To your question on why ver is returning a different URB than what is in the BuildLabEx registry key I'm not sure - can you see what its in that key vs the UBR key? The ver code queries the UBR key for that information.

Specifically what they are doing is:

  1. Call GetVersion() (https://msdn.microsoft.com/en-us/library/windows/desktop/ms724439(v=vs.85).aspx)
  2. Call an local GetVersionRevision() API - which uses the UBR registry key
  3. Format a string:
    "%d.%d.%05d.%d", (ver & 0xFF), ((ver >> 8) & 0xFF), ((ver >> 16) & 0xFFFF), revision)

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 10, 2017
@feiskyer
Copy link
Member Author

@taylorb-microsoft Addressed comments. PTAL

@feiskyer
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 10, 2017
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 10, 2017
@feiskyer
Copy link
Member Author

/retest

@feiskyer
Copy link
Member Author

/assign @yujuhong @tallclair

@JiangtianLi
Copy link
Contributor

@feiskyer @taylorb-microsoft Thanks for fixing this. I have a similar fix in #55496 but will revert to use yours. One question, for Linux,

OS-IMAGE                              KERNEL-VERSION
Debian GNU/Linux 8 (jessie)           4.11.0-1013-azure

Should the osImageVersion be something more verbal such as "Microsoft Windows 10 Enterprise" or "Microsoft Windows Server Datacenter", e.g. HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProductName? Just my 2 cents.

@k8s-ci-robot k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 15, 2017
@feiskyer
Copy link
Member Author

@tallclair Addressed comments. PTAL

@feiskyer
Copy link
Member Author

/retest

@tallclair
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, tallclair

Associated issue: 55083

The full list of commands accepted by this bot can be found here.

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

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

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@feiskyer feiskyer added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed milestone/incomplete-labels labels Nov 22, 2017
@feiskyer
Copy link
Member Author

feiskyer commented Nov 22, 2017

@tallclair Thanks. Could you also help to approve the milestone?

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Needs Attention

@feiskyer @tallclair @yujuhong @kubernetes/sig-node-misc

Action required: During code slush, pull requests in the milestone should be in progress.
If this pull request is not being actively worked on, please remove it from the milestone.
If it is being worked on, please add the status/in-progress label so it can be tracked with other in-flight pull requests.

Note: If this pull request is not resolved or labeled as priority/critical-urgent by Wed, Nov 22 it will be moved out of the v1.9 milestone.

Pull Request Labels
  • sig/node: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/cleanup: Adding tests, refactoring, fixing old bugs.
Help

@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 56115, 55143, 56179). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit d09f679 into kubernetes:master Nov 22, 2017
@feiskyer feiskyer deleted the version branch November 23, 2017 01:29
k8s-github-robot pushed a commit that referenced this pull request Jan 29, 2018
Automatic merge from submit-queue (batch tested with PRs 56995, 58498, 57426, 58902, 58863). 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>.

Get windows kernel version directly from registry

**What this PR does / why we need it**:

#55143 gets windows kernel version by calling windows.GetVersion(), but it doesn't work on windows 10. From https://msdn.microsoft.com/en-us/library/windows/desktop/ms724439(v=vs.85).aspx, GetVersion requires app to be manifested.

Applications not manifested for Windows 8.1 or Windows 10 will return the Windows 8 OS version value (6.2). I tried a toy go program using GetVersion on Windows 10 and it returns 0x23f00206.

Given the limited win32 functions in golang, we should read from registry directly.

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

**Special notes for your reviewer**:

Should also cherry-pick to v1.9.

**Release note**:

```release-note
Get windows kernel version directly from registry
```

/cc @JiangtianLi @taylorb-microsoft
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/needs-attention priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. 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.

Should use GetVersion vs Shelling out to ver
10 participants