Allow specification of multiple VMs #1

w-miller · 2018-07-17T09:10:15Z

Also some other minor fixes.

markgoddard

Nice, looks like a good start.

markgoddard · 2018-07-17T10:06:06Z

tasks/destroy-volumes.yml

@@ -4,8 +4,9 @@
    destroy_virt_volume.sh
    {{ item.name }}
    {{ item.pool }}
-  with_items: "{{ libvirt_vm_volumes }}"
+  with_items: "{{ vm.volumes | default([]) }}"


I'm not sure that this default will work in the same way as before. The default filter only applies the default if the variable is undefined. I think there are two potential solutions. Either omit the deprecated variables from defaults/main.yml, such that they are actually undefined, or use the boolean argument to default. Possibly the first is more consistent.

See http://jinja.pocoo.org/docs/2.10/templates/#default.

Same goes for other defaults.

Good point. I don't think omitting the deprecated variables from defaults/main.yml will work, because they are being referenced in the default singleton definition of libvirt_vms, and we get an error of referencing an undefined variable. Even if we use default(omit) filters in each element of the libvirt_vms definition, omitting role variables doesn't work.

I guess using default's boolean argument is the best option.

Having said all that, using your vars suggestion means we don't need the default filters at all!

Did a little testing and looks like you're right. Don't we need to use default when defining the vars though?

markgoddard · 2018-07-17T10:09:15Z

tasks/vm.yml

@@ -1,7 +1,7 @@
 ---
 - name: Ensure the VM console log directory exists
  file:
-    path: "{{ libvirt_vm_console_log_path | dirname }}"
+    path: "{{ vm.console_log_path | default('/var/log/libvirt-consoles/' +  vm.name + '.log') | dirname }}"


Might improve usability to make the default directory configurable.

markgoddard · 2018-07-17T10:14:11Z

templates/vm.xml.j2

  <os>
-    <type arch='{{ libvirt_vm_arch }}'{% if libvirt_vm_machine is not none %} machine='{{ libvirt_vm_machine }}'{% endif %}>hvm</type>
+    <type arch='{{ libvirt_vm_arch }}'{% if vm.machine is defined %} machine='{{ vm.machine | default('pc-1.0') }}'{% endif %}>hvm</type>


Not sure the logic is quite right here. Docs say:

Default is None if engine is kvm, otherwise pc-1.0.

You can use {% set var=val %} if the logic gets hairy.

markgoddard · 2018-07-17T10:15:01Z

templates/vm.xml.j2

    <bootmenu enable='no'/>
+    <boot dev='hd'/>
+    <boot dev='network'/>


I guess this will continue to work for non network-booted VMs?

Looks like when I boot a VM with a hard drive and no network interface, it successfully boots from the hard drive even with this configuration.

markgoddard · 2018-07-17T10:17:18Z

templates/vm.xml.j2

    <bios useserial='yes'/>
  </os>
-  <cpu{% if libvirt_vm_cpu_mode is not none %} mode='{{ libvirt_vm_cpu_mode }}'{% endif %}>
+  <cpu mode='{% if vm.cpu_mode is defined and vm.cpu_mode is not none %}{{ vm.cpu_mode }}{% elif libvirt_vm_engine == 'kvm' %}host_passthrough{% else %}host-model{% endif %}'>


host-passthrough

markgoddard · 2018-07-17T10:20:01Z

templates/vm.xml.j2

     </serial>
     <serial type='pty'/>
     <console type='file'>
-       <source path='{{ libvirt_vm_console_log_path }}'/>
+       <source path='{{ vm.console_log_path | default('/var/log/libvirt-consoles/' + vm.name + '.log') }}'/>


If you wanted to avoid repeating this logic, you could define a per-task variable. You could even do something like this in main.yml:

- include_tasks: create_vm.yml vars: vm_console_path: "{{ vm.console_log_path | default('/var/log/libvirt-consoles/' + vm.name + '.log') }}" ...

Can't believe I didn't think of that - much better idea.

Adapt the role to allow a list of VM specifications. This involves: * Splitting libvirt_vm_* variables into role-scope and VM-scope. * Deprecating the old set of variables.

Libvirt pools are created with `become` in ansible-role-libvirt-host, so the volumes created here must also use `become` to ensure they can access the created pools.

Also, set power toggles for virsh.

w-miller · 2018-07-17T16:20:44Z

Updated and ready for re-review.

markgoddard · 2018-07-17T17:04:11Z

tasks/main.yml

  with_items: "{{ libvirt_vms }}"
  loop_control:
    loop_var: vm
  when: (vm.state | default('present')) == 'present'

 - include_tasks: vm.yml
+  vars:
+    libvirt_vm_console_log_path: "{{ vm.console_log_path | default(libvirt_vm_default_console_log_dir + '/' +  vm.name + 'console.log') }}"


Does this need a hyphen before console.log?

markgoddard · 2018-07-17T17:07:54Z

tasks/main.yml

+    cpu_mode_default: "{{ 'host-passthrough' if libvirt_vm_engine == 'kvm' else 'host-model' }}"
+    libvirt_vm_cpu_mode: "{{ vm.cpu_mode | default(cpu_mode_default) }}"
+    libvirt_vm_volumes: "{{ vm.volumes | default([]) }}"
+    libvirt_vm_interfaces: "{{ vm.interfaces | default([]) }}"


