stackhpc / ansible-role-libvirt-vm

This role configures and creates VMs on a KVM hypervisor.
128 stars 67 forks source link

XML tweaks and supporting changes #34

Closed goetzk closed 4 years ago

goetzk commented 4 years ago

Hi, Sorry if this is a bit too varied; I'm happy to do individual PRs if you would prefer.

The change series here adds several features to the XML produced (some of which were discussed last year in #17), plus supporting changes to documentation (README) and defaults/main..yml

jovial commented 4 years ago

Thanks, these look like good additions. Do you think it would be worth putting the cpu features behind flags, i.e something like:

  <features>
    {% if enable_feature_acpi %}
      <acpi/>
    {% endif %}
    {% if enable_feature_smm %}
      <smm/>
    {% endif %}
  </features>

or possibly a list? It's easier to do stuff like:

    boot_firmware: "{{ vm.boot_firmware | default('bios', true) | lower }}"
    enable_feature_acpi_default: "{{ true if boot_firmware == 'efi' else false }}"
    enable_feature_acpi: "{{ vm.enable_feature_acpi | default(enable_feature_acpi_default, true) }}"
    enable_feature_smm_default: "{{ enable_secure_boot }}"
    enable_feature_smm: "{{ vm.enable_feature_smm | default(enable_feature_smm_default, true) }}"
    enable_secure_boot: "{{ vm.enable_secure_boot | default(false, true) | bool }}"

with flags.

goetzk commented 4 years ago

It should be possible to break the features in to different groupings.

Looking at libvirt features example I wonder if flags is the most effective way for the features we're currently looking at. https://libvirt.org/formatdomain.html#elementsFeatures

For the basic self closing options a list with a loop would cover them. Example:

values:
 - pae
 - acpi
 - apic
 - hap
 - privnet

plus

{% for value in values -%}
 <{{value}}/>
{% endfor %}

giving

  <pae/>
  <acpi/>
  <apic/>
  <hap/>
  <privnet/>

The other features require more fiddling so would need extra gating/flags as you suggest.

goetzk commented 4 years ago

Hi @markgoddard , Not sure if you consider the stray <model type='virtio'/> tag issue a blocker but hoping you can review the rest of the changes I've made. If you'd like me to try and fix up the logic that could lead to stray <model type='virtio'/> I'll try and sort it out soon.

thanks,