diff --git a/examples/machine-set.yaml b/examples/machine-set.yaml index 6b2fcb963..d56617075 100644 --- a/examples/machine-set.yaml +++ b/examples/machine-set.yaml @@ -33,7 +33,7 @@ spec: ignKey: /var/lib/libvirt/images/worker.ign volume: poolName: default - baseVolumeID: /var/lib/libvirt/images/coreos_base + baseVolumeID: coreos_base networkInterfaceName: tectonic networkInterfaceAddress: 192.168.124.12 autostart: false diff --git a/examples/machine-with-full-paths.yaml b/examples/machine-with-full-paths.yaml index 696a4a114..7e087700f 100644 --- a/examples/machine-with-full-paths.yaml +++ b/examples/machine-with-full-paths.yaml @@ -18,7 +18,7 @@ spec: ignKey: /var/lib/libvirt/images/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/examples/machine-with-userdata.yml b/examples/machine-with-userdata.yml index f857f203e..098f76ddc 100644 --- a/examples/machine-with-userdata.yml +++ b/examples/machine-with-userdata.yml @@ -21,7 +21,7 @@ spec: userDataSecret: libvirt-actuator-user-data-secret volume: poolName: default - baseVolumeID: /var/lib/libvirt/images/fedora_base + baseVolumeID: fedora_base networkInterfaceName: default networkInterfaceAddress: 192.168.122.0/24 autostart: false diff --git a/examples/machine.yaml b/examples/machine.yaml index ff65030c1..d1954014e 100644 --- a/examples/machine.yaml +++ b/examples/machine.yaml @@ -19,7 +19,7 @@ spec: ignKey: /var/lib/libvirt/images/worker.ign volume: poolName: default - baseVolumeID: /var/lib/libvirt/images/coreos_base + baseVolumeID: coreos_base networkInterfaceName: tectonic networkInterfaceAddress: 192.168.124.12 autostart: false diff --git a/pkg/cloud/libvirt/actuators/machine/actuator.go b/pkg/cloud/libvirt/actuators/machine/actuator.go index cfd3e6044..9c023c111 100644 --- a/pkg/cloud/libvirt/actuators/machine/actuator.go +++ b/pkg/cloud/libvirt/actuators/machine/actuator.go @@ -118,7 +118,7 @@ func (a *Actuator) Create(context context.Context, cluster *machinev1.Cluster, m return a.handleMachineError(machine, apierrors.InvalidMachineConfiguration("error getting machineProviderConfig from spec: %v", err), createEventAction) } - client, err := a.clientBuilder(machineProviderConfig.URI) + client, err := a.clientBuilder(machineProviderConfig.URI, machineProviderConfig.Volume.PoolName) if err != nil { return a.handleMachineError(machine, apierrors.CreateMachine("error creating libvirt client: %v", err), createEventAction) } @@ -157,7 +157,7 @@ func (a *Actuator) Delete(context context.Context, cluster *machinev1.Cluster, m return a.handleMachineError(machine, apierrors.InvalidMachineConfiguration("error getting machineProviderConfig from spec: %v", err), deleteEventAction) } - client, err := a.clientBuilder(machineProviderConfig.URI) + client, err := a.clientBuilder(machineProviderConfig.URI, machineProviderConfig.Volume.PoolName) if err != nil { return a.handleMachineError(machine, apierrors.DeleteMachine("error creating libvirt client: %v", err), deleteEventAction) } @@ -185,7 +185,7 @@ func (a *Actuator) Update(context context.Context, cluster *machinev1.Cluster, m return a.handleMachineError(machine, apierrors.InvalidMachineConfiguration("error getting machineProviderConfig from spec: %v", err), updateEventAction) } - client, err := a.clientBuilder(machineProviderConfig.URI) + client, err := a.clientBuilder(machineProviderConfig.URI, machineProviderConfig.Volume.PoolName) if err != nil { return a.handleMachineError(machine, apierrors.UpdateMachine("error creating libvirt client: %v", err), updateEventAction) } @@ -218,7 +218,7 @@ func (a *Actuator) Exists(context context.Context, cluster *machinev1.Cluster, m return false, a.handleMachineError(machine, apierrors.InvalidMachineConfiguration("error getting machineProviderConfig from spec: %v", err), noEventAction) } - client, err := a.clientBuilder(machineProviderConfig.URI) + client, err := a.clientBuilder(machineProviderConfig.URI, machineProviderConfig.Volume.PoolName) if err != nil { return false, errWrapper.WithLog(err, "error creating libvirt client") } @@ -245,10 +245,9 @@ func (a *Actuator) createVolumeAndDomain(machine *machinev1.Machine, machineProv // Create volume if err := client.CreateVolume( libvirtclient.CreateVolumeInput{ - VolumeName: domainName, - PoolName: machineProviderConfig.Volume.PoolName, - BaseVolumeID: machineProviderConfig.Volume.BaseVolumeID, - VolumeFormat: "qcow2", + VolumeName: domainName, + BaseVolumeName: machineProviderConfig.Volume.BaseVolumeID, + VolumeFormat: "qcow2", }); err != nil { return nil, a.handleMachineError(machine, apierrors.CreateMachine("error creating volume %v", err), createEventAction) } @@ -261,7 +260,6 @@ func (a *Actuator) createVolumeAndDomain(machine *machinev1.Machine, machineProv VolumeName: domainName, CloudInitVolumeName: cloudInitVolumeName(domainName), IgnitionVolumeName: ignitionVolumeName(domainName), - VolumePoolName: machineProviderConfig.Volume.PoolName, NetworkInterfaceName: machineProviderConfig.NetworkInterfaceName, NetworkInterfaceAddress: machineProviderConfig.NetworkInterfaceAddress, AddressRange: a.cidrOffset, diff --git a/pkg/cloud/libvirt/actuators/machine/actuator_test.go b/pkg/cloud/libvirt/actuators/machine/actuator_test.go index fa2816137..6ab318b50 100644 --- a/pkg/cloud/libvirt/actuators/machine/actuator_test.go +++ b/pkg/cloud/libvirt/actuators/machine/actuator_test.go @@ -180,7 +180,7 @@ func TestMachineEvents(t *testing.T) { params := ActuatorParams{ ClusterClient: fakeclusterclientset.NewSimpleClientset(tc.machine), KubeClient: kubernetesfake.NewSimpleClientset(), - ClientBuilder: func(uri string) (libvirtclient.Client, error) { + ClientBuilder: func(uri string, pool string) (libvirtclient.Client, error) { if tc.error == libvirtClientError { return nil, fmt.Errorf(libvirtClientError) } diff --git a/pkg/cloud/libvirt/actuators/machine/stubs.go b/pkg/cloud/libvirt/actuators/machine/stubs.go index 8835aac28..6ca529233 100644 --- a/pkg/cloud/libvirt/actuators/machine/stubs.go +++ b/pkg/cloud/libvirt/actuators/machine/stubs.go @@ -25,7 +25,7 @@ func stubProviderConfig() *providerconfigv1.LibvirtMachineProviderConfig { }, Volume: &providerconfigv1.Volume{ PoolName: "default", - BaseVolumeID: "/var/lib/libvirt/images/fedora_base", + BaseVolumeID: "fedora_base", }, NetworkInterfaceName: "default", NetworkInterfaceAddress: "192.168.124.12/24", diff --git a/pkg/cloud/libvirt/client/client.go b/pkg/cloud/libvirt/client/client.go index 33f85ef1e..d822ed1f1 100644 --- a/pkg/cloud/libvirt/client/client.go +++ b/pkg/cloud/libvirt/client/client.go @@ -36,9 +36,6 @@ type CreateDomainInput struct { // IgnitionVolumeName of ignition volume to be added to domain definition IgnitionVolumeName string - // VolumePoolName of pool where VolumeName volume is located - VolumePoolName string - // NetworkInterfaceName as name of network interface NetworkInterfaceName string @@ -72,11 +69,8 @@ type CreateVolumeInput struct { // VolumeName to be created VolumeName string - // PoolName where VolumeName volume is located - PoolName string - - // BaseVolumeID as base volume ID - BaseVolumeID string + // BaseVolumeName as name of the base volume + BaseVolumeName string // Source as location of base volume Source string @@ -86,7 +80,7 @@ type CreateVolumeInput struct { } // LibvirtClientBuilderFuncType is function type for building aws client -type LibvirtClientBuilderFuncType func(URI string) (Client, error) +type LibvirtClientBuilderFuncType func(URI string, poolName string) (Client, error) // Client is a wrapper object for actual libvirt library to allow for easier testing. type Client interface { @@ -120,12 +114,17 @@ type Client interface { type libvirtClient struct { connection *libvirt.Connect + + // storage pool that holds all volumes + pool *libvirt.StoragePool + // cache pool's name so we don't have to call failable GetName() method on pool all the time. + poolName string } var _ Client = &libvirtClient{} // NewClient returns libvirt client for the specified URI -func NewClient(URI string) (Client, error) { +func NewClient(URI string, poolName string) (Client, error) { connection, err := libvirt.NewConnect(URI) if err != nil { return nil, err @@ -133,8 +132,15 @@ func NewClient(URI string) (Client, error) { glog.Infof("Created libvirt connection: %p", connection) + pool, err := connection.LookupStoragePoolByName(poolName) + if err != nil { + return nil, fmt.Errorf("can't find storage pool %q: %v", poolName, err) + } + return &libvirtClient{ connection: connection, + pool: pool, + poolName: poolName, }, nil } @@ -170,15 +176,24 @@ func (client *libvirtClient) CreateDomain(input CreateDomainInput) error { glog.Info("Create ignition configuration") if input.Ignition != nil { - if err := setIgnition(&domainDef, client, input.Ignition, input.KubeClient, input.MachineNamespace, input.IgnitionVolumeName, input.VolumePoolName); err != nil { + if err := setIgnition(&domainDef, client, input.Ignition, input.KubeClient, input.MachineNamespace, input.IgnitionVolumeName); err != nil { return err } } else if input.IgnKey != "" { - if err := setCoreOSIgnition(&domainDef, input.IgnKey); err != nil { + ignVolume, err := client.getVolume(input.IgnKey) + 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 if input.CloudInit != nil { - if err := setCloudInit(&domainDef, client, input.CloudInit, input.KubeClient, input.MachineNamespace, input.CloudInitVolumeName, input.VolumePoolName, input.DomainName); err != nil { + if err := setCloudInit(&domainDef, client, input.CloudInit, input.KubeClient, input.MachineNamespace, input.CloudInitVolumeName, input.DomainName); err != nil { return err } } else { @@ -186,8 +201,11 @@ func (client *libvirtClient) CreateDomain(input CreateDomainInput) error { } glog.Info("Create volume") - VolumeKey := baseVolumePath + input.VolumeName - if err := setDisks(&domainDef, client.connection, VolumeKey); err != nil { + diskVolume, err := client.getVolume(input.VolumeName) + if err != nil { + return fmt.Errorf("can't retrieve volume %s for pool %s: %v", input.VolumeName, client.poolName, err) + } + if err := setDisks(&domainDef, diskVolume); err != nil { return fmt.Errorf("Failed to setDisks: %s", err) } @@ -331,28 +349,14 @@ func (client *libvirtClient) DeleteDomain(name string) error { // CreateVolume creates volume based on CreateVolumeInput func (client *libvirtClient) CreateVolume(input CreateVolumeInput) error { var volume *libvirt.StorageVol - - glog.Infof("Create a libvirt volume with name %s for pool %s from the base volume %s", input.VolumeName, input.PoolName, input.BaseVolumeID) + glog.Infof("Create a libvirt volume with name %s for pool %s from the base volume %s", input.VolumeName, client.poolName, input.BaseVolumeName) // TODO: lock pool - //client.poolMutexKV.Lock(input.PoolName) - //defer client.poolMutexKV.Unlock(input.PoolName) - - pool, err := client.connection.LookupStoragePoolByName(input.PoolName) - if err != nil { - return fmt.Errorf("can't find storage pool '%s'", input.PoolName) - } - defer pool.Free() + //client.poolMutexKV.Lock(client.poolName) + //defer client.poolMutexKV.Unlock(client.poolName) - // 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(input.VolumeName); err == nil { + volume, err := client.getVolume(input.VolumeName) + if err == nil { return fmt.Errorf("storage volume '%s' already exists", input.VolumeName) } @@ -361,8 +365,8 @@ func (client *libvirtClient) CreateVolume(input CreateVolumeInput) error { var img image // an source image was given, this mean we can't choose size if input.Source != "" { - if input.BaseVolumeID != "" { - return fmt.Errorf("'base_volume_id' can't be specified when also 'source' is given") + if input.BaseVolumeName != "" { + return fmt.Errorf("'base_volume_name' can't be specified when also 'source' is given") } if img, err = newImage(input.Source); err != nil { @@ -377,17 +381,18 @@ func (client *libvirtClient) CreateVolume(input CreateVolumeInput) error { glog.Infof("Image %s image is: %d bytes", img, size) volumeDef.Capacity.Unit = "B" volumeDef.Capacity.Value = size - } else if input.BaseVolumeID != "" { + } else if input.BaseVolumeName != "" { volume = nil - baseVolume, err := client.connection.LookupStorageVolByKey(input.BaseVolumeID) + baseVolume, err := client.getVolume(input.BaseVolumeName) + if err != nil { - return fmt.Errorf("Can't retrieve volume %s", input.BaseVolumeID) + return fmt.Errorf("Can't retrieve volume %s", input.BaseVolumeName) } var baseVolumeInfo *libvirt.StorageVolInfo baseVolumeInfo, err = baseVolume.GetInfo() if err != nil { - return fmt.Errorf("Can't retrieve volume info %s", input.BaseVolumeID) + return fmt.Errorf("Can't retrieve volume info %s", input.BaseVolumeName) } if baseVolumeInfo.Capacity > uint64(defaultSize) { volumeDef.Capacity.Value = baseVolumeInfo.Capacity @@ -396,7 +401,7 @@ func (client *libvirtClient) CreateVolume(input CreateVolumeInput) error { } backingStoreDef, err := newDefBackingStoreFromLibvirt(baseVolume) if err != nil { - return fmt.Errorf("Could not retrieve backing store %s", input.BaseVolumeID) + return fmt.Errorf("Could not retrieve backing store %s", input.BaseVolumeName) } volumeDef.BackingStore = &backingStoreDef } @@ -408,7 +413,16 @@ func (client *libvirtClient) CreateVolume(input CreateVolumeInput) error { } // create the volume - v, err := pool.StorageVolCreateXML(string(volumeDefXML), 0) + // 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 client.pool.Refresh(0) + }) + if err != nil { + return fmt.Errorf("can't find storage pool '%s'", client.poolName) + } + + v, err := client.pool.StorageVolCreateXML(string(volumeDefXML), 0) if err != nil { return fmt.Errorf("Error creating libvirt volume: %s", err) } @@ -434,14 +448,13 @@ func (client *libvirtClient) CreateVolume(input CreateVolumeInput) error { } // VolumeExists checks if a volume exists -func (client *libvirtClient) VolumeExists(volumeName string) (bool, error) { - glog.Infof("Check if %q volume exists", volumeName) +func (client *libvirtClient) VolumeExists(name string) (bool, error) { + glog.Infof("Check if %q volume exists", name) if client.connection == nil { return false, ErrLibVirtConIsNil } - volumePath := fmt.Sprintf(baseVolumePath+"%s", volumeName) - volume, err := client.connection.LookupStorageVolByPath(volumePath) + volume, err := client.getVolume(name) if err != nil { return false, nil } @@ -449,6 +462,20 @@ func (client *libvirtClient) VolumeExists(volumeName string) (bool, error) { return true, nil } +func (client *libvirtClient) getVolume(volumeName string) (*libvirt.StorageVol, error) { + // Check whether the storage volume exists. Its name needs to be + // unique. + volume, err := client.pool.LookupStorageVolByName(volumeName) + if err != nil { + // Let's try by ID in case of older Installer + volume, err = client.connection.LookupStorageVolByKey(volumeName) + if err != nil { + return nil, fmt.Errorf("can't retrieve volume %q: %v", volumeName, err) + } + } + return volume, nil +} + // DeleteVolume deletes a domain based on its name func (client *libvirtClient) DeleteVolume(name string) error { exists, err := client.VolumeExists(name) @@ -461,10 +488,9 @@ func (client *libvirtClient) DeleteVolume(name string) error { } glog.Infof("Deleting volume %s", name) - volumePath := fmt.Sprintf(baseVolumePath+"%s", name) - volume, err := client.connection.LookupStorageVolByPath(volumePath) + volume, err := client.getVolume(name) if err != nil { - return fmt.Errorf("Can't retrieve volume %s", volumePath) + return fmt.Errorf("Can't retrieve volume %s", name) } defer volume.Free() @@ -477,12 +503,8 @@ func (client *libvirtClient) DeleteVolume(name string) error { defer volPool.Free() // TODO: add locking support - //poolName, err := volPool.GetName() - //if err != nil { - // return fmt.Errorf("Error retrieving name of volume: %s", err) - //} - //client.poolMutexKV.Lock(poolName) - //defer client.poolMutexKV.Unlock(poolName) + //client.poolMutexKV.Lock(client.poolName) + //defer client.poolMutexKV.Unlock(client.poolName) waitForSuccess("Error refreshing pool for volume", func() error { return volPool.Refresh(0) @@ -493,12 +515,12 @@ func (client *libvirtClient) DeleteVolume(name string) error { // 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/pkg/cloud/libvirt/client/cloudinit.go b/pkg/cloud/libvirt/client/cloudinit.go index 85105e45e..ad9f0c1a6 100644 --- a/pkg/cloud/libvirt/client/cloudinit.go +++ b/pkg/cloud/libvirt/client/cloudinit.go @@ -19,7 +19,7 @@ import ( providerconfigv1 "github.com/openshift/cluster-api-provider-libvirt/pkg/apis/libvirtproviderconfig/v1beta1" ) -func setCloudInit(domainDef *libvirtxml.Domain, client *libvirtClient, cloudInit *providerconfigv1.CloudInit, kubeClient kubernetes.Interface, machineNamespace, volumeName, poolName, domainName string) error { +func setCloudInit(domainDef *libvirtxml.Domain, client *libvirtClient, cloudInit *providerconfigv1.CloudInit, kubeClient kubernetes.Interface, machineNamespace, volumeName, domainName string) error { // At least user data or ssh access needs to be set to create the cloud init if cloudInit.UserDataSecret == "" && !cloudInit.SSHAccess { @@ -56,7 +56,7 @@ func setCloudInit(domainDef *libvirtxml.Domain, client *libvirtClient, cloudInit cloudInitDef.UserData = string(userData) cloudInitDef.MetaData = string(metaData) cloudInitDef.Name = cloudInitISOName - cloudInitDef.PoolName = poolName + cloudInitDef.PoolName = client.poolName glog.Infof("cloudInitDef: %+v", cloudInitDef) diff --git a/pkg/cloud/libvirt/client/domain.go b/pkg/cloud/libvirt/client/domain.go index 966236d00..0bbef71fe 100644 --- a/pkg/cloud/libvirt/client/domain.go +++ b/pkg/cloud/libvirt/client/domain.go @@ -19,10 +19,6 @@ import ( "github.com/openshift/cluster-api-provider-libvirt/lib/cidr" ) -const ( - baseVolumePath = "/var/lib/libvirt/images/" -) - // ErrLibVirtConIsNil is returned when the libvirt connection is nil. var ErrLibVirtConIsNil = errors.New("the libvirt connection was nil") @@ -265,13 +261,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) - glog.Info("Looking up storage volume by key") - diskVolume, err := virConn.LookupStorageVolByKey(volumeKey) - if err != nil { - return fmt.Errorf("Can't retrieve volume %s", volumeKey) - } glog.Info("Getting disk volume") diskVolumeFile, err := diskVolume.GetPath() if err != nil { diff --git a/pkg/cloud/libvirt/client/ignition.go b/pkg/cloud/libvirt/client/ignition.go index 79d5a4042..5cd6f8ba2 100644 --- a/pkg/cloud/libvirt/client/ignition.go +++ b/pkg/cloud/libvirt/client/ignition.go @@ -15,7 +15,7 @@ import ( providerconfigv1 "github.com/openshift/cluster-api-provider-libvirt/pkg/apis/libvirtproviderconfig/v1beta1" ) -func setIgnition(domainDef *libvirtxml.Domain, client *libvirtClient, ignition *providerconfigv1.Ignition, kubeClient kubernetes.Interface, machineNamespace, volumeName, poolName string) error { +func setIgnition(domainDef *libvirtxml.Domain, client *libvirtClient, ignition *providerconfigv1.Ignition, kubeClient kubernetes.Interface, machineNamespace, volumeName string) error { glog.Info("Creating ignition file") ignitionDef := newIgnitionDef() @@ -33,7 +33,7 @@ func setIgnition(domainDef *libvirtxml.Domain, client *libvirtClient, ignition * } ignitionDef.Name = volumeName - ignitionDef.PoolName = poolName + ignitionDef.PoolName = client.poolName ignitionDef.Content = string(userDataSecret) glog.Infof("Ignition: %+v", ignitionDef) diff --git a/test/utils/manifests.go b/test/utils/manifests.go index 05933b261..ef8294c00 100644 --- a/test/utils/manifests.go +++ b/test/utils/manifests.go @@ -16,7 +16,7 @@ func TestingMachineProviderSpec(uri, clusterID string) (machinev1.ProviderSpec, }, Volume: &providerconfigv1.Volume{ PoolName: "default", - BaseVolumeID: "/var/lib/libvirt/images/fedora_base", + BaseVolumeID: "fedora_base", }, NetworkInterfaceName: "default", NetworkInterfaceAddress: "192.168.124.12/24", @@ -45,7 +45,7 @@ func MasterMachineProviderSpec(masterUserDataSecret, libvirturi string) (machine }, Volume: &providerconfigv1.Volume{ PoolName: "default", - BaseVolumeID: "/var/lib/libvirt/images/fedora_base", + BaseVolumeID: "fedora_base", }, NetworkInterfaceName: "default", NetworkInterfaceAddress: "192.168.122.0/24",