add sysinfo support

[nsballmann]

For testing certain scenarios with vagrant-libvirt, we need in the guest system a value for the systems serial number in the DMI/SMBIOS system information. The domain format of libvirt allows to specify those values.

This PR adds support to do that.

While adding -smbios type=1,serial=$serial_value to the qemuargs parameter of the libvirt provider is already able to achieve this, a dedicated provider config value adds native support from the Vagrantfile layering system. For example, in the .box included ~/.vagrant.d/boxes/${box_name}/${box_version}/libvirt/Vagrantfile a random serial number can be enforced by adding the following:

require 'securerandom'
Vagrant.configure("2") do |config|
  config.vm.provider :libvirt do |libvirt|
    libvirt.dmi_system_serial = SecureRandom.alphanumeric(8).upcase
  end
end

Then in an instance specific ~/workspaces/vagrant/box_with_serial_ABCDEFGH/Vagrantfile this value can be overwritten by adding:

Vagrant.configure("2") do |config|
  config.vm.provider :libvirt do |libvirt|
    libvirt.dmi_system_serial = "ABCDEFGH"
  end
end

In the PR this is implemented in a (hopefully) purely optional fashion.

[nsballmann]

Just noticed: I forgot to add a section to the README.md. I will do that.

[electrofelix]

As these are only set for create, and will not be updated on subsequent starts, can we presume that changing this after first boot should not be supported?

I'm thinking it's probably a bad idea to change the serial, but just wanted to check in case you use case needed to consider preserving the serial to use across subsequent halts and bring back up.

Is being able to update after creation something you would need to be able to do for debugging or is it always going to to cause more issues than would be useful?

[nsballmann]

Yes, as we try to replicate "real" HW, we don't need to update the serial number between a vagrant halt and the subsequent vagrant up. The serial number shouldn't change while turning off the machine. I surely can think of scenarios, where this would be desirable, but we don't need that (for now).

I'm okay with an "update" in between a vagrant destroy and the subsequent vagrant up. The real world equivalent to that would be throwing away the old HW and buying a new one (with a new or the same serial).

[electrofelix]

Looking at this and at https://libvirt.org/formatdomain.html#smbios-system-information I think might need to come up with a way to avoid needing a new config attribute for each entry that would be useful.

It feels like would want to support being able to configure using something like the following:

require 'securerandom'
Vagrant.configure("2") do |config|
  config.vm.provider :libvirt do |libvirt|
    libvirt.sysinfo :section => 'system', :serial => SecureRandom.alphanumeric(8).upcase
  end
end

Could try to keep this relatively simple by just having it store in a hash and then expand in the template based on the keys and nested items:

<%- unless @sysinfo.empty? -%>
    <smbios mode='sysinfo'/>
<% end -%>
  </os>
<%- unless @sysinfo.empty? -%>
  <sysinfo type='smbios'>
  <%- @sysinfo.each |section, entries| do -%>
    <<%= section %>>
    <%- @entries.each |name, value| do -%>
      <entry name='<%= name %>'><%= value %></entry>
    <% end -%>
    </<%= section %>>
  <% end -%>
  </sysinfo>
<% end -%>

That should provide the ability to control most of sysinfo, and we can work out in the future potentially what ones to allow modification for subsequent boots when adding support to the start domain, for now leave it as all only being settable on create.

Can also add validation in the future for what sections are known to be valid, though might want to be soft errors where the user is warned rather than aborting if there is a mismatch.

Does this seem reasonable?

[electrofelix]

Nearly forgot, one possible improvement would be to maintain an internal list of recognized entries that can automatically be assigned a corresponding section if there is concern about making the end user interface as simple as possible.

e.g. support the following:

require 'securerandom'
Vagrant.configure("2") do |config|
  config.vm.provider :libvirt do |libvirt|
    libvirt.sysinfo :serial => SecureRandom.alphanumeric(8).upcase
  end
end

Where the function:

def sysinfo(options = {})
  entry_sections = {
    :serial => :system,
  }

  section = options.del(:section)
  entry = Hash[*options.first]
  raise "Section required for sysinfo entry #{entry.keys.first}" if section.nil? and entry_sections.fetch(entry.keys.first).nil?

  sysinfo_options[section] ||= {}
  sysinfo_options[section].merge!(entry)
end

Possibly it might make it more robust to support explicit entry name and value with section being optional to avoid collisions and store as a hash:

Vagrant.configure("2") do |config|
  config.vm.provider :libvirt do |libvirt|
    libvirt.sysinfo :name => 'serial', :value => SecureRandom.alphanumeric(8).upcase
  end
end

Either way I think need to avoid needing one config option per sysinfo entry as more are added.

[electrofelix]

