diff --git a/cmd/vm_rm.go b/cmd/vm_rm.go index 1fad40b..d86e8b3 100644 --- a/cmd/vm_rm.go +++ b/cmd/vm_rm.go @@ -16,7 +16,7 @@ func rmMultiple(v *virter.Virter, vms []string) error { staticDHCP := viper.GetBool("libvirt.static_dhcp") var errs error for _, vm := range vms { - err := v.VMRm(vm, staticDHCP) + err := v.VMRm(vm, !staticDHCP, true) if err != nil { e := fmt.Errorf("failed to remove VM '%s': %w", vm, err) errs = multierror.Append(errs, e) diff --git a/internal/virter/image.go b/internal/virter/image.go index 01a4234..63e39b2 100644 --- a/internal/virter/image.go +++ b/internal/virter/image.go @@ -601,7 +601,7 @@ func (v *Virter) ImageBuild(ctx context.Context, tools ImageBuildTools, vmConfig err = v.imageBuildProvisionCommit(ctx, tools, vmConfig, pingConfig, buildConfig, opts...) if err != nil { log.Warn("could not build image, deleting VM") - if rmErr := v.VMRm(vmConfig.Name, vmConfig.StaticDHCP); rmErr != nil { + if rmErr := v.VMRm(vmConfig.Name, !vmConfig.StaticDHCP, true); rmErr != nil { return fmt.Errorf("could not delete VM: %v, after build failed: %w", rmErr, err) } return err diff --git a/internal/virter/vm.go b/internal/virter/vm.go index 0a41f0a..51d37c0 100644 --- a/internal/virter/vm.go +++ b/internal/virter/vm.go @@ -109,6 +109,23 @@ func (v *Virter) VMRun(vmConfig VMConfig) error { return fmt.Errorf("could not create new host key: %w", err) } + meta := &VMMeta{ + HostKey: hostkey.PublicKey(), + } + + vmXML, err := v.vmXML(v.provisionStoragePool.Name, vmConfig, mac, meta) + if err != nil { + return err + } + + log.Debugf("Using domain XML: %s", vmXML) + + log.Print("Define VM") + d, err := v.libvirt.DomainDefineXML(vmXML) + if err != nil { + return fmt.Errorf("could not define domain: %w", err) + } + log.Print("Create boot volume") _, err = v.ImageSpawn(vmConfig.Name, vmConfig.Image, vmConfig.BootCapacityKiB) if err != nil { @@ -129,23 +146,6 @@ func (v *Virter) VMRun(vmConfig VMConfig) error { } } - meta := &VMMeta{ - HostKey: hostkey.PublicKey(), - } - - vmXML, err := v.vmXML(v.provisionStoragePool.Name, vmConfig, mac, meta) - if err != nil { - return err - } - - log.Debugf("Using domain XML: %s", vmXML) - - log.Print("Define VM") - d, err := v.libvirt.DomainDefineXML(vmXML) - if err != nil { - return fmt.Errorf("could not define domain: %w", err) - } - if !vmConfig.StaticDHCP { // Add DHCP entry after defining the VM to ensure that it can be // removed when removing the VM, but before starting it to ensure that @@ -224,26 +224,7 @@ func tryDialSSH(ctx context.Context, shellClientBuilder ShellClientBuilder, host } // VMRm removes a VM. -func (v *Virter) VMRm(vmName string, staticDHCP bool) error { - err := v.vmRmExceptBoot(vmName, !staticDHCP) - if err != nil { - return err - } - - layer, err := v.FindDynamicLayer(vmName) - if err != nil { - return fmt.Errorf("could not get volume %s: %w", vmName, err) - } - - err = layer.DeleteAllIfUnused() - if err != nil { - return fmt.Errorf("could not delete volume %s: %w", vmName, err) - } - - return nil -} - -func (v *Virter) vmRmExceptBoot(vmName string, removeDHCPEntries bool) error { +func (v *Virter) VMRm(vmName string, removeDHCPEntries bool, removeBoot bool) error { domain, err := v.libvirt.DomainLookupByName(vmName) if err != nil { if hasErrorCode(err, libvirt.ErrNoDomain) { @@ -253,62 +234,85 @@ func (v *Virter) vmRmExceptBoot(vmName string, removeDHCPEntries bool) error { return fmt.Errorf("could not get domain: %w", err) } - disks, err := v.getDisksOfDomain(domain) + active, err := v.libvirt.DomainIsActive(domain) if err != nil { - return err + return fmt.Errorf("could not check if domain is active: %w", err) } - err = v.rmSnapshots(domain) + persistent, err := v.libvirt.DomainIsPersistent(domain) if err != nil { - return err + return fmt.Errorf("could not check if domain is persistent: %w", err) } - active, err := v.libvirt.DomainIsActive(domain) + // Stop the VM before removing the resources it depends on. But only if + // it is active (running). And only if it is persistent, otherwise the + // domain is gone and we cannot query what resources it depended on. + if active > 0 && persistent > 0 { + log.Print("Stop VM") + err = v.libvirt.DomainDestroy(domain) + if err != nil { + return fmt.Errorf("could not destroy domain: %w", err) + } + } + + err = v.removeDomainDHCP(domain, removeDHCPEntries) if err != nil { - return fmt.Errorf("could not check if domain is active: %w", err) + return err } - persistent, err := v.libvirt.DomainIsPersistent(domain) + err = v.rmSnapshots(domain) if err != nil { - return fmt.Errorf("could not check if domain is persistent: %w", err) + return err } - err = v.removeDomainDHCP(domain, removeDHCPEntries) + disks, err := v.getDisksOfDomain(domain) if err != nil { return err } - if active != 0 { - log.Print("Stop VM") - err = v.libvirt.DomainDestroy(domain) + for _, disk := range disks { + if !removeBoot && disk == DynamicLayerName(vmName) { + // do not delete boot volume + continue + } + + err = v.rmVolume(disk) if err != nil { - return fmt.Errorf("could not destroy domain: %w", err) + return err } } - if persistent != 0 { + if persistent > 0 { log.Print("Undefine VM") err = v.libvirt.DomainUndefineFlags(domain, libvirt.DomainUndefineNvram) if err != nil { return fmt.Errorf("could not undefine domain: %w", err) } + } else if active > 0 { + // Stop the VM if we did not stop it previously. + log.Print("Stop VM") + err = v.libvirt.DomainDestroy(domain) + if err != nil { + return fmt.Errorf("could not destroy domain: %w", err) + } } - for _, disk := range disks { - if disk == DynamicLayerName(vmName) { - // do not delete boot volume - continue - } + return nil +} - layer, err := v.FindRawLayer(disk) - if err != nil { - return fmt.Errorf("could not get volume %s: %w", disk, err) +func (v *Virter) rmVolume(rawVolumeName string) error { + layer, err := v.FindRawLayer(rawVolumeName) + if err != nil { + if hasErrorCode(err, libvirt.ErrNoStorageVol) { + return nil } - err = layer.DeleteAllIfUnused() - if err != nil { - return fmt.Errorf("could not delete volume %s: %w", disk, err) - } + return fmt.Errorf("could not get volume %s: %w", rawVolumeName, err) + } + + err = layer.DeleteAllIfUnused() + if err != nil { + return fmt.Errorf("could not delete volume %s: %w", rawVolumeName, err) } return nil @@ -356,7 +360,7 @@ func (v *Virter) VMCommit(ctx context.Context, afterNotifier AfterNotifier, vmNa } } - err = v.vmRmExceptBoot(vmName, !staticDHCP) + err = v.VMRm(vmName, !staticDHCP, false) if err != nil { return err } diff --git a/internal/virter/vm_test.go b/internal/virter/vm_test.go index 525379a..020f827 100644 --- a/internal/virter/vm_test.go +++ b/internal/virter/vm_test.go @@ -120,16 +120,6 @@ var vmRmTests = []map[string]bool{ bootVolume: true, domainCreated: true, }, - { - ciDataVolume: true, - bootVolume: true, - domainCreated: true, - }, - { - ciDataVolume: true, - bootVolume: true, - domainCreated: true, - }, { ciDataVolume: true, bootVolume: true, @@ -150,9 +140,9 @@ var vmRmTests = []map[string]bool{ }, } -func addDisk(l *FakeLibvirtConnection, vmName, volumeName string) { - disks := l.domains[vmName].description.Devices.Disks - l.domains[vmName].description.Devices.Disks = append(disks, libvirtxml.DomainDisk{ +func addDisk(domain *FakeLibvirtDomain, vmName, volumeName string) { + disks := domain.description.Devices.Disks + domain.description.Devices.Disks = append(disks, libvirtxml.DomainDisk{ Source: &libvirtxml.DomainDiskSource{ Volume: &libvirtxml.DomainDiskSourceVolume{ Volume: virter.DynamicLayerName(volumeName), @@ -171,6 +161,13 @@ func TestVMRm(t *testing.T) { domain := newFakeLibvirtDomain(vmMAC) domain.persistent = r[domainPersistent] domain.active = r[domainCreated] + + // Always add the disks to the description. The + // test arguments specify whether the volumes + // themselves should exist. + addDisk(domain, vmName, vmName) + addDisk(domain, vmName, ciDataVolumeName) + l.domains[vmName] = domain fakeNetworkAddHost(l.networks[networkName], vmMAC, vmIP) @@ -181,13 +178,12 @@ func TestVMRm(t *testing.T) { } if r[ciDataVolume] { - l.addEmptyRawVol(virter.DynamicLayerName(vmName)) - addDisk(l, vmName, ciDataVolumeName) + l.addEmptyRawVol(virter.DynamicLayerName(ciDataVolumeName)) } v := virter.New(l, poolName, networkName, newMockKeystore()) - err := v.VMRm(vmName, r[staticDHCP]) + err := v.VMRm(vmName, !r[staticDHCP], true) assert.NoError(t, err) assert.Empty(t, l.vols) @@ -241,6 +237,7 @@ func TestVMCommit(t *testing.T) { domain := newFakeLibvirtDomain(vmMAC) domain.persistent = true domain.active = r[commitDomainActive] + addDisk(domain, vmName, ciDataVolumeName) l.domains[vmName] = domain l.vols[virter.DynamicLayerName(vmName)] = &FakeLibvirtStorageVol{ @@ -249,8 +246,7 @@ func TestVMCommit(t *testing.T) { BackingStore: img.description.BackingStore, }, } - l.vols[virter.DynamicLayerName(ciDataVolumeName)] = &FakeLibvirtStorageVol{description: &libvirtxml.StorageVolume{Name: virter.DynamicLayerName(vmName)}} - addDisk(l, vmName, ciDataVolumeName) + l.addEmptyRawVol(virter.DynamicLayerName(ciDataVolumeName)) fakeNetworkAddHost(l.networks[networkName], vmMAC, vmIP)