Add disk driver options with minor refactor (#1000)

Adds disk driver options: io, copy_on_read, discard and detect_zeroes
for both the primary vm disk and additional disks.

Minor refactor of existing volume_cache to deprecate and replace with a
single call to disk_driver that contains all of the options. Usage of
the volume_cache option will now result in a message to ui that it has
been replaced, as well as a warning that it is ignored if disk_driveris
set.

The old option volume_cache is only used if disk_driver is not present
(even if :cache is not set - in that case, the hypervisor default is
always used).

Resolves #998

README.md

@@ -387,6 +387,14 @@ end
   set, which should be fine for paravirtualized guests, but some fully
   virtualized guests may require hda. NOTE: this option also applies only to
   disks associated with a box image.
+* `disk_driver` - Extra options for the main disk driver ([see Libvirt documentation](http://libvirt.org/formatdomain.html#elementsDisks)).
+  NOTE: this option also applies only to disks associated with a box image. In all cases, the value `nil` can be used to force the hypervisor default behaviour (e.g. to override settings defined in top-level Vagrantfiles). Supported options include:
+  * `:cache` - Controls the cache mechanism. Possible values are "default", "none", "writethrough", "writeback", "directsync" and "unsafe".
+  * `:io` - Controls specific policies on I/O. Possible values are "threads" and "native".
+  * `:copy_on_read` - Controls whether to copy read backing file into the image file. The value can be either "on" or "off".
+  * `:discard` - Controls whether discard requests (also known as "trim" or "unmap") are ignored or passed to the filesystem. Possible values are "unmap" or "ignore".
+    Note: for discard to work, you will likely also need to set `disk_bus = 'scsi'`
+  * `:detect_zeroes` - Controls whether to detect zero write requests. The value can be "off", "on" or "unmap".
 * `nic_model_type` - parameter specifies the model of the network adapter when
   you create a domain value by default virtio KVM believe possible values, see
   the [documentation for
@@ -432,10 +440,6 @@ end
   ]
   ```
 * `loader` - Sets path to custom UEFI loader.
-* `volume_cache` - Controls the cache mechanism. Possible values are "default",
-  "none", "writethrough", "writeback", "directsync" and "unsafe". [See
-  driver->cache in Libvirt
-  documentation](http://libvirt.org/formatdomain.html#elementsDisks).
 * `kernel` - To launch the guest with a kernel residing on host filesystems.
   Equivalent to qemu `-kernel`.
 * `initrd` - To specify the initramfs/initrd to use for the guest. Equivalent
@@ -536,7 +540,7 @@ Vagrant.configure("2") do |config|
       domain.memory = 2048
       domain.cpus = 2
       domain.nested = true
-      domain.volume_cache = 'none'
+      domain.disk_driver :cache => 'none'
     end
   end
 
@@ -846,11 +850,6 @@ It has a number of options:
 * `size` - Size of the disk image. If unspecified, defaults to 10G.
 * `type` - Type of disk image to create. Defaults to *qcow2*.
 * `bus` - Type of bus to connect device to. Defaults to *virtio*.
-* `cache` - Cache mode to use, e.g. `none`, `writeback`, `writethrough` (see
-  the [libvirt documentation for possible
-  values](http://libvirt.org/formatdomain.html#elementsDisks) or
-  [here](https://www.suse.com/documentation/sles11/book_kvm/data/sect1_chapter_book_kvm.html)
-  for a fuller explanation). Defaults to *default*.
 * `allow_existing` - Set to true if you want to allow the VM to use a
   pre-existing disk. If the disk doesn't exist it will be created.
   Disks with this option set to true need to be removed manually.
@@ -858,13 +857,25 @@ It has a number of options:
 * `serial` - Serial number of the disk device.
 * `wwn` - WWN number of the disk device.
 
+The following disk performance options can also be configured
+(see the [libvirt documentation for possible values](http://libvirt.org/formatdomain.html#elementsDisks)
+or [here](https://www.suse.com/documentation/sles11/book_kvm/data/sect1_chapter_book_kvm.html) for a fuller explanation).
+In all cases, the options use the hypervisor default if not specified, or if set to `nil`.
+
+* `cache` - Cache mode to use. Value may be `default`, `none`, `writeback`, `writethrough`, `directsync` or `unsafe`.
+* `io` - Controls specific policies on I/O. Value may be `threads` or `native`.
+* `copy_on_read` - Controls whether to copy read backing file into the image file. Value may be `on` or `off`.
+* `discard` - Controls whether discard requests (also known as "trim" or "unmap") are ignored or passed to the filesystem. Value may be `unmap` or `ignore`.
+  Note: for discard to work, you will likely also need to set `:bus => 'scsi'`
+* `detect_zeroes` - Controls whether to detect zero write requests. Value may be `off`, `on` or `unmap`.
+
 The following example creates two additional disks.
 
 ```ruby
 Vagrant.configure("2") do |config|
   config.vm.provider :libvirt do |libvirt|
     libvirt.storage :file, :size => '20G'
-    libvirt.storage :file, :size => '40G', :type => 'raw'
+    libvirt.storage :file, :size => '40G', :bus => 'scsi', :type => 'raw', :discard => 'unmap', :detect_zeroes => 'on'
   end
 end
 ```

lib/vagrant-libvirt/action/create_domain.rb

@@ -55,11 +55,12 @@ module VagrantPlugins
           @machine_arch = config.machine_arch
           @disk_bus = config.disk_bus
           @disk_device = config.disk_device
+          @disk_driver_opts = config.disk_driver_opts
           @nested = config.nested
           @memory_size = config.memory.to_i * 1024
           @memory_backing = config.memory_backing
           @management_network_mac = config.management_network_mac
-          @domain_volume_cache = config.volume_cache
+          @domain_volume_cache = config.volume_cache || 'default'
           @kernel = config.kernel
           @cmd_line = config.cmd_line
           @emulator_path = config.emulator_path
@@ -250,7 +251,13 @@ module VagrantPlugins
           end
           env[:ui].info(" -- Storage pool:      #{@storage_pool_name}")
           env[:ui].info(" -- Image:             #{@domain_volume_path} (#{env[:box_virtual_size]}G)")
-          env[:ui].info(" -- Volume Cache:      #{@domain_volume_cache}")
+
+          if not @disk_driver_opts.empty?
+            env[:ui].info(" -- Disk driver opts:  #{@disk_driver_opts.reject { |k,v| v.nil? }.map { |k,v| "#{k}='#{v}'"}.join(' ')}")
+          else
+            env[:ui].info(" -- Disk driver opts:  cache='#{@domain_volume_cache}'")
+          end
+
           env[:ui].info(" -- Kernel:            #{@kernel}")
           env[:ui].info(" -- Initrd:            #{@initrd}")
           env[:ui].info(" -- Graphics Type:     #{@graphics_type}")

lib/vagrant-libvirt/config.rb

@@ -95,9 +95,10 @@ module VagrantPlugins
       attr_accessor :machine_virtual_size
       attr_accessor :disk_bus
       attr_accessor :disk_device
+      attr_accessor :disk_driver_opts
       attr_accessor :nic_model_type
       attr_accessor :nested
-      attr_accessor :volume_cache
+      attr_accessor :volume_cache # deprecated, kept for backwards compatibility; use disk_driver
       attr_accessor :kernel
       attr_accessor :cmd_line
       attr_accessor :initrd
@@ -234,6 +235,7 @@ module VagrantPlugins
         @machine_virtual_size = UNSET_VALUE
         @disk_bus          = UNSET_VALUE
         @disk_device       = UNSET_VALUE
+        @disk_driver_opts  = {}
         @nic_model_type    = UNSET_VALUE
         @nested            = UNSET_VALUE
         @volume_cache      = UNSET_VALUE
@@ -587,6 +589,12 @@ module VagrantPlugins
         @smartcard_dev[:source_service] = options[:source_service] if @smartcard_dev[:type] == 'tcp'
       end
 
+      # Disk driver options for primary disk
+      def disk_driver(options = {})
+        supported_opts = [:cache, :io, :copy_on_read, :discard, :detect_zeroes]
+        @disk_driver_opts = options.select { |k,_| supported_opts.include? k }
+      end
+
       # NOTE: this will run twice for each time it's needed- keep it idempotent
       def storage(storage_type, options = {})
         if storage_type == :file
@@ -641,6 +649,10 @@ module VagrantPlugins
           allow_existing: options[:allow_existing],
           shareable: options[:shareable],
           serial: options[:serial],
+          io: options[:io],
+          copy_on_read: options[:copy_on_read],
+          discard: options[:discard],
+          detect_zeroes: options[:detect_zeroes],
           pool: options[:pool], # overrides storage_pool setting for additional disks
           wwn: options[:wwn],
         }
@@ -795,9 +807,10 @@ module VagrantPlugins
         @machine_virtual_size = nil if @machine_virtual_size == UNSET_VALUE
         @disk_bus = 'virtio' if @disk_bus == UNSET_VALUE
         @disk_device = 'vda' if @disk_device == UNSET_VALUE
+        @disk_driver_opts = {} if @disk_driver_opts == UNSET_VALUE
         @nic_model_type = nil if @nic_model_type == UNSET_VALUE
         @nested = false if @nested == UNSET_VALUE
-        @volume_cache = 'default' if @volume_cache == UNSET_VALUE
+        @volume_cache = nil if @volume_cache == UNSET_VALUE
         @kernel = nil if @kernel == UNSET_VALUE
         @cmd_line = '' if @cmd_line == UNSET_VALUE
         @initrd = '' if @initrd == UNSET_VALUE
@@ -915,6 +928,14 @@ module VagrantPlugins
           end
         end
 
+        if !machine.provider_config.volume_cache.nil? and machine.provider_config.volume_cache != UNSET_VALUE
+          machine.ui.warn("Libvirt Provider: volume_cache is deprecated. Use disk_driver :cache => '#{machine.provider_config.volume_cache}' instead.")
+
+          if !machine.provider_config.disk_driver_opts.empty?
+            machine.ui.warn("Libvirt Provider: volume_cache has no effect when disk_driver is defined.")
+          end
+        end
+
         { 'Libvirt Provider' => errors }
       end
 
@@ -928,6 +949,8 @@ module VagrantPlugins
           c += other.cdroms
           result.cdroms = c
 
+          result.disk_driver_opts = disk_driver_opts.merge(other.disk_driver_opts)
+          
           c = clock_timers.dup
           c += other.clock_timers
           result.clock_timers = c

lib/vagrant-libvirt/templates/domain.xml.erb

@@ -115,7 +115,11 @@
     <% end %>
     <% if @domain_volume_path %>
     <disk type='file' device='disk'>
-      <driver name='qemu' type='qcow2' cache='<%= @domain_volume_cache %>'/>
+      <driver name='qemu' type='qcow2' <%=
+        @disk_driver_opts.empty? ? "cache='#{@domain_volume_cache}'" :
+            @disk_driver_opts.reject { |k,v| v.nil? }
+                             .map { |k,v| "#{k}='#{v}'"}
+                             .join(' ') -%>/>
       <source file='<%= @domain_volume_path %>'/>
       <%# we need to ensure a unique target dev -%>
       <target dev='<%= @disk_device %>' bus='<%= @disk_bus %>'/>
@@ -124,7 +128,12 @@
 <%# additional disks -%>
 <% @disks.each do |d| -%>
     <disk type='file' device='disk'>
-      <driver name='qemu' type='<%= d[:type] %>' cache='<%= d[:cache] %>'/>
+      <driver name='qemu' type='<%= d[:type] %>' <%=
+        d.select { |k,_| [:cache, :io, :copy_on_read, :discard, :detect_zeroes].include? k }
+         .reject { |k,v| v.nil? }
+         .map { |k,v| "#{k}='#{v}'"}
+         .join(' ')
+      -%>/>
       <source file='<%= d[:absolute_path] %>'/>
       <target dev='<%= d[:device] %>' bus='<%= d[:bus] %>'/>
       <% if d[:shareable] %>

spec/unit/templates/domain_all_settings.xml

@@ -43,7 +43,7 @@
   <devices>
     <emulator>/usr/bin/kvm-spice</emulator>
     <disk type='file' device='disk'>
-      <driver name='qemu' type='qcow2' cache='unsafe'/>
+      <driver name='qemu' type='qcow2' cache='unsafe' io='threads' copy_on_read='on' discard='unmap' detect_zeroes='on'/>
       <source file='/var/lib/libvirt/images/test.qcow2'/>
       <target dev='vda' bus='ide'/>
     </disk>
@@ -53,7 +53,7 @@
       <target dev='vdb' bus='virtio'/>
     </disk>
     <disk type='file' device='disk'>
-      <driver name='qemu' type='qcow2' cache='default'/>
+      <driver name='qemu' type='qcow2' cache='default' io='threads' copy_on_read='on' discard='unmap' detect_zeroes='on'/>
       <source file='/var/lib/libvirt/images/test-disk2.qcow2'/>
       <target dev='vdc' bus='virtio'/>
     </disk>

spec/unit/templates/domain_spec.rb

@@ -45,11 +45,12 @@ describe 'templates/domain' do
       domain.boot('hd')
       domain.emulator_path = '/usr/bin/kvm-spice'
       domain.instance_variable_set('@domain_volume_path', '/var/lib/libvirt/images/test.qcow2')
-      domain.instance_variable_set('@domain_volume_cache', 'unsafe')
+      domain.instance_variable_set('@domain_volume_cache', 'deprecated')
       domain.disk_bus = 'ide'
       domain.disk_device = 'vda'
+      domain.disk_driver(:cache => 'unsafe', :io => 'threads', :copy_on_read => 'on', :discard => 'unmap', :detect_zeroes => 'on')
       domain.storage(:file, path: 'test-disk1.qcow2')
-      domain.storage(:file, path: 'test-disk2.qcow2')
+      domain.storage(:file, path: 'test-disk2.qcow2', io: 'threads', copy_on_read: 'on', discard: 'unmap', detect_zeroes: 'on')
       domain.disks.each do |disk|
         disk[:absolute_path] = '/var/lib/libvirt/images/' + disk[:path]
       end