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

check chassis_vendor first for vm_info to allow for customized sys_vendor #52

Closed
wants to merge 2 commits into from
Closed

Conversation

asciiprod
Copy link

Hetzner Cloud uses dmi sys_vendor to identify itself (e.g. to cloud-init).
This confuses landscape, so it needs to be treated like KVM

Copy link
Contributor

@simpoir simpoir left a comment

Choose a reason for hiding this comment

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

At first glance, it looks good, but we've seen in the past that we cannot simply use sys_vendor for mapping hypervisor (e.g. amazon has both bare metal and VMs).

chassis_vendor and possibly other fields generally are more robust and is a strategy systemd-detect-virt also uses AFAIK.

I'd like to see with a dmidecode dump if there aren't more generic field we can use instead of adding another mapping.

@asciiprod
Copy link
Author

I agree that chassis_vendor is probably a better approach. Our newer QEMU/Seabios will have 'QEMU' in /sys/class/dmi/id/chassis_vendor while DO still has 'Bochs'

@asciiprod asciiprod changed the title set vm_info to kvm for Hetzner Cloud instances check chassis_vendor first for vm_info to allow for customized sys_vendor Aug 17, 2018
Copy link
Contributor

@simpoir simpoir left a comment

Choose a reason for hiding this comment

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

Thanks again for the change. It looks like the test case provided passes both with the previous order and the one in this change.
As I see it, this would only change with conflicting values between sys and chassis (e.g. sys=vmware chassis=qemu). If you know that to be a real case, please update the test to cover that, for regression purposes, otherwise it looks like this case is already covered.

@asciiprod
Copy link
Author

I am not aware of anyone else setting chassis to QEMU. So this should be much better to detect KVM based servers than the previous order. If I am not mistaken, after merging this change, you could remove Openstack and DO. I haven't checked AWS, but possibly that as well.

@simpoir
Copy link
Contributor

simpoir commented Aug 22, 2018

Thanks again for your contribution, but I'm closing this PR; the proposed change does not appear to fix any currently known issue and is unlikely to be released.

Unless there is a known issue with the detection which would warrant an SRU, this won't go through. For future releases of the client we will want to rely on the (smarter) systemd-detect-virt instead of the mapping we're using here.

@simpoir simpoir closed this Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants