From 00b16f152ee24782eb6cdf36d8ca988258d46e49 Mon Sep 17 00:00:00 2001 From: Enxebre Date: Mon, 8 Oct 2018 15:06:05 +0200 Subject: [PATCH] drop volumes full paths --- cloud/libvirt/actuators/machine/actuator.go | 7 +- .../libvirt/actuators/machine/utils/domain.go | 33 ++++---- .../libvirt/actuators/machine/utils/volume.go | 83 ++++++++++--------- ...paths.yaml => machine-with-vol-names.yaml} | 4 +- tests/actuator/test.go | 16 ++-- 5 files changed, 74 insertions(+), 69 deletions(-) rename examples/{machine-with-full-paths.yaml => machine-with-vol-names.yaml} (86%) diff --git a/cloud/libvirt/actuators/machine/actuator.go b/cloud/libvirt/actuators/machine/actuator.go index 5491a0f2d..c64d4e08d 100644 --- a/cloud/libvirt/actuators/machine/actuator.go +++ b/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 diff --git a/cloud/libvirt/actuators/machine/utils/domain.go b/cloud/libvirt/actuators/machine/utils/domain.go index d4ae91005..ff9ff8e4d 100644 --- a/cloud/libvirt/actuators/machine/utils/domain.go +++ b/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,7 +457,15 @@ 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 { @@ -475,11 +473,12 @@ func CreateDomain(name, ignKey, volumeName, hostName, networkInterfaceName, netw } 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) } diff --git a/cloud/libvirt/actuators/machine/utils/volume.go b/cloud/libvirt/actuators/machine/utils/volume.go index 5f519524c..5efa9f44d 100644 --- a/cloud/libvirt/actuators/machine/utils/volume.go +++ b/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 diff --git a/examples/machine-with-full-paths.yaml b/examples/machine-with-vol-names.yaml similarity index 86% rename from examples/machine-with-full-paths.yaml rename to examples/machine-with-vol-names.yaml index 41b48471d..a7fa60fee 100644 --- a/examples/machine-with-full-paths.yaml +++ b/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 diff --git a/tests/actuator/test.go b/tests/actuator/test.go index e6d35c5d2..08cc5d95e 100644 --- a/tests/actuator/test.go +++ b/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) } }