Disks with allow_existing: true are cleaned up when domain is destroyed

[anonym]

Steps to reproduce

1. Add a disk with allow_existing: true set, e.g.:

Vagrant.configure("2") do |config|
  config.vm.provider :libvirt do |domain|
    domain.storage(:file, size: '15G', allow_existing: true, path: 'foo.qcow2')
  end
end

2. vagrant up
3. vagrant destroy --force

Expected behaviour

The disk with allow_existing: true should still exist.

Actual behaviour

The disk with allow_existing: true was removed by vagrant destroy --force.

System configuration

Debian Unstable

Libvirt version:
3.0.0-3

Output of vagrant version; vagrant plugin list:
Installed Version: 1.9.1
vagrant-libvirt (0.0.37, system)

Output of VAGRANT_LOG=debug vagrant ... --provider=libvirt
Irrelevant; nothing about disks is logged for vagrant destroy.

A Vagrantfile to reproduce the issue:

Vagrant.configure("2") do |config|
  config.vm.box = 'tails-builder-amd64-jessie-20170105'
  config.vm.box_url = "http://dl.amnesia.boum.org/tails/project/vagrant/tails-builder-amd64-jessie-20170105.box"
  config.vm.box_download_checksum_type = "sha256"
  config.vm.box_download_checksum = '4600eaffa300123c6a1cff8a1b69f1366a62c2f502ef059c9196d7ae5e5fff8b'
  config.vm.provision "shell", inline: <<-EOF
    true
  EOF
  config.vm.provider :libvirt do |domain|
    domain.default_prefix = config.vm.box
    domain.driver = 'kvm'
    domain.management_network_guest_ipv6 = 'no'
    domain.connect_via_ssh = false
    domain.machine_arch = 'x86_64'
    domain.machine_type = 'q35'
    domain.cpu_mode = 'host-passthrough'
    domain.emulator_path = '/usr/bin/qemu-system-x86_64'
    domain.memory = 512
    domain.storage(
      :file, size: '15G', allow_existing: true, cache: 'none',
      path: 'apt-cacher-ng-data.qcow2'
    )
  end
end

The issue with the above Vagrantfile is that apt-cacher-ng-data.qcow2 is cleaned up on vagrant destroy.

Are you using upstream vagrant package or your distros package?
Distro (Debian)

I looked briefly at the code for cleaning up disks in lib/vagrant-libvirt/action/destroy_domain.rb, and the problem is that we end up running the case with domain.destroy(destroy_volumes: true); when vagrant destroy is run, the disks are not added to env[:machine].provider_config.disks (it's empty), so we never got to the case where we check disk-by-disk if allow_existing is set. In fact, when running vagrant destroy my domain.storage call from the above Vagrantfile is not run (actually the block passed to config.vm.provider is not run), which explains why it's not getting added to env[:machine].provider_config.disks.

[electrofelix]

Testing with a slight smaller config and a different box, this is currently working on vagrant 1.8.7 with vagrant-libvirt 0.0.37.

Wonder if you could config that you're experiencing the same problem with the following Vagrantfile

Vagrant.configure("2") do |config|
  config.vm.box = "debian/jessie64"
  config.vm.provision "shell", inline: <<-EOF
    true
  EOF
  config.vm.provider :libvirt do |domain|
    domain.driver = 'kvm'
    domain.management_network_guest_ipv6 = 'no'
    domain.connect_via_ssh = false
    domain.machine_arch = 'x86_64'
    domain.cpu_mode = 'host-passthrough'
    domain.memory = 512
    domain.storage(
      :file, size: '15G', allow_existing: true, cache: 'none',
      path: 'apt-cacher-ng-data.qcow2'
    )
  end
end

I can't upgrade my local vagrant installation right now, so I'll have to come back to this, but if you can confirm you still see the bug with the pruned config above it'll help me narrow it down as I look at what changed between vagrant 1.8.7 and 1.9.1 to result in vagrant-libvirt not seeing the config correctly.

[anonym]

Interestingly I cannot reproduce this any more, not with your Vagrantfile, or mine. In the end this issue didn't turn out to block me from what I was trying to do when I experienced the problem, and now it's very awkward to revert back to the exact state I had then. It seems very likely that the error was on my side, even though I took great precautions trying to prevent that, so perhaps you should just reject this ticket.

Sorry for the inconvenience!

[infernix]

Thanks for checking; closing until reproducable.