feat: add support to differentiate specific hypervisors on s390x

[routerhan]

Resolves #2037

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: routerhan / name: ChingHan Chen (8161e6e)
• ✅ login: dependabot[bot] (7e751f0)

[k8s-ci-robot]

Welcome @routerhan!

It looks like this is your first PR to kubernetes-sigs/node-feature-discovery 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/node-feature-discovery has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @routerhan. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[netlify]

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	8161e6e
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/67ef872c0acb530008b0787a
😎 Deploy Preview	https://deploy-preview-2055--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[routerhan]

/assign @routerhan

[mfranczy]

lgtm

/cc @marquiz

[marquiz]

Thank you @routerhan for the enhancement.

Sorry, I'm a bit late in the game and rampaging here 🙄

First observation, I'd consider naming the feature hypervisor_name. The reason being that on x86_64 side we already have HYPERVISOR (flag telling if we're virtualized). We might want to add this new feature on the x86 side, and using the same name would be a bonus (HYPERVISOR and hypervisor are not truly clashing but definitely confusing if we have both).

Second thought/question. We have the cpu model feature (model and getCPUModel()) so we could consider putting this there. Then also renaming would not be important. Writing this I'm not really certain would it be more logical under cpuid or model feature 😊

Thoughts @routerhan @mfranczy @ArangoGutierrez ?

/ok-to-test

[mfranczy]

@marquiz

Writing this I'm not really certain would it be more logical under cpuid or model feature 😊

Good point! I believe cpuid should strictly refer to cases where it directly invokes the CPUID instruction for clarity.
CPU model is a better option, but I think the system category is the best choice here 😄

My reasoning is that this check primarily determines which virtualization software is running on the node, rather than verifying CPU virtualization capabilities or whether the node itself is a virtual machine.

[marquiz]

whether the node itself is a virtual machine.

Isn't this exactly what this PR is doing(?)

[mfranczy]

Isn't this exactly what this PR is doing(?)

My understanding of this PR is that it detects which hypervisor is running on the node that hosts VMs, not if it the node is a guest itself. But maybe I am wrong @routerhan?

[mfranczy]

Ah that's my bad, sorry.

Add support to differentiate hypervisors on s390x nodes. While the existing HYPERVISOR label works for KVM, it does not identify z/VM as virtual machine. Using tools like lscpu, this feature would add finer-grained labels for hypervisors such as z/VM and PR/SM (LPAR).

I should read the issue before reviewing the PR.

[routerhan]

Thank you @marquiz for the feedbacks.

Second thought/question. We have the cpu model feature (model and getCPUModel()) so we could consider putting this there. Then also renaming would not be important. Writing this I'm not really certain would it be more logical under cpuid or model feature 😊
Thoughts @routerhan @mfranczy @ArangoGutierrez ?

It also makes sense to me to put it under model feature.
So here is what I plan to do:
I'll put getHypervisor() into cpu.go file, and calling getHypervisor() from getCPUModel()

One more thing, this PR is s390x-specific solution, other architecture like x86 would throw an error, since they do not have /proc/sysinfo for detecting hypervisor information.

What do you think @marquiz @mfranczy ?

[marquiz]

It also makes sense to me to put it under model feature. So here is what I plan to do: I'll put getHypervisor() into cpu.go file, and calling getHypervisor() from getCPUModel()

One more thing, this PR is s390x-specific solution, other architecture like x86 would throw an error, since they do not have /proc/sysinfo for detecting hypervisor information.

AFAIU, /proc/sysinfo is very specific to s390x so from that point-of-view it (getHypervisor) could reside in an arch specific source file. But I think would be good enough to check the existence of /proc/sysinfo and only try to read it if it exists (not spitting out errors on other archs). And, only set the feature if hypervisor != "". So, I'll leave it up to you, as long as we don't see unnecessary misleading errors on other archs.

[marquiz]

/retest

[routerhan]

/retest

[marquiz]

Thank you @routerhan. One more ask (that I missed earlier): I think it would be nice to document this in the "Available features" table in docs/usage/customization-guide.md, even if it's just s390x for now (note that it's s390x-only)

[marquiz]

Thanks @routerhan. Looks good to me
/assign @ArangoGutierrez
ping @mfranczy

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, mfranczy, ozhuraki, routerhan

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:

• OWNERS [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment