-
Notifications
You must be signed in to change notification settings - Fork 293
feat(cpu): detect hypervisor on x86_64 and arm64 via CPUID #2388
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
base: master
Are you sure you want to change the base?
feat(cpu): detect hypervisor on x86_64 and arm64 via CPUID #2388
Conversation
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Welcome @whatever125! |
|
Hi @whatever125. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
marquiz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @whatever125. A few comments below
source/cpu/cpu.go
Outdated
| hv := strings.ToLower(cpuid.CPU.HypervisorVendorID.String()) | ||
|
|
||
| switch hv { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| hv := strings.ToLower(cpuid.CPU.HypervisorVendorID.String()) | |
| switch hv { | |
| switch hv := strings.ToLower(cpuid.CPU.HypervisorVendorID.String()); hv { |
source/cpu/cpu.go
Outdated
| case "qemu": | ||
| return "qemu", nil | ||
| case "apple": | ||
| return "apple", nil | ||
| case "qnx": | ||
| return "qnx", nil | ||
| case "acrn": | ||
| return "acrn", nil | ||
| case "sre": | ||
| return "sre", nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we put all these cases where we don't mangle the value under the default case?
| case "qemu": | |
| return "qemu", nil | |
| case "apple": | |
| return "apple", nil | |
| case "qnx": | |
| return "qnx", nil | |
| case "acrn": | |
| return "acrn", nil | |
| case "sre": | |
| return "sre", nil | |
| default: | |
| return hv, nil |
source/cpu/cpu.go
Outdated
| if _, err := os.Stat("/proc/sysinfo"); err == nil { | ||
| data, err := os.ReadFile("/proc/sysinfo") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would make the code more maintainable and readable to split it into multiple functions, something like:
| if _, err := os.Stat("/proc/sysinfo"); err == nil { | |
| data, err := os.ReadFile("/proc/sysinfo") | |
| if _, err := os.Stat("/proc/sysinfo"); err == nil { | |
| return getHypervisorFromProcSysinfo | |
| } else { | |
| klog.Error(err, "failed to stat /proc/sysinfo") | |
| } | |
| if cpuid.CPU.VM() { | |
| return getHyperVisorFromCPUID(), nil | |
| } | |
| return "none", nil |
Also, if the stat fails, we might still want to fall back to the cpuid method (like above). Thoughts?
|
/ok-to-test |
|
Thank you for the feedback @marquiz. I've updated the code as you suggested:
|
source/cpu/cpu.go
Outdated
| hv, discovered := getHypervisorFromProcSysinfo() | ||
| if discovered { | ||
| return hv, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this does not find anything (discovered == false), do we want to fall back to getHypervisorFromCPUID()? Probably not, or WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after digging deeper, i've reconsidered the logic:
- PR/SM virtualization is always present on s390x, so it should be the default fallback value (consistent with the original code)
/proc/sysinfoexists only on s390x. its absence means another arch -> go to cpuid- not being able to read it -> just log and return the error
- cpuid doesn't support s390x, so we shouldn't fall back to it in any case
- parsing
/proc/sysinfoand not finding "Control Program:" means PR/SM virtualization by default -> return "PR/SM", not a blank string
so this is what i propose:
func getHypervisor() (string, error) {
// use /proc/sysinfo for s390x
if _, err := os.Stat("/proc/sysinfo"); err == nil {
return getHypervisorFromProcSysinfo()
}
// use cpuid lib for x86/arm
if cpuid.CPU.VM() {
return getHypervisorFromCPUID()
}
// no hv discovered
return "none", nil
}and then the getHypervisorFromProcSysinfo() is just the old getHypervisor().
maybe we could use runtime.GOARCH == "s390x" for a cleaner arch detection instead of doing stat on /proc/sysinfo. what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could use
runtime.GOARCH == "s390x"for a cleaner arch detection instead of doing stat on/proc/sysinfo. what do you think?
That works 👍 Would make sense with the fact that /proc/sysinfo only exists on s390x
|
@marquiz thanks! i've updated the code to use |
52a922f to
6a0f931
Compare
marquiz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice @whatever125. This became really clean and simple.
/assign @ArangoGutierrez @fmuyassarov
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marquiz, whatever125 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Added detection of the virtualization platform on x86/arm64 archs using klauspost/cpuid lib.
/proc/sysinfo.cpu.model.hypervisornow reports hypervisor type, ornonefor bare-metal, and raw CPUID string for unknown hypervisors.Resolves #2267