search

gardener / machine-controller-manager-provider-azure

This repository is the out of tree implementation of the machine driver for Azure cloud provider
Apache License 2.0
8 stars 27 forks source link

Data disk image reference for vsmp #165

Closed hebelsan closed 1 month ago

hebelsan commented 3 months ago

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: https://github.com/gardener/gardener-extension-provider-azure/issues/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
hebelsan commented 3 months ago

Tested with: https://github.com/gardener/gardener-extension-provider-azure/pull/891

gardener-robot commented 2 months ago

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

YakovKugel commented 2 months ago

Hello All, any updates about the progress on that feature ?

hebelsan commented 2 months ago

@unmarshall, @rishabh-11 I addressed all of your improvements. Since you mentioned that you want to merge issue 936 first are there any updates or time estimation for when this fix will be done? If that fix got merged I will do one final test especially like you mentioned to check that when a DeleteMachine gets called, both NICs and Disks with Image References gets deleted in case of a failed vm creation.

elankath commented 2 months ago

@hebelsan thanks for your changes. Yes, we will address it this week. Please give us a few days for estimating release as we need to judge whether a few fixes should go in next upcoming release or deferred.

YakovKugel commented 2 months ago

Hello Colleagues, any update on that topic ?

rishabh-11 commented 2 months ago

Hey Yakov, the PR for fixing https://github.com/gardener/machine-controller-manager/issues/936 will be merged by tomorrow EOD. Then Alexander can do the test mentioned in https://github.com/gardener/machine-controller-manager-provider-azure/pull/165#issuecomment-2324629295 and after that we will merge this PR as well.

YakovKugel commented 2 months ago

Hello Rishab, thank you for your reply, when do you think it will be available in Gardener Canary, and which channel/ticket do I need to use for progress tracking?

hebelsan commented 2 months ago

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.

hebelsan commented 1 month ago

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

hebelsan commented 1 month ago

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 commented 1 month ago

/unhold

YakovKugel commented 1 month ago

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

kon-angelo commented 1 month ago

@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 2 weeks ago

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