Are there any negative consequences of reusing these variable names?

I didn't think there would be, but I shouldn't have been so naïve. Testing revealed some strange errors in variable scope when redefining these so I'll give them different names.

markgoddard · 2018-07-17T17:19:17Z

defaults/main.yml

+# See README.md or tasks/main.yml for these attributes' defaults.
+libvirt_vms:
+      # State of the VM. May be 'present' or 'absent'.
+    - state: "{{ libvirt_vm_state }}"


I still don't think the defaults will work correctly in this case, since the variable is defined - it will be None. How about just keeping the old defaults on the deprecated variables?

Yeah, true. It seems more consistent to use the boolean flag in the default filter for all the variables though, rather than putting some of the defaults in here.

Also, ensure rejectattr condition works properly, and use boolean mode for default filters so defaults work in the case of using deprecated variables.

w-miller · 2018-07-18T10:42:26Z

Updated and ready for re-review. I've tried to test a few scenarios a bit more thoroughly this time, with regard to defaults.

/var/log/libvirt/ has permissions issues.

markgoddard

This looks sensible to me.

w-miller · 2018-08-02T14:46:36Z

Does this need some more reviewers?

Logging the console to a file prevents a serial PTY from also being available, which is not ideal. Add a toggle to enable console logging which defaults to false, meaning the old PTY behaviour is maintained by default.

Console log file enabled toggle

w-miller · 2018-08-03T13:39:01Z

Since I'd based this branch off support-virt-engine which had console logging enabled already, I've merged the console logging toggle commits into this pull request.

markgoddard

Great stuff Will.

Merge

markgoddard and others added 2 commits April 28, 2018 17:56

Add support for logging serial console to a file

3e886a7

Update README to reflect new API for multiple VMs

48b2a25

markgoddard suggested changes Jul 17, 2018

View reviewed changes

Will Miller added 6 commits July 17, 2018 15:12

Allow specification of multiple VMs

e8e3b4c

Adapt the role to allow a list of VM specifications. This involves: * Splitting libvirt_vm_* variables into role-scope and VM-scope. * Deprecating the old set of variables.

Ensure libvirt module tasks use become

543ac79

Libvirt pools are created with `become` in ansible-role-libvirt-host, so the volumes created here must also use `become` to ensure they can access the created pools.

Remove use of deprecated success filter

0d9b954

Set boot order to hard drive, then network

10f7947

Also, set power toggles for virsh.

Add default console log and iamge cache paths

2156bd3

Set VM attribute defaults in main.yml

e90b854

w-miller force-pushed the vm_list2 branch from 7669896 to e90b854 Compare July 17, 2018 16:19

w-miller closed this Jul 17, 2018

w-miller reopened this Jul 17, 2018

markgoddard reviewed Jul 17, 2018

View reviewed changes

Will Miller added 3 commits July 18, 2018 10:30

Ensure we fail if VM name, memory, vcpus not set

e5cf3c3

Don't reuse variable names in playbooks

d7bb5b4

Also, ensure rejectattr condition works properly, and use boolean mode for default filters so defaults work in the case of using deprecated variables.

Add hyphen to console log path

f1683c9

Use /var/log/libvirt-consoles/ as default log dir

70bde8b

/var/log/libvirt/ has permissions issues.

markgoddard approved these changes Jul 20, 2018

View reviewed changes

markgoddard force-pushed the support-virt-engine branch from 3e886a7 to da56e8c Compare August 2, 2018 15:41

markgoddard changed the base branch from support-virt-engine to master August 2, 2018 15:43

Will Miller added 2 commits August 3, 2018 11:01

Remove trailing whitespace in README

397ba6f

Make console logging optional

9ed04fd

Logging the console to a file prevents a serial PTY from also being available, which is not ideal. Add a toggle to enable console logging which defaults to false, meaning the old PTY behaviour is maintained by default.

w-miller mentioned this pull request Aug 3, 2018

WIP: Console log file 2 w-miller/ansible-role-libvirt-vm#1

Merged

Merge pull request #1 from w-miller/console-log-file-2

4f92ba8

Console log file enabled toggle

markgoddard approved these changes Aug 3, 2018

View reviewed changes

markgoddard merged commit baba356 into stackhpc:master Aug 3, 2018

w-miller deleted the vm_list2 branch August 16, 2018 10:39

markgoddard pushed a commit that referenced this pull request Oct 5, 2020

Merge pull request #1 from stackhpc/master

d4d6a68

Merge

Allow specification of multiple VMs #1

Allow specification of multiple VMs #1

Uh oh!

Conversation

w-miller commented Jul 17, 2018

Uh oh!

markgoddard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w-miller Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w-miller commented Jul 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w-miller commented Jul 18, 2018

Uh oh!

markgoddard left a comment

Choose a reason for hiding this comment

Uh oh!

w-miller commented Aug 2, 2018

Uh oh!

w-miller commented Aug 3, 2018

Uh oh!

markgoddard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

w-miller Jul 17, 2018 •

edited

Loading