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

Jump to bottom

Data disk image reference for vsmp #165

Merged

rishabh-11 merged 12 commits into gardener:master from hebelsan:vsmp

Sep 20, 2024

Contributor

hebelsan commented Aug 1, 2024

What this PR does / why we need it:
Adds the ability to attach dataDisks with image references.

Which issue(s) this PR fixes:
Related to: gardener/gardener-extension-provider-azure#788

Special notes for your reviewer:
For now it's not possible to create a dataDisk with imageReference afaik.
Therefore we create a general Disk with the imageReference before creating the VM.
This disk is then attached in the VM creation process.

Release note:

Adds the ability to attach dataDisks with image references

gardener-robot added needs/review size/m labels

gardener-robot-ci-3 added the reviewed/ok-to-test label

hebelsan mentioned this pull request

Data disk image reference #155

Closed

gardener-robot-ci-3 added needs/ok-to-test and removed reviewed/ok-to-test labels

hebelsan marked this pull request as ready for review

August 1, 2024 13:48

hebelsan requested review from a team as code owners

August 1, 2024 13:48

Contributor Author

hebelsan commented Aug 2, 2024

Tested with: gardener/gardener-extension-provider-azure#891

hebelsan mentioned this pull request

Data disk image reference gardener/gardener-extension-provider-azure#891

Merged

rishabh-11 assigned elankath and rishabh-11

rishabh-11 requested changes

View reviewed changes

Contributor

rishabh-11 left a comment

Initial, set of review comments.

pkg/azure/access/helpers/disk.go Show resolved Hide resolved

pkg/azure/api/providerspec.go

@@ @@ -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"`

Contributor

rishabh-11 Aug 12, 2024

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

pkg/azure/provider/helpers/driver.go Outdated

    
            @@ -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, imageRefDisks map[DataDiskLun]DiskID) (*armcompute.VirtualMachine, error) {

Contributor

rishabh-11 Aug 12, 2024

Can you rename imageRefDisks to imageRefDiskIDs everywhere?

pkg/azure/provider/helpers/driver.go Outdated

Comment on lines 639 to 644

+              		vmImagesAccess, err := factory.GetVirtualMachineImagesAccess(connectConfig)
+              		if err != nil {
+              			return status.WrapError(codes.Internal, fmt.Sprintf("Failed to create image access, Err: %v", err), err)
+              		}
+              		image, err := accesshelpers.GetVMImage(ctx, vmImagesAccess, location, imageRef)
+              		if err != nil {
+              			return status.WrapError(codes.Internal, fmt.Sprintf("Failed to get image, Err: %v", err), err)
+              		}
+              		disk.Properties.CreationData.ImageReference = &armcompute.ImageDiskReference{
+              			ID: image.ID,
+              		}

Contributor

rishabh-11 Aug 12, 2024 •

edited

Loading

This part is already there in getVirtualMachineImage. Why not reuse it? Also, we are using VM images as a disk image. Is that fine?

Contributor Author

hebelsan Aug 13, 2024

True, I really should make use of getVirtualMachineImage here.

In regards to your second question:
What do you mean with fine?
I think that is what this feature request is about.
If I understood it correctly they provide us basically two vm images and then boot from one into the other.

gardener-robot added the needs/changes label

gardener-robot-ci-3 added the reviewed/ok-to-test label

gardener-robot-ci-2 removed the reviewed/ok-to-test label

gardener-robot-ci-1 added reviewed/ok-to-test and removed reviewed/ok-to-test labels

gardener-robot-ci-3 added reviewed/ok-to-test and removed reviewed/ok-to-test labels

rishabh-11 assigned unmarshall and unassigned rishabh-11

unmarshall requested changes

View reviewed changes

pkg/azure/api/validation/validation_test.go Outdated Show resolved Hide resolved

pkg/azure/provider/helpers/connectconfig_test.go Outdated Show resolved Hide resolved

pkg/azure/provider/helpers/driver.go Outdated Show resolved Hide resolved

pkg/azure/provider/helpers/driver.go Outdated Show resolved Hide resolved

pkg/azure/provider/helpers/driver.go Outdated Show resolved Hide resolved

pkg/azure/provider/helpers/driver.go Show resolved Hide resolved

pkg/azure/provider/helpers/driver.go Show resolved Hide resolved

pkg/azure/provider/helpers/driver.go Outdated

+              		Tags:  utils.CreateResourceTags(providerSpec.Tags),
+              		Zones: getZonesFromProviderSpec(providerSpec),
+              	}
+              	err = setDiskImageReference(ctx, &params, specDataDisk, providerSpec.Location, factory, connectConfig)