@nsballmann hoping to do a release in a couple of days so if you're happy with my suggested changes to facilitate the addition of other sysinfo settings and can update the PR should be able to have it released in the next week

[nsballmann]

@electrofelix To incorporate those parts is on my agenda for a few days now. I just haven't found the time to do so.

I wanted to do sth. similar like you are proposing when I was implementing it. But being fairly new to Vagrant/Ruby syntax (and being more of a Python guy) I'm just unaware of the available options (and their syntax), so I chose to take the path I can walk on (essentially frankensteining the tpm_* domain specific options into dmi_* ones, especially with the little time I'm allowed to put into this.

But with a release next week looming, I can prioritize this and get it done today or tomorrow.

[nsballmann]

Hm... I just noticed: Maybe it's better at the beginning of create_domain.rb to only take the config values which make sense. This would simplify the template a lot.

[nsballmann]

@electrofelix Please review. :)

[nsballmann]

I still need to add some basic unit tests.

[nsballmann]

I don't understand the errors I get with the tests. Please help.

[electrofelix]

Sorry, had hoped to get to review it today, hopefully tomorrow

[electrofelix]

I think the main thing is about moving the implicit validation to the config object and making it more explicit validation. And whether to add an entry to the merge function to handle merging correctly.

Other than that there are some tricks to condensing various bits here and there, but those aren't too important.

[electrofelix]

This seems like effectively validation which should really be done in the config object if it is considered a good idea to filter out invalid entries, and also consider whether to either emit a warning or trigger an error.

Otherwise typos as someone is writing a Vagrantfile cannot be detected by the vagrant validate command and mistakes only get picked up when the machine has been brought up.

I appreciate there is probably more churn than you were expecting, so I'm happy to help move stuff around if that's alright with you.

[nsballmann]

I think I addressed this in 31a13ba.

[electrofelix]

I think this can be condensed a bit:

unless @sysinfo.empty?
  env[:ui].info(" -- Sysinfo:")
  sections = [
    [:bios, "BIOS"],
    [:system, "System"],
    [:base_board, "Base Board"],
    [:oem_strings, "OEM Strings"],
  ]

  sections.each do |section_key, description|
    section = @sysinfo.fetch(section_key, [])
    unless section.empty?
      env[:ui].info("   -- #{description}:")
      section.each do |key, value|
        env[:ui].info("    -> #{key}: #{value}")
    end
  end
end

[electrofelix]

Given the hash is being exposed directly it means that users have no concept of what can and cannot be set in the hash, adding bad values will just be ignored but there is nothing to give them any helpful suggestions or alert them when what has been inputted will be ignored.

This comes back to the implicit validation that is currently in the create domain. I think better to move that to this class and make one of two choices:

• provide a wrapper that performs validation on hash at input
• move the code confirming valid sections and entries to the validation function to run after the various Vagrantfile's are merged (I'm leaning towards this and happy to help move the code if this makes sense to you)

Probably will also want to add an entry to the merge function to ensure any sysinfo settings provided by the box Vagrantfile are deep merged with the project Vagrantfile sysinfo settings. I think it can be done via the following as we know that there are only two levels and they are hashes so there is no need to use a very complex deep merge:

result.sysinfo = sysinfo.merge(other.sysinfo) { |_k, x, y| x.merge(y) }

Though it's probably worth checking if that is desirable behaviour. I'm guessing it's likely better than requiring needing to provide a complete sysinfo hash to override a single entry?

[nsballmann]

I think I addressed the validation in 31a13ba and 4314405.
I think I addressed the merge in a8a9b28.

[electrofelix]

I'm wondering if there is some restriction with erb and string vs symbols conversion, I'd suggest using the same rather than switching between when you check the key and then attempt to access it.

[nsballmann]

I assumed they are the same and just found out they are not, especially when used as key in a hash. Now I do understand your comment. :)

[nsballmann]

I think I addressed this in 8e12797.

[nsballmann]

I think the renderer can't handle the nested hashes... I think we have to rely on a flatter hierarchy.

[nsballmann]

I'm starting to not like Ruby. I will stop here. If somebody wants to pick up the pieces, feel free. I'm done.

[electrofelix]

@nsballmann I'll look to get it over the finish line over the next few days

[electrofelix]

@nsballmann sorry to say the problem is my fault rather than ruby. The domain template test is one I wrote very early on before I really had any appreciation for how to write tests for vagrant plugins and it shows in the size of the foot canon I created.

I looked at this a couple of times over the last week and didn't understand what the issue was, just assumed it was something to do with mixing symbols and strings and a typo. It's actually caused by the test code using the config object as a proxy for what happens in the create domain action:

vagrant-libvirt/spec/unit/templates/domain_spec.rb

