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.

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

Data disk image reference for vsmp #165

Merged
merged 12 commits into from
Sep 20, 2024
27 changes: 27 additions & 0 deletions pkg/azure/access/helpers/disk.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ package helpers

import (
"context"
"time"

"k8s.io/klog/v2"

"github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
Expand All @@ -17,6 +19,9 @@ import (

const (
diskDeleteServiceLabel = "disk_delete"
diskCreateServiceLabel = "disk_create"

defaultDiskOperationTimeout = 10 * time.Minute
)

// DeleteDisk deletes disk for passed in resourceGroup and diskName.
Expand All @@ -38,3 +43,25 @@ func DeleteDisk(ctx context.Context, client *armcompute.DisksClient, resourceGro
klog.Infof("Successfully deleted Disk: %s, for ResourceGroup: %s", diskName, resourceGroup)
return
}

// CreateDisk creates a Disk given a resourceGroup and disk creation parameters.
// NOTE: All calls to this Azure API are instrumented as prometheus metric.
func CreateDisk(ctx context.Context, client *armcompute.DisksClient, resourceGroup, diskName string, diskCreationParams armcompute.Disk) (disk *armcompute.Disk, err error) {
defer instrument.AZAPIMetricRecorderFn(diskCreateServiceLabel, &err)()

createCtx, cancelFn := context.WithTimeout(ctx, defaultDiskOperationTimeout)
defer cancelFn()
poller, err := client.BeginCreateOrUpdate(createCtx, resourceGroup, diskName, diskCreationParams, nil)
if err != nil {
errors.LogAzAPIError(err, "Failed to trigger create of Disk [Name: %s, ResourceGroup: %s]", resourceGroup, diskName)
return
}
createResp, err := poller.PollUntilDone(createCtx, nil)
if err != nil {
errors.LogAzAPIError(err, "Polling failed while waiting for create of Disk: %s for ResourceGroup: %s", diskName, resourceGroup)
return
}
disk = &createResp.Disk
klog.Infof("Successfully created Disk: %s, for ResourceGroup: %s", diskName, resourceGroup)
return
hebelsan marked this conversation as resolved.
Show resolved Hide resolved
}
2 changes: 2 additions & 0 deletions pkg/azure/api/providerspec.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ type AzureDataDisk struct {
StorageAccountType string `json:"storageAccountType,omitempty"`
// DiskSizeGB is the size of an empty disk in gigabytes.
DiskSizeGB int32 `json:"diskSizeGB,omitempty"`
// ImageRef optionally specifies an image source
ImageRef *AzureImageReference `json:"imageRef,omitempty"`
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 please validate the ImageRef if specified? For Data Disks the validation of image ref is currently missing. Modify the existing validateDataDisks function in validation.go for this

}

// AzureManagedDiskParameters is the parameters of a managed disk.
Expand Down
4 changes: 4 additions & 0 deletions pkg/azure/api/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,10 @@ func validateDataDisks(disks []api.AzureDataDisk, fldPath *field.Path) field.Err
if utils.IsEmptyString(disk.StorageAccountType) {
allErrs = append(allErrs, field.Required(fldPath.Child("storageAccountType"), "must provide storageAccountType"))
}

if disk.ImageRef != nil {
allErrs = append(allErrs, validateStorageImageRef(*disk.ImageRef, fldPath.Child("imageRef"))...)
}
}

for lun, numOccurrence := range luns {
Expand Down
3 changes: 2 additions & 1 deletion pkg/azure/api/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,8 @@ func TestValidateCloudConfiguration(t *testing.T) {
g := NewWithT(t)
t.Parallel()
for _, entry := range table {
t.Run(entry.description, func(_ *testing.T) {
t.Run(entry.description, func(t *testing.T) {
t.Parallel()
errList := validateCloudConfiguration(entry.cloudConfiguration, fldPath)
if entry.matcher != nil {
g.Expect(errList).To(entry.matcher)
Expand Down
3 changes: 2 additions & 1 deletion pkg/azure/provider/helpers/connectconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ func TestDetermineAzureCloudConfiguration(t *testing.T) {
g := NewWithT(t)
t.Parallel()
for _, test := range tests {
t.Run(test.description, func(_ *testing.T) {
t.Run(test.description, func(t *testing.T) {
t.Parallel()
cloudConfiguration := DetermineAzureCloudConfiguration(test.testConfiguration)
g.Expect(cloudConfiguration).To(Equal(*test.expectedOutput))
})
Expand Down
137 changes: 122 additions & 15 deletions pkg/azure/provider/helpers/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,18 @@ import (
"github.com/gardener/machine-controller-manager-provider-azure/pkg/azure/utils"
)

// DataDiskLun is a type alias for int32 which semantically represents a dataDisk lun
type DataDiskLun int32

// DiskID is a type alias for *string which semantically represents a Disk ID
type DiskID *string

// ExtractProviderSpecAndConnectConfig extracts api.AzureProviderSpec from mcc and access.ConnectConfig from secret.
func ExtractProviderSpecAndConnectConfig(mcc *v1alpha1.MachineClass, secret *corev1.Secret) (api.AzureProviderSpec, access.ConnectConfig, error) {
var (
err error
providerSpec api.AzureProviderSpec
connectConfig access.ConnectConfig
err error
providerSpec api.AzureProviderSpec
connectConfig access.ConnectConfig
)
// validate provider Spec provider. Exit early if it is not azure.
if err = validation.ValidateMachineClassProvider(mcc); err != nil {
Expand Down Expand Up @@ -127,7 +133,7 @@ func createDataDiskNames(providerSpec api.AzureProviderSpec, vmName string) []st
dataDisks := providerSpec.Properties.StorageProfile.DataDisks
diskNames := make([]string, 0, len(dataDisks))
for _, disk := range dataDisks {
diskName := utils.CreateDataDiskName(vmName, disk)
diskName := utils.CreateDataDiskName(vmName, disk.Name, disk.Lun)
diskNames = append(diskNames, diskName)
}
return diskNames
Expand Down Expand Up @@ -534,12 +540,12 @@ func checkAndAcceptAgreementIfNotAccepted(ctx context.Context, factory access.Fa
}

// CreateVM gathers the VM creation parameters and invokes a call to create or update the VM.
func CreateVM(ctx context.Context, factory access.Factory, connectConfig access.ConnectConfig, providerSpec api.AzureProviderSpec, imageRef armcompute.ImageReference, plan *armcompute.Plan, secret *corev1.Secret, nicID string, vmName string) (*armcompute.VirtualMachine, error) {
func CreateVM(ctx context.Context, factory access.Factory, connectConfig access.ConnectConfig, providerSpec api.AzureProviderSpec, vmImageRef armcompute.ImageReference, plan *armcompute.Plan, secret *corev1.Secret, nicID string, vmName string, imageRefDiskIDs map[DataDiskLun]DiskID) (*armcompute.VirtualMachine, error) {
vmAccess, err := factory.GetVirtualMachinesAccess(connectConfig)
if err != nil {
return nil, status.WrapError(codes.Internal, fmt.Sprintf("Failed to create virtual machine access to process request: [resourceGroup: %s, vmName: %s], Err: %v", providerSpec.ResourceGroup, vmName, err), err)
}
vmCreationParams, err := createVMCreationParams(providerSpec, imageRef, plan, secret, nicID, vmName)
vmCreationParams, err := createVMCreationParams(providerSpec, vmImageRef, plan, secret, nicID, vmName, imageRefDiskIDs)
if err != nil {
return nil, status.WrapError(codes.Internal, fmt.Sprintf("Failed to create virtual machine parameters to create VM: [ResourceGroup: %s, Name: %s], Err: %v", providerSpec.ResourceGroup, vmName, err), err)
}
Expand All @@ -552,6 +558,94 @@ func CreateVM(ctx context.Context, factory access.Factory, connectConfig access.
return vm, nil
}

// CreateDisksWithImageRef creates a disk with CreationData (e.g. ImageReference or GalleryImageReference)
func CreateDisksWithImageRef(ctx context.Context, factory access.Factory, connectConfig access.ConnectConfig, providerSpec api.AzureProviderSpec, vmName string) (map[DataDiskLun]DiskID, error) {
disksAccess, err := factory.GetDisksAccess(connectConfig)
if err != nil {
return nil, status.WrapError(codes.Internal, fmt.Sprintf("Failed to create disk access for VM: [ResourceGroup: %s], Err: %v", providerSpec.ResourceGroup, err), err)
}

disks := make(map[DataDiskLun]DiskID)
dataDiskSpecs := providerSpec.Properties.StorageProfile.DataDisks
if utils.IsSliceNilOrEmpty(dataDiskSpecs) {
return disks, nil
}

for _, specDataDisk := range dataDiskSpecs {
// skip if dataDisk does not have imageRef property
if specDataDisk.ImageRef == nil {
continue
}
diskName := utils.CreateDataDiskName(vmName, specDataDisk.Name, specDataDisk.Lun)
diskCreationParams, err := createDiskCreationParams(ctx, specDataDisk, providerSpec, factory, connectConfig)
if err != nil {
errCode := accesserrors.GetMatchingErrorCode(err)
return nil, status.WrapError(errCode, fmt.Sprintf("Failed to create disk creation params: [ResourceGroup: %s, Name: %s], Err: %v", providerSpec.ResourceGroup, diskName, err), err)
}
disk, err := accesshelpers.CreateDisk(ctx, disksAccess, providerSpec.ResourceGroup, diskName, diskCreationParams)
if err != nil {
errCode := accesserrors.GetMatchingErrorCode(err)
return nil, status.WrapError(errCode, fmt.Sprintf("Failed to create Disk: [ResourceGroup: %s, Name: %s], Err: %v", providerSpec.ResourceGroup, diskName, err), err)
}
disks[DataDiskLun(specDataDisk.Lun)] = disk.ID
klog.Infof("Successfully created Disk: [ResourceGroup: %s, Name: %s]", providerSpec.ResourceGroup, diskName)
}

return disks, nil
}

func createDiskCreationParams(ctx context.Context, specDataDisk api.AzureDataDisk, providerSpec api.AzureProviderSpec, factory access.Factory, connectConfig access.ConnectConfig) (params armcompute.Disk, err error) {
creationData, err := createDiskCreationData(ctx, specDataDisk, providerSpec.Location, factory, connectConfig)
if err != nil {
return armcompute.Disk{}, err
}
params = armcompute.Disk{
Location: to.Ptr(providerSpec.Location),
Properties: &armcompute.DiskProperties{
CreationData: creationData,
DiskSizeGB: to.Ptr[int32](specDataDisk.DiskSizeGB),
OSType: to.Ptr(armcompute.OperatingSystemTypesLinux),
},
SKU: &armcompute.DiskSKU{
Name: to.Ptr(armcompute.DiskStorageAccountTypes(specDataDisk.StorageAccountType)),
},
Tags: utils.CreateResourceTags(providerSpec.Tags),
Zones: getZonesFromProviderSpec(providerSpec),
}
return
}

func createDiskCreationData(ctx context.Context, specDataDisk api.AzureDataDisk, location string, factory access.Factory, connectConfig access.ConnectConfig) (*armcompute.CreationData, error) {
creationData := &armcompute.CreationData{
CreateOption: to.Ptr(armcompute.DiskCreateOptionFromImage),
}
if !utils.IsEmptyString(specDataDisk.ImageRef.ID) {
creationData.ImageReference = &armcompute.ImageDiskReference{
ID: &specDataDisk.ImageRef.ID,
}
} else if !utils.IsNilOrEmptyStringPtr(specDataDisk.ImageRef.SharedGalleryImageID) {
creationData.GalleryImageReference = &armcompute.ImageDiskReference{
SharedGalleryImageID: specDataDisk.ImageRef.SharedGalleryImageID,
}
} else if !utils.IsNilOrEmptyStringPtr(specDataDisk.ImageRef.CommunityGalleryImageID) {
creationData.GalleryImageReference = &armcompute.ImageDiskReference{
CommunityGalleryImageID: specDataDisk.ImageRef.CommunityGalleryImageID,
}
} else if !utils.IsNilOrEmptyStringPtr(specDataDisk.ImageRef.URN) {
hebelsan marked this conversation as resolved.
Show resolved Hide resolved
imageRef := utils.ToImageReference(*specDataDisk.ImageRef.URN)

image, err := getVirtualMachineImage(ctx, factory, connectConfig, location, imageRef)
if err != nil {
return nil, err
}

creationData.ImageReference = &armcompute.ImageDiskReference{
ID: image.ID,
}
}
return creationData, nil
}

// LogVMCreation is a convenience method which helps to extract relevant details from the created virtual machine and logs it.
// Today the azure create VM call is atomic only w.r.t creation of VM, OSDisk, DataDisk(s). NIC still has to be created prior to creation of the VM.
// Therefore, this method produces a log which also prints the OSDisk, DataDisks that are created (which helps in traceability). For completeness it
Expand All @@ -578,12 +672,16 @@ func LogVMCreation(location, resourceGroup string, vm *armcompute.VirtualMachine
klog.Infof(msgBuilder.String())
}

func createVMCreationParams(providerSpec api.AzureProviderSpec, imageRef armcompute.ImageReference, plan *armcompute.Plan, secret *corev1.Secret, nicID, vmName string) (armcompute.VirtualMachine, error) {
func createVMCreationParams(providerSpec api.AzureProviderSpec, imageRef armcompute.ImageReference, plan *armcompute.Plan, secret *corev1.Secret, nicID, vmName string, imageRefDiskIDs map[DataDiskLun]DiskID) (armcompute.VirtualMachine, error) {
vmTags := utils.CreateResourceTags(providerSpec.Tags)
sshConfiguration, err := getSSHConfiguration(providerSpec.Properties.OsProfile.LinuxConfiguration.SSH)
if err != nil {
return armcompute.VirtualMachine{}, err
}
dataDisks, err := getDataDisks(providerSpec.Properties.StorageProfile.DataDisks, vmName, imageRefDiskIDs)
if err != nil {
return armcompute.VirtualMachine{}, status.WrapError(codes.Internal, fmt.Sprintf("Failed to create vm creation params, Err: %v", err), err)
}

vm := armcompute.VirtualMachine{
Location: to.Ptr(providerSpec.Location),
Expand Down Expand Up @@ -613,7 +711,7 @@ func createVMCreationParams(providerSpec api.AzureProviderSpec, imageRef armcomp
},
},
StorageProfile: &armcompute.StorageProfile{
DataDisks: getDataDisks(providerSpec.Properties.StorageProfile.DataDisks, vmName),
DataDisks: dataDisks,
ImageReference: &imageRef,
OSDisk: &armcompute.OSDisk{
CreateOption: to.Ptr(armcompute.DiskCreateOptionTypes(providerSpec.Properties.StorageProfile.OsDisk.CreateOption)),
Expand Down Expand Up @@ -663,19 +761,18 @@ func createVMCreationParams(providerSpec api.AzureProviderSpec, imageRef armcomp
return vm, nil
}

func getDataDisks(specDataDisks []api.AzureDataDisk, vmName string) []*armcompute.DataDisk {
func getDataDisks(dataDiskSpecs []api.AzureDataDisk, vmName string, imageRefDiskIDs map[DataDiskLun]DiskID) ([]*armcompute.DataDisk, error) {
var dataDisks []*armcompute.DataDisk
if utils.IsSliceNilOrEmpty(specDataDisks) {
return dataDisks
if utils.IsSliceNilOrEmpty(dataDiskSpecs) {
return dataDisks, nil
}
for _, specDataDisk := range specDataDisks {
dataDiskName := utils.CreateDataDiskName(vmName, specDataDisk)
for _, specDataDisk := range dataDiskSpecs {
dataDiskName := utils.CreateDataDiskName(vmName, specDataDisk.Name, specDataDisk.Lun)
caching := armcompute.CachingTypesNone
if !utils.IsEmptyString(specDataDisk.Caching) {
caching = armcompute.CachingTypes(specDataDisk.Caching)
}
dataDisk := &armcompute.DataDisk{

CreateOption: to.Ptr(armcompute.DiskCreateOptionTypesEmpty),
Lun: to.Ptr(specDataDisk.Lun),
Caching: to.Ptr(caching),
Expand All @@ -686,9 +783,19 @@ func getDataDisks(specDataDisks []api.AzureDataDisk, vmName string) []*armcomput
},
Name: to.Ptr(dataDiskName),
}
if specDataDisk.ImageRef != nil {
diskID := imageRefDiskIDs[DataDiskLun(specDataDisk.Lun)]
if diskID == nil {
hebelsan marked this conversation as resolved.
Show resolved Hide resolved
return nil, fmt.Errorf("unexpected error, this cannot happen and points to a coding error: "+
"could not find id of pre created disk '%s' with lun '%d'",
dataDiskName, specDataDisk.Lun)
}
dataDisk.CreateOption = to.Ptr(armcompute.DiskCreateOptionTypesAttach)
dataDisk.ManagedDisk.ID = diskID
}
dataDisks = append(dataDisks, dataDisk)
}
return dataDisks
return dataDisks, nil
}

func getVMIdentity(specVMIdentityID *string) *armcompute.VirtualMachineIdentity {
Expand Down
2 changes: 1 addition & 1 deletion pkg/azure/provider/helpers/resourcegraphprocessor.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func getDataDiskNameSuffixes(providerSpec api.AzureProviderSpec) sets.Set[string
dataDiskNameSuffixes := sets.New[string]()
dataDisks := providerSpec.Properties.StorageProfile.DataDisks
for _, dataDisk := range dataDisks {
dataDiskNameSuffixes.Insert(utils.GetDataDiskNameSuffix(dataDisk))
dataDiskNameSuffixes.Insert(utils.GetDataDiskNameSuffix(dataDisk.Name, dataDisk.Lun))
}
return dataDiskNameSuffixes
}
11 changes: 10 additions & 1 deletion pkg/azure/provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ func (d defaultDriver) CreateMachine(ctx context.Context, req *driver.CreateMach
if err != nil {
return
}

subnet, err := helpers.GetSubnet(ctx, d.factory, connectConfig, providerSpec)
if err != nil {
return
Expand All @@ -79,10 +80,18 @@ func (d defaultDriver) CreateMachine(ctx context.Context, req *driver.CreateMach
return
}

vm, err := helpers.CreateVM(ctx, d.factory, connectConfig, providerSpec, imageReference, plan, req.Secret, nicID, vmName)
// create disks with image ref since they can not be created together with the vm
// TODO parallelize creation with nic?
imageRefDiskIDs, err := helpers.CreateDisksWithImageRef(ctx, d.factory, connectConfig, providerSpec, vmName)
unmarshall marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return
}

vm, err := helpers.CreateVM(ctx, d.factory, connectConfig, providerSpec, imageReference, plan, req.Secret, nicID, vmName, imageRefDiskIDs)
if err != nil {
return
}

resp = helpers.ConstructCreateMachineResponse(providerSpec.Location, vmName)
helpers.LogVMCreation(providerSpec.Location, providerSpec.ResourceGroup, vm)
return
Expand Down
28 changes: 7 additions & 21 deletions pkg/azure/testhelp/fakes/machineresources.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package fakes

import (
"fmt"
"strings"

"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5"
Expand Down Expand Up @@ -293,7 +292,7 @@ func createDataDiskResources(spec api.AzureProviderSpec, vmID *string, vmName st
specDataDisks := spec.Properties.StorageProfile.DataDisks
dataDisks := make(map[string]*armcompute.Disk, len(specDataDisks))
for _, specDataDisk := range specDataDisks {
diskName := utils.CreateDataDiskName(vmName, specDataDisk)
diskName := utils.CreateDataDiskName(vmName, specDataDisk.Name, specDataDisk.Lun)
dataDisks[diskName] = createDiskResource(spec, diskName, vmID, nil)
}
return dataDisks
Expand Down Expand Up @@ -374,31 +373,18 @@ func createVMResource(spec api.AzureProviderSpec, vmName string, plan *armcomput
}

func createImageReference(imageRef api.AzureImageReference) *armcompute.ImageReference {
var (
id *string
publisher *string
sku *string
offer *string
version *string
)
if !utils.IsEmptyString(imageRef.ID) {
id = to.Ptr(imageRef.ID)
return &armcompute.ImageReference{
ID: to.Ptr(imageRef.ID),
}
}
if !utils.IsNilOrEmptyStringPtr(imageRef.URN) {
urnParts := strings.Split(*imageRef.URN, ":")
publisher = to.Ptr(urnParts[0])
offer = to.Ptr(urnParts[1])
sku = to.Ptr(urnParts[2])
version = to.Ptr(urnParts[3])
urnImageRef := utils.ToImageReference(*imageRef.URN)
return &urnImageRef
}
return &armcompute.ImageReference{
CommunityGalleryImageID: imageRef.CommunityGalleryImageID,
ID: id,
Offer: offer,
Publisher: publisher,
SKU: sku,
SharedGalleryImageID: imageRef.SharedGalleryImageID,
Version: version,
}
}

Expand Down Expand Up @@ -440,7 +426,7 @@ func createDataDisks(spec api.AzureProviderSpec, vmName string, deleteOption *ar
}
dataDisks := make([]*armcompute.DataDisk, 0, len(specDataDisks))
for _, disk := range specDataDisks {
diskName := utils.CreateDataDiskName(vmName, disk)
diskName := utils.CreateDataDiskName(vmName, disk.Name, disk.Lun)
d := createDataDisk(disk.Lun, armcompute.CachingTypes(disk.Caching), deleteOption, disk.DiskSizeGB, armcompute.StorageAccountTypes(disk.StorageAccountType), diskName)
dataDisks = append(dataDisks, d)
}
Expand Down
Loading