Contributor

unmarshall Aug 23, 2024

Since setDiskImageReference is the only function that can return an error while constructing Disk creation params, it should be called first before you start to create the struct itself.

pkg/azure/provider/helpers/driver.go Outdated

+              	return
+              }
+              func setDiskImageReference(ctx context.Context, disk *armcompute.Disk, specDataDisk api.AzureDataDisk, location string, factory access.Factory, connectConfig access.ConnectConfig) error {

Contributor

unmarshall Aug 23, 2024

ideally this should only return an image ID which the caller should use.

Contributor Author

hebelsan Aug 28, 2024

I replaced the func setDiskImageReference with createDiskCreationData.
It now does not tamper on the disk reference anymore.

pkg/azure/provider/provider.go Show resolved Hide resolved

unmarshall added reviewed/do-not-merge status/on-hold labels

gardener-robot added the needs/rebase label

gardener-robot commented Aug 28, 2024

@hebelsan You need rebase this pull request with latest master branch. Please check.

gardener-robot-ci-2 added the reviewed/ok-to-test label

gardener-robot-ci-1 removed the reviewed/ok-to-test label


          Refactor createDiskCreationParams

2228e05

hebelsan force-pushed the vsmp branch from b886079 to 2228e05 Compare

September 13, 2024 13:29

gardener-robot-ci-1 added reviewed/ok-to-test and removed reviewed/ok-to-test labels

Contributor Author

hebelsan commented Sep 13, 2024

I tested the DeleteMachine by locally manipulating the createMachine function so it would always fail.
Then I created a shoot with two vms and verified that two disks and two nics got created although the vm creation failed.
After patching the shoot to only one vm, only one disk and one nic remained left.
I guess that should verify that in case of a failed vm the deletion still works as expected.

Contributor Author

hebelsan commented Sep 16, 2024

/hold just did a test which should successfully create a VM but it got stuck in "creating"

Contributor Author

hebelsan commented Sep 17, 2024

Found the issue of my setup.
The machine was stuck in creation because I used the following dataVolume image:
communityGalleryImageID: /CommunityGalleries/gardenlinux-13e998fe-534d-4b0a-8a27-f16a73aef620/Images/gardenlinux/Versions/1443.5.0
which was probably too old...
after switching to:
communityGalleryImageID: /CommunityGalleries/gardenlinux-13e998fe-534d-4b0a-8a27-f16a73aef620/Images/gardenlinux/Versions/1443.10.0
everything worked as expected.

hebelsan removed the reviewed/do-not-merge label

Contributor Author

hebelsan commented Sep 17, 2024

/unhold

rishabh-11 reviewed

View reviewed changes

Contributor

rishabh-11 left a comment

/lgtm

gardener-robot added reviewed/lgtm needs/changes and removed needs/changes needs/rebase needs/review labels

gardener-robot-ci-1 added the reviewed/ok-to-test label

gardener-robot removed the reviewed/lgtm label

unmarshall approved these changes

View reviewed changes

Contributor

unmarshall left a comment

/lgtm

gardener-robot added reviewed/lgtm and removed needs/changes labels

rishabh-11 approved these changes

View reviewed changes

rishabh-11 merged commit 99303ed into gardener:master

7 checks passed

gardener-robot added status/closed and removed status/on-hold labels

YakovKugel commented Sep 20, 2024

Hello @rishabh-11, congratulations on merging that feature! Could you please say when it will be available in Gardener Canary ?

Contributor

kon-angelo commented Sep 20, 2024

@YakovKugel it will come with the next release of https://github.com/gardener/gardener-extension-provider-azure probably in the next couple weeks.

YakovKugel commented Oct 31, 2024 •

edited

Loading

Hello @rishabh-11 , is that feature already in Gardener Canary and we could use ?

hebelsan deleted the vsmp branch

October 31, 2024 15:29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs/ok-to-test reviewed/lgtm reviewed/ok-to-test size/m status/closed