Skip to content

Conversation

@caseywilliams
Copy link
Contributor

Three commits:

Previously, the DMI data source collected data in its constructor, so even when DMI wouldn't have been used anyway, it would still try to collect on instantiation. The first commit here does the following:

  • changes the DMI source so that it only collects data once one of the data values is accessed, and
  • renames dmi_base to smbios_base, since DMI isn't an accurate name on windows or solaris

The second commit adds an implementation of the smbios_base using WMI. This worked fine in manual tests, but there are no automated tests here (yet?).

The third commit adds the Hyper-V detector. Checking the CPUID result for Microsoft Hv seems to be a reliable way to do this, but SMBIOS is checked as a fallback.

(Also, it occurred to me that this will have a few test fixture conflicts with the xen PR I have open now (#15) - I'll fix/rebase whichever as necessary)

@caseywilliams caseywilliams changed the title Add Hyper-V detector (FACT-1667) Add Hyper-V detector Aug 17, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.9%) to 95.636% when pulling 7768590 on caseywilliams:hyperv-detector into 7cc2bfd on puppetlabs:master.

@caseywilliams caseywilliams requested a review from branan August 17, 2017 00:54

namespace whereami { namespace detectors {

result hyperv(const sources::cpuid_base& cpuid_source, sources::smbios_base& dmi_source)

Choose a reason for hiding this comment

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

Nit - did you also want to rename dmi_source to smbios_source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I forgot to change this name - thanks!

{
result res {vm::hyperv};

if (cpuid_source.vendor() == "Microsoft Hv") {

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, missed that - should I try to rule out Virtual PC before validating Hyper-V? Or do we care?

@Iristyle
Copy link

Some interesting notes about parts of CPUID behaving strangely for other things when checking from inside Hyper-V - https://software.intel.com/en-us/forums/virtualization-software-development/topic/701229

Don't know if any of that impacts us, but thought I'd mention it.

@caseywilliams
Copy link
Contributor Author

Hmm, good find. In that thread, it looks like their issues revolve around Microsoft deliberately obscuring CPUID leaves that describe the capabilities of the physical processor within Hyper-V. I would hope that if MS is making adjustments like this, they'd leave the vendor leaf alone, since it's (supposed to be) reporting the presence of Hyper-V anyway (as opposed to physical information). That said, even if it reports the physical vendor or nothing, that seems okay to me, since the fallback SMBIOS vendor information comes from the hardware on a physical PC and (at least as far as I've seen) only reports "Microsoft" for Hyper-V.

I have a few little cleanup tasks to do here - found a few misspellings, etc. Updating shortly.

@Iristyle
Copy link

Yeah, I was less concerned about use of CPUID in this PR which seems fine, and more curious about whether or not there are other facts we look to CPUID for that might behave incorrectly inside Hyper-V.

@caseywilliams
Copy link
Contributor Author

Oh, yeah - In this library we've only used leaves for the vendor and the "hypervisor present" bit (last one here), which seems safe (Facter currently relies on virt-what to make these same queries). I don't expect we'll make any use of other leaves.

Casey Williams added 3 commits August 18, 2017 12:30
Previously, the DMI data source collected data in its constructor, so
even when DMI wouldn't have been used anyway, it would still try to
collect its data from dmidecode and /sys on instantiation. This commit
changes the DMI source so that it only collects data when one of data
values is accessed.

Also renames DMI to SMBIOS.
Adds a Windows WMI implementation of smbios_base
Adds a Hyper-V detector, which returns a valid result when:

- the CPUID instruction's vendor string is "Microsoft Hv", or
- the SMBIOS manufacturer contains "Microsoft"
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.9%) to 95.7% when pulling b8204ad on caseywilliams:hyperv-detector into 325cda6 on puppetlabs:master.

@branan branan merged commit 3672731 into puppetlabs:master Aug 25, 2017
@caseywilliams caseywilliams deleted the hyperv-detector branch November 7, 2017 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants