On May 8, 2009, at 1:46 PM, [email protected] wrote:
>
> From: R.I.Pienaar <[email protected]>
>
>
> Signed-off-by: R.I.Pienaar <[email protected]>
> ---
> lib/facter/diskdrives.rb | 40 ++++++++
> lib/facter/util/diskdrives.rb | 214 ++++++++++++++++++++++++++++++++
> +++++++++
> 2 files changed, 254 insertions(+), 0 deletions(-)
> create mode 100644 lib/facter/diskdrives.rb
> create mode 100644 lib/facter/util/diskdrives.rb
>
> diff --git a/lib/facter/diskdrives.rb b/lib/facter/diskdrives.rb
> new file mode 100644
> index 0000000..90f66b4
> --- /dev/null
> +++ b/lib/facter/diskdrives.rb
> @@ -0,0 +1,40 @@
> +# diskdrives.rb
> +# Try to get additional Facts about the machine's disk drives
> +
> +require 'facter/util/diskdrives'
> +
> +Facter.add(:diskdrives) do
> + setcode do
> + Facter::Util::DiskDrives.get_drive_list.join(",")
> + end
> +end
> +
> +Facter::Util::DiskDrives.get_drive_sizes.each do |drive|
> + drive.each do |d, size|
> + Facter.add("disksize_#{d}") do
> + setcode do
> + size
> + end
> + end
> + end
> +end
> +
> +Facter::Util::DiskDrives.get_drive_types.each do |drive|
> + drive.each do |drive, type|
> + Facter.add("disktype_#{drive}") do
> + setcode do
> + type
> + end
> + end
> + end
> +end
> +
> +Facter::Util::DiskDrives.get_drive_models.each do |drive|
> + drive.each do |drive, model|
> + Facter.add("diskmodel_#{drive}") do
> + setcode do
> + model
> + end
> + end
> + end
> +end
Does the 'drive' in each of these refer to the same information?
Would it be possible to have one method that returns a hash of
information for each drive that you could then use to define your
facts? Something like:
Facter::Util::DiskDrives.drives.each do |name, data|
data.each do |param, value|
Facter.add("#{param}_#{name}") { setcode { value }}
end
end
Five lines of code isntead of 30 or so.
> diff --git a/lib/facter/util/diskdrives.rb b/lib/facter/util/
> diskdrives.rb
> new file mode 100644
> index 0000000..ba546aa
> --- /dev/null
> +++ b/lib/facter/util/diskdrives.rb
> @@ -0,0 +1,214 @@
> +# A base module for collecting Disk Drive-related
> +# information from all kinds of platforms.
> +module Facter::Util::DiskDrives
> + # All the types of drives we know about
> + DRIVETYPES = ["xvd", "ide", "scsi"]
> +
> + # platforms we work on
> + def self.supported_platforms
> + [:linux]
> + end
Is this used anywhere? Shouldn't it be used to confine the facts?
> + # fetch all known drive types and return a nice array
> + def self.get_drive_list
> + drivelist = []
> +
> + DRIVETYPES.each do |t|
> + eval("drivelist << self.get_#{t}_list")
I'd *much* prefer all of these evals use this form instead:
drivelist << send("get_#{t}_list")
We're refreshingly light on eval, and I'd like to keep it that way.
This is true for the evals below, too.
And really, this whole thing could be reduced to:
def self.get_drive_list
DRIVETYPES.collect { |t|
send("get_#{t}_list") }.compact.flattent.sort
end
None of these temporary variables are necessary.
>
> + end
> +
> + drivelist.compact.flatten.sort
> + end
> +
> + # finds out the sizes for all known drives
> + def self.get_drive_sizes
> + sizes = []
> +
> + DRIVETYPES.each do |t|
> + eval("sizes << self.get_#{t}_sizes")
> + end
> +
> + sizes.compact.flatten
> + end
> +
> + # finds out what type of drive each known drive is
> + def self.get_drive_types
> + drivetypes = []
> +
> + DRIVETYPES.each do |t|
> + drivelist = []
> + eval("drivelist << self.get_#{t}_list")
> +
> + drivelist.compact.flatten.each do |d|
> + drivetypes << {d, t} if d != nil
> + end
> + end
> +
> + drivetypes
> + end
> +
> + # find out the manufacturer/modelof each known drive
> + def self.get_drive_models
> + models = []
> + DRIVETYPES.each do |t|
> + eval("models << self.get_#{t}_models")
> + end
> +
> + models.compact.flatten
> + end
> +
> + # returns 'Xen Virtual Disk' for each xen disk
> + def self.get_xvd_models
> + case Facter.value(:kernel)
> + when 'Linux'
I'd just do 'return [] unless Facter.value(:kernel) == 'Linux''.
>
> + models = []
> + drives = self.get_xvd_list
> +
> + if drives
> + drives.each do |d|
> + models << {d, "Xen Virtual Disk"}
> + end
> + end
> +
> + models
> + end
> + end
> +
> + # returns the drive make for each SCSI disk
> + def self.get_scsi_models
> + case Facter.value(:kernel)
> + when 'Linux'
> + models = []
> + drives = self.get_scsi_list
> +
> + if drives
> + self.get_scsi_list.each do |d|
> + sfile = "/sys/block/#{d}/device/model"
> +
> + model = %x{/bin/cat #{sfile}}.chomp if
> File.exists?(sfile)
> +
> + models << {d => model} if models
> + end
> + end
> +
> + models
> + end
> + end
> +
> + # returns the drive make for each IDE disk
> + def self.get_ide_models
> + case Facter.value(:kernel)
> + when 'Linux'
> + models = []
> + drives = self.get_ide_list
> +
> + if drives
> + drives.each do |d|
> + sfile = "/proc/ide/#{d}/model"
> +
> + model = %x{/bin/cat #{sfile}}.chomp if
> File.exists?(sfile)
> +
> + models << {d => model} if models
> + end
> + end
> +
> + models
> + end
> + end
> +
> + # returns the size of each known Xen Virtual Disk
> + def self.get_xvd_sizes
> + case Facter.value(:kernel)
> + when 'Linux'
> + self.get_linux_block_sizes(self.get_xvd_list)
> + end
> + end
> +
> + # returns the size of each known SCSI Disk
> + def self.get_scsi_sizes
> + self.get_linux_block_sizes(self.get_scsi_list)
> + end
> +
> + # returns the size of each known IDE Disk
> + def self.get_ide_sizes
> + case Facter.value(:kernel)
> + when 'Linux'
The repetition of this pattern indicates to me that the fact
collection is a bit inside out: You're collecting all of the data
outside the Fact declarations, which means you can't use the 'confine'
statements.
If you can provide methods to get the appropriate data directly, then
you can confine your facts:
Facter::Util::DiskDrives.drives.each do |name, data|
data.each do |param, value|
Facter.add("#{param}_#{name}") do
confine :operatingsystem =>
Facter::Util::Drives.supported_platforms
setcode { Facter::Util::DiskDrives.get_drive_data(name, param) }
end
end
end
The only complication with this is that it results in duplication of
calls, once to define the facts and once to retrieve the value. It
simplifies your module considerably, though, and with the caching we
should be adding in 1.6 I think it will be easier to work around.
>
> + drives = self.get_ide_list
> + sizes = []
> +
> + if drives
> + drives.each do |d|
> + sfile = "/sys/block/#{d}/size"
> +
> + # this fails when running puppet as a daemon,
> unknown reason!
> + # size = File.open(sfile).gets.chomp if
> File.exists?(sfile)
> + size = %x{/bin/cat #{sfile}}.chomp if
> File.exists?(sfile)
> +
> + sizes << {d => size} if size
> + end
> + end
> +
> + sizes.size > 0 ? sizes : nil
> + end
> + end
> +
> + # reads size of all linux block devices passed as an argument
> + def self.get_linux_block_sizes(drives)
> + sizes = []
> +
> + if drives
> + drives.each do |d|
> + sfile = "/sys/block/#{d}/size"
> +
> + # this fails when running puppet as a daemon,
> unknown reason!
> + # size = File.open(sfile).gets.chomp if File.exists?
> (sfile)
> + size = %x{/bin/cat #{sfile}}.chomp if File.exists?
> (sfile)
> +
> + sizes << {d => size} if size
Why use an array of hashes, isntead of just a hash?
>
> + end
> + end
> +
> + sizes.size > 0 ? sizes : nil
> + end
> +
> + # returns a list of Xen Virtual Disks drives
> + def self.get_xvd_list
> + drives = []
> +
> + case Facter.value(:kernel)
> + when 'Linux'
> + sdir = "/sys/block"
> + drives = Dir.open(sdir).entries.grep(/^xv/) if
> File.exist?(sdir)
> + end
> +
> + drives.size > 0 ? drives : nil
> + end
> +
> + # returns a list of SCSI drives
> + def self.get_scsi_list
> + drives = []
> +
> + case Facter.value(:kernel)
> + when 'Linux'
> + sdir = "/sys/block"
> + drives = Dir.open(sdir).entries.grep(/^s/) if
> File.exist?(sdir)
> + end
> +
> + drives.size > 0 ? drives : nil
> + end
> +
> + # returns a list of IDE drives
> + def self.get_ide_list
> + drives = []
> +
> + case Facter.value(:kernel)
> + when 'Linux'
> + sdir = "/proc/ide"
> + drives = Dir.open(sdir).entries.grep(/^hd/) if
> File.exist?(sdir)
> + end
> +
> + drives.size > 0 ? drives : nil
> + end
> +end
> +
> +# vi:tabstop=4:expandtab:ai
> --
> 1.5.5.6
>
>
> >
--
Charm is a way of getting the answer yes without asking a clear
question. -- Albert Camus
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
