Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Libvirt #2

Merged
merged 7 commits into from
May 26, 2020
Merged

Libvirt #2

merged 7 commits into from
May 26, 2020

Conversation

greg-hellings
Copy link
Contributor

@greg-hellings greg-hellings commented Apr 16, 2020

Adds several new roles that can be useful:

  • libvirtd installs libvirt on RHEL/Fedora/Ubuntu
  • network_scripts templates out a set of ifcfg-* scripts to the host and optionally restarts networking after they're in place
  • libvirt_rhel_vm uploads a RHEL/CentOS cloud image to the server and slightly modifies it to be usable as a libvirt image, then starts a vm from it

@greg-hellings
Copy link
Contributor Author

This is the first PR into the Ansible collection. Please be kind about what you see and what you find. But don't hesitate to open issues if there are problems beyond the ken of this pull that need fixing or updating.

Copy link
Contributor

@tehsmyers tehsmyers left a comment

Choose a reason for hiding this comment

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

I didn't go nuts looking for grammers and stuffs, functionally these all look good.

{% if 'block' in item %}
{{ item['block']|string }}
{% endif %}
{% if 'BONDING_OPTS' in item %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a loop be handy here, with all the templated VAR_NAMES in some vars array (ifcfg_vars used in this suggestion)?

{% for VAR in ifcfg_vars %}
{% if VAR in item %}{{ VAR }}="{{ item[VAR] | string }}"{% endif %}
{% endfor %}

From a maintenance standpoint, though, I'm not sure if I prefer the loop approach or just writing it all out, since it's way simpler, if slightly tedious, to come in here and just fix what needs fixing in the unlikely event that new and interesting things find their way into ifcfg scripts. I'm indifferent. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wishy-washy on the difference there. I like explicit. Because it's an easy place to come reference and because it avoids someone going:

networks:
  DERPDERP_MC_DERP: 123

And then going, New Issue -> "It's broke"

But I also like generic because it allows a user to go, "I know more than Greg does" and do the things they want.

roles/libvirt_rhel_vm/tasks/modify_image.yml Show resolved Hide resolved
@@ -0,0 +1,54 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious as to why this must be written in bash vs using the Ansible command or shell modules. If we need to do it this way, we should ensure it passes shellcheck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strictly speaking nothing in this MUST be written in a separate bash file. A few considerations and possibilities:

  1. It does use quite a bit of Jinja2 templating, including things like loops, which make writing it inline in Ansible non-trivial. So having it as a template file is definitely desirable for that matter. We gain syntax highlighting for the most part, and don't clutter up the yaml file.
  2. The aforementioned templating makes it difficult or impossible to pass to Shellcheck. Jinja2 formatting keys are pretty much incompatible with Shellcheck because it chokes on the { character outside of quotation marks/function definitions.
  3. If you would like this executed directly by the shell instead of being uploaded to a tempfile and then executed with "command", I can do that by just having shell execute the result of "{{ lookup('template', 'setup_domain.sh') }}" here. In general, though, command is to be preferred over shell where possible.

That said, I have cleaned up one lint error in the script.

Obviously, the ideal would be if we had Ansible modules to do this work. But that would be a very tall mountain to climb for this one-off scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote a somewhat similar bit of automation using Ansible YAML. Consider the following snipped code:

tasks/main.yml:

- name: Make the VMs be
  include_tasks: create_vm.yml
  loop: "{{ vms }}"

tasks/create_vm.yml:

- name: check if the VM exists
  stat:
    path: /etc/libvirt/qemu/{{ item.hostname }}.xml
  register: vm_file

- name: create VM if necessary
  block:
    - name: calculate RAM
      set_fact:
        ram_real: "{{ item.ram | int * 1024 }}"

   - name: do virt-install
      block:
        - name: construct virt-install command-line
          set_fact:
            cli: >
              virt-install
              --name {{ item.hostname }}
              --memory {{ ram_real | default('1024') }}
              --arch x86_64
              --disk pool=default,size={{ item.disk | int }}
              --vcpus {{ item.vcpus | default('2') }}
              --controller usb3
              --rng /dev/urandom
              --os-type linux
              {% if dist_info[item.dist].variant is defined %}
              --os-variant {{ dist_info[item.dist].variant }}
              {% endif %}
              --network bridge={{ network_bridge }}
              --location {{ dist_info[item.dist].iso }}
              {% if ks_path is defined %}
              --initrd-inject={{ ks_path }}
              --extra-args=ks=file:{{ ks_file }}
              {% endif %}

        - name: print virt-install command-line
          debug:
            msg: "{{ cli }}"

        - name: run virt-install
          command: "{{ cli }}"


  when: not vm_file.stat.exists


Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been trying to make sense of how this whole script could be done with playbook commands. In order to achieve a measure of idempotence across the four commands it would need to be quite a series of five commands, none of which would be easy to run idempotently on its own. It would be something like:

  • check if VM exists
  • if it doesn't exist:
    • create backing image from uploaded image
    • resize disk
    • edit VM image
    • run VM
    • indicate changed if these 4 complete successfully
  • else:
    • indicate no change needed
  • trap error if any of those 4 steps error or if check errored

Since no modules seem to exist, at least in Ansible itself, to manage any of those tasks, wrapping it all into a shell script seems to do a pretty neat job. It handles a modicum of idempotence (at least at the level of if the domain already exists), and there isn't an easy interface to managing images and libvirt in Ansible without knowing how to write libvirt's XML. And even that doesn't have any support for the virt-customize call that's in the script.

If we're going to tear apart this shell script, I think that needs to be a separate task where we write modules for things like image management and virt-customize and we take the time to learn libvirt's XML format.

roles/network_scripts/README.md Outdated Show resolved Hide resolved
For libvirt_rhel_vm
network_bridge
network_scripts
@pcahyna
Copy link

pcahyna commented May 14, 2020

@greg-hellings would the linux-system-roles/network role with its initscripts provider work, instead of creating a special network_scripts role? @tyll

@pcahyna
Copy link

pcahyna commented May 14, 2020

@mwperina @mnecas is there an existing libvirt role that you are working on? Or are you working only on oVirt roles?

network_scripts_dest_path: "{{ libvirt_rhel_vm_nic_config_path }}"
network_scripts_nics: "{{ libvirt_rhel_vm_domain.nics }}"
network_scripts_restart: false
import_role:
Copy link

Choose a reason for hiding this comment

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

@mnecas
Copy link

mnecas commented May 14, 2020

@mwperina @mnecas is there an existing libvirt role that you are working on? Or are you working only on oVirt roles?

Sorry cannot help, only oVirt roles/collection.

@tyll
Copy link

tyll commented May 14, 2020

@greg-hellings would the linux-system-roles/network role with its initscripts provider work, instead of creating a special network_scripts role? @tyll

If they do not need special features from network-scripts that the role does not provide, sure. Not sure why it is using network-scripts instead of NetworkManager in the first place.

@greg-hellings
Copy link
Contributor Author

The network scripts are used, instead of NetworkManager, because those scripts are being passed in through virsh to a target VM.

@greg-hellings greg-hellings merged commit a5e9cae into oasis-roles:master May 26, 2020
@greg-hellings greg-hellings deleted the libvirt branch May 26, 2020 17:02
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.

6 participants