Skip to content

Commit

Permalink
define domain before creating resources it depends on
Browse files Browse the repository at this point in the history 
And undefine it after those resources have been removed.

This has the advantage that the VM is defined whenever the other
resources exist. So we can find those other resources in order to remove
them.

Previously a failure while removing a VM could leave orphaned disks.
These caused problems when trying to create the same VM again.
  • Loading branch information
@JoelColledge
JoelColledge committed Nov 8, 2021
1 parent b5509ff commit b41c687
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 84 deletions.
2 changes: 1 addition & 1 deletion cmd/vm_rm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion internal/virter/image.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
132 changes: 68 additions & 64 deletions internal/virter/vm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
32 changes: 14 additions & 18 deletions internal/virter/vm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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),
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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{
Expand All @@ -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)

Expand Down

0 comments on commit b41c687

Please sign in to comment.