[Pkg-puppet-devel] [facter] 06/46: (#20236) Refactor the virtual fact and fix dmidecode
Stig Sandbeck Mathisen
ssm at debian.org
Sun Sep 1 10:47:26 UTC 2013
This is an automated email from the git hooks/post-receive script.
ssm pushed a commit to branch master
in repository facter.
commit ef8db187e4e1a6ec906dc73459a1feff2be40c9c
Author: Jeff McCune <jeff at puppetlabs.com>
Date: Mon Apr 15 22:13:47 2013 -0400
(#20236) Refactor the virtual fact and fix dmidecode
Without this patch there are a ridiculous number of problems with the
virtual fact. This patch reduces the number of problems to something
more along the lines of an absurd number of problems.
The major problem this patch addresses is that a single local variable,
`result`, is used to store the determined-thus-far value of the virtual
fact. Sometimes this value is checked to see if it's still at the
initial value of 'physical'. Only sometimes, however, most of the time
subsequent platform checks blindly overwrite the value even if we've
already made a determination.
This patch addresses the problem by using the `next` keyword to return a
value from the block passed to `setcode` as soon as a value is
determined. Only if all attempts have failed do we resort to returning
'physical' This patch completely eliminates the use of the `result`
accumulator that is constantly being overwritten.
As part of this change in behavior, the bug described in #20236 is
resolved because we no longer fail to check `dmidecode` if the output of
`lspci` does not result in a determination.
Surprisingly, the spec tests pass with almost no modification, the one
exception being an unexpected invocation on a mocked object resulting
from a confinement check added by this patch. This leads me to believe
this is a true refactor, or as close to one as we're going to get.
The patch does change the behavior substantially. If multiple results
could be determined without this patch applied then Facter would return
the value of the last determination. With this patch applied Facter
will return the first determined result. It's unclear which behavior is
expected of the system, the spec tests do not express an expected
behavior in this scenario. The comments at the top of virtual.rb lead
us to believe the first result should be returned, implying this patch
gets us a little closer to correct behavior. I suspect there will be
issues filed regardless of which way we go. There certainly have been
no end to the number of issues filed against the virtual fact thus far.
---
lib/facter/virtual.rb | 201 +++++++++++++++++++++------------------------
spec/unit/virtual_spec.rb | 1 +
2 files changed, 95 insertions(+), 107 deletions(-)
diff --git a/lib/facter/virtual.rb b/lib/facter/virtual.rb
index 0a68e14..72ef9fb 100644
--- a/lib/facter/virtual.rb
+++ b/lib/facter/virtual.rb
@@ -44,129 +44,116 @@ Facter.add("virtual") do
end
end
-
Facter.add("virtual") do
- confine :kernel => %w{Linux FreeBSD OpenBSD SunOS HP-UX GNU/kFreeBSD}
-
- result = "physical"
+ confine :kernel => ["FreeBSD", "GNU/kFreeBSD"]
setcode do
+ "jail" if Facter::Util::Virtual.jail?
+ end
+end
- if Facter.value(:kernel) == "SunOS" and Facter::Util::Virtual.zone?
- result = "zone"
- end
-
- if Facter.value(:kernel)=="HP-UX"
- result = "hpvm" if Facter::Util::Virtual.hpvm?
- end
-
- if Facter.value(:architecture)=="s390x"
- result = "zlinux" if Facter::Util::Virtual.zlinux?
- end
-
- if Facter::Util::Virtual.openvz?
- result = Facter::Util::Virtual.openvz_type()
- end
+Facter.add("virtual") do
+ confine :kernel => 'SunOS'
- if Facter::Util::Virtual.vserver?
- result = Facter::Util::Virtual.vserver_type()
+ setcode do
+ next "zone" if Facter::Util::Virtual.zone?
+
+ resolver = Facter::Util::Resolution.new('prtdiag')
+ resolver.timeout = 6
+ resolver.setcode('prtdiag')
+ output = resolver.value
+ if output
+ lines = output.split("\n")
+ next "parallels" if lines.any? {|l| l =~ /Parallels/ }
+ next "vmware" if lines.any? {|l| l =~ /VM[wW]are/ }
+ next "virtualbox" if lines.any? {|l| l =~ /VirtualBox/ }
+ next "xenhvm" if lines.any? {|l| l =~ /HVM domU/ }
end
+ end
+end
- if Facter::Util::Virtual.xen?
- if FileTest.exists?("/dev/xen/evtchn")
- result = "xen0"
- elsif FileTest.exists?("/proc/xen")
- result = "xenu"
- end
- end
+Facter.add("virtual") do
+ confine :kernel => 'HP-UX'
- if Facter::Util::Virtual.virtualbox?
- result = "virtualbox"
- end
+ setcode do
+ "hpvm" if Facter::Util::Virtual.hpvm?
+ end
+end
- if Facter::Util::Virtual.kvm?
- result = Facter::Util::Virtual.kvm_type()
- end
+Facter.add("virtual") do
+ confine :architecture => 's390x'
- if ["FreeBSD", "GNU/kFreeBSD"].include? Facter.value(:kernel)
- result = "jail" if Facter::Util::Virtual.jail?
- end
+ setcode do
+ "zlinux" if Facter::Util::Virtual.zlinux?
+ end
+end
- if Facter::Util::Virtual.rhev?
- result = "rhev"
- end
+Facter.add("virtual") do
+ confine :kernel => 'OpenBSD'
- if Facter::Util::Virtual.ovirt?
- result = "ovirt"
+ setcode do
+ output = Facter::Util::Resolution.exec('sysctl -n hw.product 2>/dev/null')
+ if output
+ lines = output.split("\n")
+ next "parallels" if lines.any? {|l| l =~ /Parallels/ }
+ next "vmware" if lines.any? {|l| l =~ /VMware/ }
+ next "virtualbox" if lines.any? {|l| l =~ /VirtualBox/ }
+ next "xenhvm" if lines.any? {|l| l =~ /HVM domU/ }
end
+ end
+end
- if result == "physical"
- output = Facter::Util::Virtual.lspci
- if not output.nil?
- output.each_line do |p|
- # --- look for the vmware video card to determine if it is virtual => vmware.
- # --- 00:0f.0 VGA compatible controller: VMware Inc [VMware SVGA II] PCI Display Adapter
- result = "vmware" if p =~ /VM[wW]are/
- # --- look for virtualbox video card to determine if it is virtual => virtualbox.
- # --- 00:02.0 VGA compatible controller: InnoTek Systemberatung GmbH VirtualBox Graphics Adapter
- result = "virtualbox" if p =~ /VirtualBox/
- # --- look for pci vendor id used by Parallels video card
- # --- 01:00.0 VGA compatible controller: Unknown device 1ab8:4005
- result = "parallels" if p =~ /1ab8:|[Pp]arallels/
- # --- look for pci vendor id used by Xen HVM device
- # --- 00:03.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01)
- result = "xenhvm" if p =~ /XenSource/
- # --- look for Hyper-V video card
- # --- 00:08.0 VGA compatible controller: Microsoft Corporation Hyper-V virtual VGA
- result = "hyperv" if p =~ /Microsoft Corporation Hyper-V/
- # --- look for gmetrics for GCE
- # --- 00:05.0 Class 8007: Google, Inc. Device 6442
- result = "gce" if p =~ /Class 8007: Google, Inc/
- end
- else
- output = Facter::Util::Resolution.exec('dmidecode')
- if not output.nil?
- output.each_line do |pd|
- result = "parallels" if pd =~ /Parallels/
- result = "vmware" if pd =~ /VMware/
- result = "virtualbox" if pd =~ /VirtualBox/
- result = "xenhvm" if pd =~ /HVM domU/
- result = "hyperv" if pd =~ /Product Name: Virtual Machine/
- result = "rhev" if pd =~ /Product Name: RHEV Hypervisor/
- result = "ovirt" if pd =~ /Product Name: oVirt Node/
- end
- elsif Facter.value(:kernel) == 'SunOS'
- res = Facter::Util::Resolution.new('prtdiag')
- res.timeout = 6
- res.setcode('prtdiag')
- output = res.value
- if not output.nil?
- output.each_line do |pd|
- result = "parallels" if pd =~ /Parallels/
- result = "vmware" if pd =~ /VMware/
- result = "virtualbox" if pd =~ /VirtualBox/
- result = "xenhvm" if pd =~ /HVM domU/
- end
- end
- elsif Facter.value(:kernel) == 'OpenBSD'
- output = Facter::Util::Resolution.exec('sysctl -n hw.product 2>/dev/null')
- if not output.nil?
- output.each_line do |pd|
- result = "parallels" if pd =~ /Parallels/
- result = "vmware" if pd =~ /VMware/
- result = "virtualbox" if pd =~ /VirtualBox/
- result = "xenhvm" if pd =~ /HVM domU/
- end
- end
- end
- end
+Facter.add("virtual") do
+ confine :kernel => %w{Linux FreeBSD OpenBSD SunOS HP-UX GNU/kFreeBSD}
- if output = Facter::Util::Resolution.exec("vmware -v")
- result = output.sub(/(\S+)\s+(\S+).*/) { | text | "#{$1}_#{$2}"}.downcase
- end
- end
+ setcode do
+ next Facter::Util::Virtual.openvz_type if Facter::Util::Virtual.openvz?
+ next Facter::Util::Virtual.vserver_type if Facter::Util::Virtual.vserver?
- result
+ if Facter::Util::Virtual.xen?
+ next "xen0" if FileTest.exists?("/dev/xen/evtchn")
+ next "xenu" if FileTest.exists?("/proc/xen")
+ end
+
+ next "virtualbox" if Facter::Util::Virtual.virtualbox?
+ next Facter::Util::Virtual.kvm_type if Facter::Util::Virtual.kvm?
+ next "rhev" if Facter::Util::Virtual.rhev?
+ next "ovirt" if Facter::Util::Virtual.ovirt?
+
+ # Parse lspci
+ output = Facter::Util::Virtual.lspci
+ if output
+ lines = output.split("\n")
+ next "vmware" if lines.any? {|l| l =~ /VM[wW]are/ }
+ next "virtualbox" if lines.any? {|l| l =~ /VirtualBox/ }
+ next "parallels" if lines.any? {|l| l =~ /1ab8:|[Pp]arallels/ }
+ next "xenhvm" if lines.any? {|l| l =~ /XenSource/ }
+ next "hyperv" if lines.any? {|l| l =~ /Microsoft Corporation Hyper-V/ }
+ next "gce" if lines.any? {|l| l =~ /Class 8007: Google, Inc/ }
+ end
+
+ # Parse dmidecode
+ output = Facter::Util::Resolution.exec('dmidecode')
+ if output
+ lines = output.split("\n")
+ next "parallels" if lines.any? {|l| l =~ /Parallels/ }
+ next "vmware" if lines.any? {|l| l =~ /VMware/ }
+ next "virtualbox" if lines.any? {|l| l =~ /VirtualBox/ }
+ next "xenhvm" if lines.any? {|l| l =~ /HVM domU/ }
+ next "hyperv" if lines.any? {|l| l =~ /Product Name: Virtual Machine/ }
+ next "rhev" if lines.any? {|l| l =~ /Product Name: RHEV Hypervisor/ }
+ next "ovirt" if lines.any? {|l| l =~ /Product Name: oVirt Node/ }
+ end
+
+ # Sample output of vmware -v `VMware Server 1.0.5 build-80187`
+ output = Facter::Util::Resolution.exec("vmware -v")
+ if output
+ mdata = output.match /(\S+)\s+(\S+)/
+ next "#{mdata[1]}_#{mdata[2]}".downcase if mdata
+ end
+
+ # Default to 'physical'
+ next 'physical'
end
end
diff --git a/spec/unit/virtual_spec.rb b/spec/unit/virtual_spec.rb
index c78870b..4aadf0e 100755
--- a/spec/unit/virtual_spec.rb
+++ b/spec/unit/virtual_spec.rb
@@ -278,6 +278,7 @@ describe "Virtual fact" do
require 'facter/util/wmi'
before do
Facter.fact(:kernel).stubs(:value).returns("windows")
+ Facter.fact(:architecture).stubs(:value).returns("x64")
end
it "should be kvm with KVM model name from Win32_ComputerSystem" do
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-puppet/facter.git
More information about the Pkg-puppet-devel
mailing list