drop volumes full paths

cloud/libvirt/actuators/machine/actuator.go

@@ -115,7 +115,7 @@ func createVolumeAndDomain(machine *clusterv1.Machine, offset int) error {
 	}
 
 	// Create domain
-	if err = libvirtutils.CreateDomain(name, ignKey, name, name, networkInterfaceName, networkInterfaceAddress, autostart, memory, vcpu, offset, client); err != nil {
+	if err = libvirtutils.CreateDomain(name, ignKey, pool, name, name, networkInterfaceName, networkInterfaceAddress, autostart, memory, vcpu, offset, client); err != nil {
 		return fmt.Errorf("error creating domain: %v", err)
 	}
 	return nil
@@ -125,7 +125,6 @@ func createVolumeAndDomain(machine *clusterv1.Machine, offset int) error {
 func deleteVolumeAndDomain(machine *clusterv1.Machine) error {
 	// decode config
 	machineProviderConfig, err := machineProviderConfigFromClusterAPIMachineSpec(&machine.Spec)
-	name := machine.Name
 	if err != nil {
 		return fmt.Errorf("error getting machineProviderConfig from spec: %v", err)
 	}
@@ -137,12 +136,12 @@ func deleteVolumeAndDomain(machine *clusterv1.Machine) error {
 	}
 
 	// delete domain
-	if err := libvirtutils.DeleteDomain(name, client); err != nil {
+	if err := libvirtutils.DeleteDomain(machine.Name, client); err != nil {
 		return fmt.Errorf("error deleting domain: %v", err)
 	}
 
 	// delete volume
-	if err := libvirtutils.DeleteVolume(name, client); err != nil {
+	if err := libvirtutils.DeleteVolume(machine.Name, machineProviderConfig.Volume.PoolName, client); err != nil {
 		return fmt.Errorf("error deleting volume: %v", err)
 	}
 	return nil

cloud/libvirt/actuators/machine/utils/domain.go

@@ -18,10 +18,6 @@ import (
 	"github.com/openshift/cluster-api-provider-libvirt/lib/cidr"
 )
 
-const (
-	baseVolumePath = "/var/lib/libvirt/images/"
-)
-
 // LibVirtConIsNil contains a nil connection error message
 var LibVirtConIsNil = "the libvirt connection was nil"
 
@@ -266,14 +262,8 @@ func randomWWN(strlen int) string {
 	return oui + string(result)
 }
 
-func setDisks(domainDef *libvirtxml.Domain, virConn *libvirt.Connect, volumeKey string) error {
+func setDisks(domainDef *libvirtxml.Domain, diskVolume libvirt.StorageVol) error {
 	disk := newDefDisk(0)
-	log.Printf("[INFO] LookupStorageVolByKey")
-	diskVolume, err := virConn.LookupStorageVolByKey(volumeKey)
-	if err != nil {
-		return fmt.Errorf("Can't retrieve volume %s", volumeKey)
-	}
-	log.Printf("[INFO] diskVolume")
 	diskVolumeFile, err := diskVolume.GetPath()
 	if err != nil {
 		return fmt.Errorf("Error retrieving volume file: %s", err)
@@ -447,7 +437,7 @@ func domainDefInit(domainDef *libvirtxml.Domain, name string, memory, vcpu int)
 	return nil
 }
 
-func CreateDomain(name, ignKey, volumeName, hostName, networkInterfaceName, networkInterfaceAddress string, autostart bool, memory, vcpu, offset int, client *Client) error {
+func CreateDomain(name, ignKey, poolName string, volumeName, hostName, networkInterfaceName, networkInterfaceAddress string, autostart bool, memory, vcpu, offset int, client *Client) error {
 	if name == "" {
 		return fmt.Errorf("Failed to create domain, name is empty")
 	}
@@ -467,19 +457,28 @@ func CreateDomain(name, ignKey, volumeName, hostName, networkInterfaceName, netw
 
 	log.Printf("[INFO] setCoreOSIgnition")
 	if ignKey != "" {
-		if err := setCoreOSIgnition(&domainDef, ignKey); err != nil {
+		ignVolume, err := getVolumeFromPool(ignKey, poolName, virConn)
+		if err != nil {
+			return fmt.Errorf("error getting ignition volume: %v", err)
+		}
+		ignVolumePath, err := ignVolume.GetPath()
+		if err != nil {
+			return fmt.Errorf("error getting ignition volume path: %v", err)
+		}
+		if err := setCoreOSIgnition(&domainDef, ignVolumePath); err != nil {
 			return err
 		}
 	} else {
 		return fmt.Errorf("machine does not has a IgnKey value")
 	}
 
 	log.Printf("[INFO] setDisks")
-	VolumeKey := baseVolumePath + volumeName
-	if volumeName == "" {
-		volumeName = name
+	diskVolume, err := getVolumeFromPool(volumeName, poolName, virConn)
+	if err != nil {
+		return fmt.Errorf("can't retrieve volume %s for pool %s: %v", volumeName, poolName, err)
 	}
-	if err := setDisks(&domainDef, virConn, VolumeKey); err != nil {
+	log.Printf("[INFO] diskVolume")
+	if err := setDisks(&domainDef, *diskVolume); err != nil {
 		return fmt.Errorf("Failed to setDisks: %s", err)
 	}
 

cloud/libvirt/actuators/machine/utils/volume.go

@@ -100,31 +100,18 @@ func newDefVolumeFromXML(s string) (libvirtxml.StorageVolume, error) {
 	return volumeDef, nil
 }
 
-func CreateVolume(volumeName, poolName, baseVolumeID, source, volumeFormat string, client *Client) error {
+func CreateVolume(volumeName, poolName, baseVolumeName, source, volumeFormat string, client *Client) error {
 	var volume *libvirt.StorageVol
 
-	log.Printf("[DEBUG] Create a libvirt volume with name %s for pool %s from the base volume %s", volumeName, poolName, baseVolumeID)
+	log.Printf("[DEBUG] Create a libvirt volume with name %s for pool %s from the base volume %s", volumeName, poolName, baseVolumeName)
 	virConn := client.libvirt
 
 	// TODO: lock pool
 	//client.poolMutexKV.Lock(poolName)
 	//defer client.poolMutexKV.Unlock(poolName)
 
-	pool, err := virConn.LookupStoragePoolByName(poolName)
-	if err != nil {
-		return fmt.Errorf("can't find storage pool '%s'", poolName)
-	}
-	defer pool.Free()
-
-	// Refresh the pool of the volume so that libvirt knows it is
-	// not longer in use.
-	waitForSuccess("error refreshing pool for volume", func() error {
-		return pool.Refresh(0)
-	})
-
-	// Check whether the storage volume already exists. Its name needs to be
-	// unique.
-	if _, err := pool.LookupStorageVolByName(volumeName); err == nil {
+	volume, err := getVolumeFromPool(volumeName, poolName, virConn)
+	if err == nil {
 		return fmt.Errorf("storage volume '%s' already exists", volumeName)
 	}
 
@@ -134,7 +121,7 @@ func CreateVolume(volumeName, poolName, baseVolumeID, source, volumeFormat strin
 	var img image
 	// an source image was given, this mean we can't choose size
 	if source != "" {
-		if baseVolumeID != "" {
+		if baseVolumeName != "" {
 			return fmt.Errorf("'base_volume_id' can't be specified when also 'source' is given")
 		}
 
@@ -150,16 +137,16 @@ func CreateVolume(volumeName, poolName, baseVolumeID, source, volumeFormat strin
 		log.Printf("Image %s image is: %d bytes", img, size)
 		volumeDef.Capacity.Unit = "B"
 		volumeDef.Capacity.Value = size
-	} else if baseVolumeID != "" {
+	} else if baseVolumeName != "" {
 		volume = nil
 		volumeDef.Capacity.Value = uint64(size)
-		baseVolume, err := client.libvirt.LookupStorageVolByKey(baseVolumeID)
+		baseVolume, err := getVolumeFromPool(baseVolumeName, poolName, virConn)
 		if err != nil {
-			return fmt.Errorf("Can't retrieve volume %s", baseVolumeID)
+			return fmt.Errorf("Can't retrieve volume %s", baseVolumeName)
 		}
 		backingStoreDef, err := newDefBackingStoreFromLibvirt(baseVolume)
 		if err != nil {
-			return fmt.Errorf("Could not retrieve backing store %s", baseVolumeID)
+			return fmt.Errorf("Could not retrieve backing store %s", baseVolumeName)
 		}
 		volumeDef.BackingStore = &backingStoreDef
 	}
@@ -171,6 +158,17 @@ func CreateVolume(volumeName, poolName, baseVolumeID, source, volumeFormat strin
 		}
 
 		// create the volume
+		pool, err := virConn.LookupStoragePoolByName(poolName)
+		defer pool.Free()
+
+		// Refresh the pool of the volume so that libvirt knows it is
+		// not longer in use.
+		waitForSuccess("error refreshing pool for volume", func() error {
+			return pool.Refresh(0)
+		})
+		if err != nil {
+			return fmt.Errorf("can't find storage pool '%s'", poolName)
+		}
 		v, err := pool.StorageVolCreateXML(string(volumeDefXML), 0)
 		if err != nil {
 			return fmt.Errorf("Error creating libvirt volume: %s", err)
@@ -196,46 +194,55 @@ func CreateVolume(volumeName, poolName, baseVolumeID, source, volumeFormat strin
 	return nil
 }
 
-// DeleteVolume removes the volume identified by `key` from libvirt
-func DeleteVolume(name string, client *Client) error {
-	volumePath := fmt.Sprintf(baseVolumePath+"%s", name)
-	volume, err := client.libvirt.LookupStorageVolByPath(volumePath)
+func getVolumeFromPool(volumeName, poolName string, virConn *libvirt.Connect) (*libvirt.StorageVol, error) {
+	pool, err := virConn.LookupStoragePoolByName(poolName)
 	if err != nil {
-		return fmt.Errorf("Can't retrieve volume %s", volumePath)
+		return nil, fmt.Errorf("can't find storage pool %q: %v", poolName, err)
 	}
-	defer volume.Free()
+	defer pool.Free()
 
 	// Refresh the pool of the volume so that libvirt knows it is
 	// not longer in use.
-	volPool, err := volume.LookupPoolByVolume()
+	waitForSuccess("error refreshing pool for volume", func() error {
+		return pool.Refresh(0)
+	})
+
+	// Check whether the storage volume exists. Its name needs to be
+	// unique.
+	volume, err := pool.LookupStorageVolByName(volumeName)
 	if err != nil {
-		return fmt.Errorf("Error retrieving pool for volume: %s", err)
+		return nil, fmt.Errorf("can't retrieve volume %q: %v", volumeName, err)
 	}
-	defer volPool.Free()
+	//defer volume.Free()
+	return volume, nil
+}
 
+// DeleteVolume removes the volume identified by `key` from libvirt
+func DeleteVolume(name string, poolName string, client *Client) error {
+	virConn := client.libvirt
+	volume, err := getVolumeFromPool(name, poolName, virConn)
+	if err != nil {
+		return fmt.Errorf("failed getting volume from pool: %v", err)
+	}
 	// TODO: add locking support
-	//poolName, err := volPool.GetName()
+	//poolName, err := pool.GetName()
 	//if err != nil {
 	//	return fmt.Errorf("Error retrieving name of volume: %s", err)
 	//}
 	//client.poolMutexKV.Lock(poolName)
 	//defer client.poolMutexKV.Unlock(poolName)
 
-	waitForSuccess("Error refreshing pool for volume", func() error {
-		return volPool.Refresh(0)
-	})
-
 	// Workaround for redhat#1293804
 	// https://bugzilla.redhat.com/show_bug.cgi?id=1293804#c12
 	// Does not solve the problem but it makes it happen less often.
 	_, err = volume.GetXMLDesc(0)
 	if err != nil {
-		return fmt.Errorf("Can't retrieve volume %s XML desc: %s", volumePath, err)
+		return fmt.Errorf("Can't retrieve volume %s XML desc: %s", name, err)
 	}
 
 	err = volume.Delete(0)
 	if err != nil {
-		return fmt.Errorf("Can't delete volume %s: %s", volumePath, err)
+		return fmt.Errorf("Can't delete volume %s: %s", name, err)
 	}
 
 	return nil

examples/machine-with-full-paths.yaml → examples/machine-with-vol-names.yaml

@@ -15,10 +15,10 @@ spec:
       kind: LibvirtMachineProviderConfig
       domainMemory: 4086
       domainVcpu: 2
-      ignKey: /var/lib/libvirt/images/worker.ign
+      ignKey: worker.ign
       volume:
         poolName: default
-        baseVolumeID: /var/lib/libvirt/images/baseVolume
+        baseVolumeID: baseVolume
       networkInterfaceName: actuatorTestNetwork
       networkInterfaceAddress: 192.168.124.0/24
       autostart: false

tests/actuator/test.go

@@ -36,7 +36,7 @@ var rootCmd = &cobra.Command{
 			expectedReachableIP string
 		}{
 			{
-				machineFile:         "machine-with-full-paths.yaml",
+				machineFile:         "machine-with-vol-names.yaml",
 				expectedReachableIP: "192.168.124.51", // currently the actuator starts with an offset of 50
 			},
 		}
@@ -46,11 +46,11 @@ var rootCmd = &cobra.Command{
 			return fmt.Errorf("Failed to build libvirt client: %s", err)
 		}
 
+		defer destroyActuatorInfraAssumtions(client)
 		if err := createActuatorInfraAssumtions(client); err != nil {
-			log.Errorf("failed creating actuator infra assumtions %v", err)
-			//return err
+			log.Errorf("failed creating actuator infra assumptions %v", err)
+			return err
 		}
-		defer destroyActuatorInfraAssumtions(client)
 
 		for _, test := range testCases {
 			// create machine to manually test
@@ -148,18 +148,18 @@ func createActuatorInfraAssumtions(client *libvirtutils.Client) error {
 
 func destroyActuatorInfraAssumtions(client *libvirtutils.Client) {
 	// Delete base volume
-	if err := libvirtutils.DeleteVolume("baseVolume", client); err != nil {
+	if err := libvirtutils.DeleteVolume("baseVolume", "default", client); err != nil {
 		log.Errorf("failed deleting base volume %v", err)
 	}
 
 	// Delete ign volume
-	if err := libvirtutils.DeleteVolume("worker.ign", client); err != nil {
-		log.Errorf("failed creating ignition %v", err)
+	if err := libvirtutils.DeleteVolume("worker.ign", "default", client); err != nil {
+		log.Errorf("failed deleting ignition %v", err)
 	}
 
 	// Delete networkInterfaceName
 	if err := libvirtutils.DeleteNetwork("actuatorTestNetwork", client); err != nil {
-		log.Errorf("failed creating network %v", err)
+		log.Errorf("failed deleting network %v", err)
 	}
 }