Lines 11 to 24 in 66e0fa7

	class DomainTemplateHelper < VagrantPlugins::ProviderLibvirt::Config
	include VagrantPlugins::ProviderLibvirt::Util::ErbTemplate
	attr_accessor :domain_volumes
	def initialize
	super
	@domain_volumes = []
	end
	def finalize!
	super
	end
	end

I had completely forgotten that this worked by assuming the instance variable names would remain the same between both.

I'll rework the test and create domain action over the following days to fix this so it's a lot more sane, by moving to a helper that can be used both by the tests and the create domain action rather than trying to have a kludge that relies on the config object.

The problem you were running into was that the contents of @sysinfo_blocks being set for the template in the test was:

{
  "bios"=>["vendor", "version", "date", "release"],
  "system"=>["manufacturer", "product", "version", "serial", "uuid", "sku", "family"],
  "base_board"=>["manufacturer", "product", "version", "serial", "asset", "location"],
  "chassis"=>["manufacturer", "version", "serial", "asset", "sku"],
  "oem_strings"=>nil,
}

Since that is what is defined on the config object, except that is not what is defined in create domain nor the structure that is expected by the template.

[nsballmann]

@electrofelix Ah okay... that makes sense. Rebasing today probably didn't include your changes, yet. Please notify me when you're done, then I'll rebase. :)

[electrofelix]

No impact, I've been tossing up whether to use a class delegating to OpenStruct to wrap the template rendering so that it can be re-used from both the create domain action and the tests, vs using a proper model that is included in both the config and a template helper class so that there is a single place for lots of the attributes to be defined instead of 3 or 4 which definitely contributed to making this much harder for you to contribute.

I'll look to try and update your PR in the next couple of days, once I've thought a little more and decided whether it needs the underlying problem tackled first to unpick the mess or it'll remain reasonably easy to sort after landing this.

[coveralls]

Coverage remained the same at 77.804% when pulling 95a2253 on nsballmann:master into 37c3330 on vagrant-libvirt:master.

[electrofelix]

So I also managed to remind myself of one of the glaring annoying issues around config validation, in that it's not possible to warn users about things being skipped outside of the validation function. This means need to retain the original sysinfo in the config object instead of filtering during finalize! as otherwise validation can't flag that there are a number of entries getting skipped/ignored.

I've also run into some fun where lots of strings as keys are getting converted to symbols without it being exactly clear what is happening. Possibly frozen strings are causing a few issues here, but the solution appears to be just to ensure lots of use of to_s when handling keys to avoid issues either way.

I may move some stuff around again at a later time when possibly switching to model for the domain pieces to allow reuse between config, create domain and tests for both, for now it should work with the additional tests covering all areas.

This was definitely far more troublesome that I originally thought when asking you to extend to cover all of the sysinfo parts, sorry about that.

[electrofelix]

@nsballmann I'll try and summarize what way I've gone with the changes, let me know what you think, as I'd like to get some feedback based on how you hope to use it as there is no guarantee I've got it right.

• Made validation more strict for incorrect block or entry names where there are explicitly defined entries, while retaining the warning about dropping empty entries.
  • For invalid blocks or entries I've found it to be more frustrating in the past to have something continuing to the next stage for something that was a typo rather than stopping and rejecting immediately.
  • Based on how the merging takes place between Vagrantfile files it's useful to be able to set an entry in a later Vagrantfile to '' or nil to unset what was defined earlier, but worthwhile letting the user know that this is happening.
• Followed the style of anything that is directly accessible starting with UNSET_VALUE rather than a hash, while only initializing attributes that are accessed via custom setter methods (e.g. disks, cdroms, etc) to an array or hash.
  • This may be revisited in the future, but figured for now just follow the existing style.
• Filtered sysinfo in the create domain action for now
  • Filtering in finalize! requires retaining an original copy in order for validation to emit warnings for the empty values being discarded, as there is no access to the ui to emit warnings from anywhere except validation and that is called after finalize! which jars as tend no to do this filtering for anything else there.
  • Also think too much logic has been going into the template so looking to avoid adding more and will in time look to move some of the filtering logic back into create_domain.rb file.
  • The filtering appears to be generic enough that it should be possible to make it available for reuse similar to how https://github.com/rails/rails/blob/962a1dd231de475130b78f4357b090a6bb28709d/activesupport/lib/active_support/core_ext/enumerable.rb#L278-L281 works, just maybe as a separate change

[electrofelix]

@nsballmann did you get a chance to have a look to see if my tweaks made sense? Or do they cause any issues?

Sorry for the delay in following up, I was trying to line up a change to migrate the docs to GitHub pages

[nsballmann]

I'm sorry, I haven't had the time, yet. I will have a look next week.