Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drop volumes full paths #45

Closed
wants to merge 1 commit into from
Closed

drop volumes full paths #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions cloud/libvirt/actuators/machine/actuator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although you've added pool here, why do we pass name, twice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although you've added pool here, why do we pass name, twice?

We actually pass it three times ;). CreateDomain allows the domain, volume, and host names to be configured seperately, but we opt to use the same value for all three here.

return fmt.Errorf("error creating domain: %v", err)
}
return nil
Expand All @@ -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)
}
Expand All @@ -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
Expand Down
33 changes: 16 additions & 17 deletions cloud/libvirt/actuators/machine/utils/domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
}
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add which pool:

error getting ignition from pool %q ...

It will help any subsequent debugging if we know which pool (by name).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add which pool...

I'd rather handle that in getVolumeFromPool, where all callers can benefit. And at least one of the errors there already includes the pool name.

}
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If getVolumeFromPool becomes consistent about listing the volume and pool in its error messages, we can drop them here.

}
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)
}

Expand Down
83 changes: 45 additions & 38 deletions cloud/libvirt/actuators/machine/utils/volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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")
}

Expand All @@ -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
}
Expand All @@ -171,6 +158,17 @@ func CreateVolume(volumeName, poolName, baseVolumeID, source, volumeFormat strin
}

// create the volume
pool, err := virConn.LookupStoragePoolByName(poolName)
defer pool.Free()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you actually call pool.Free() in the face of an error? Won't pool be nil in those cases?


// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this exit? Will it run forever if the pool fails to refresh successfully?

})
if err != nil {
return fmt.Errorf("can't find storage pool '%s'", poolName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use %q for quoting strings.

}
v, err := pool.StorageVolCreateXML(string(volumeDefXML), 0)
if err != nil {
return fmt.Errorf("Error creating libvirt volume: %s", err)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this run forever?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm new to Go/Installer so maybe I'm missing something but pool.Refresh call is blocking and since it does I/O, theoretically it can take a long time before returning. If the call returns an error, it's very unlikely to succeed if called again. So I'm not sure looking over it until it succeeds is the way to go here.

})

// 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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to remove this and instead free the returned volumes in each of the calling sites?

return volume, nil
}

// DeleteVolume removes the volume identified by `key` from libvirt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment needs updating; key isn't listed as a parameter.

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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove dead code.

//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
Expand Down
4 changes: 2 additions & 2 deletions examples/machine-with-full-paths.yaml → examples/machine-with-vol-names.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions tests/actuator/test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
}
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
}

Expand Down