Added support for Data Disks in Azure and AliCloud #397

guydc · 2020-01-29T18:18:08Z

What this PR does / why we need it:

Enable MCM support for Data Disks in AliCloud and Azure. Aligns MCM’s capability to create and attach multiple devices across all major IaaS providers, for use cases such as memory expansion, resource isolation and encryption.

Use a test suite for all driver tests.

Which issue(s) this PR fixes:
Fixes #396
Part of gardener/gardener#1743

Special notes for your reviewer:

Release note:

Added support for multiple Data Disks in Azure and AliCloud.

(cherry picked from commit 7c658e2911155d7acb49a6f54ade3190e4c76718)

hardikdr · 2020-01-31T11:34:31Z

thanks @guydaichs for the PR, we shall review it soon.

pkg/apis/machine/validation/awsmachineclass.go

pkg/driver/driver_alicloud.go

pkg/driver/driver_azure.go

hardikdr · 2020-02-06T10:56:46Z

Thanks @guydaichs for PR/feature. As this does not fall under our existing CI scope[multiple-disks], we shall test it out separately soon, and update the thread here.

@jia-jerry @dkistner Can you please take a brief look for the Alicloud and Azure part respectively :-)

hardikdr · 2020-02-06T10:57:54Z

cc @rewiko
As you had enabled support for multiple-disks recently in other PR.

prashanth26 · 2020-02-06T10:58:48Z

Looks like there is an issue here -
pkg/apis/machine/validation/azuremachineclass.go:144: Sprintf format %s has arg lun of wrong type int32

dkistner · 2020-02-07T07:55:17Z

I will try to check the azure specific part soon.

guydc · 2020-02-10T17:08:48Z

@rewiko - I added a verification for block device names in awsmachineclass, based on the recommended naming documented by AWS. I changed a few unit tests that were using a device named /dev/vdga. Is this ok from your point of view?

pkg/apis/machine/validation/awsmachineclass.go

pkg/apis/machine/validation/awsmachineclass_test.go

rewiko · 2020-02-10T19:31:00Z

LGTM just a few comments

pkg/apis/machine/types.go

pkg/apis/machine/validation/alicloudmachineclass.go

hardikdr

Thanks for the nice PR @guydaichs

pkg/apis/machine/v1alpha1/types.go

pkg/driver/driver_alicloud.go

pkg/driver/driver_azure.go

guydc · 2020-03-30T06:04:37Z

How are the luns managed by the PVC/PV controller in k8s ? Is it possible that, kubernetes allocates the same lun value for a pvc-disk and we set the same lun value for the data-disk here, what will be the behaviour then

@hardikdr - It looks like the attachment process in azure finds an available lun : https://github.com/kubernetes-sigs/azuredisk-csi-driver/blob/master/vendor/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go#L280.

My assumptions are:

MCM will provision and attach data-disks before any PV is attached.
If an existing PV is (re-)attached to a node, an available lun would be selected on-demand.
If the data-disk definition for an azure machine will change, the node would be destroyed and re-provisioned.

Do you think that we need an additional safety mechanism?

hardikdr

Overall looks good to me, I have few minor comments inline, thanks for the much-needed feature.

kubernetes/machine_classes/aws-machine-class.yaml

pkg/apis/machine/validation/alicloudmachineclass.go

pkg/apis/machine/validation/awsmachineclass_test.go

pkg/driver/driver_alicloud.go

hardikdr · 2020-04-07T08:26:39Z

Do you think that we need an additional safety mechanism?

Looks good to me, but you might want to take a call on the suggestion made here by @dkistner , if you feel to make the lun optional, and enumerate from zero if not specified.

CLAassistant · 2020-04-07T09:13:07Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ guydaichs
❌ alexwo
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

hardikdr · 2020-04-07T09:34:40Z

Overall, thanks a lot to @guydaichs for the PR, and thanks to everyone for reviewing it.

I'd suggest following, to further complete the review and go ahead merging it.

@guydaichs Can you please address the remaining review-comments on the PR. Also, Resolve Conversations wherever you feel it's taken care of already. There is also a conflicting file, you might want to consider.

Overall, an apology from my side for the delayed review.

@dkistner @jia-jerry Can you please follow-up (resolve the conversations if taken care of or concern is not valid anymore) and approve the PR from the Azure's and Alicloud's perspective respectively when it's ready. Thanks a lot in advance.

jia-jerry

one minor issue

jia-jerry · 2020-04-20T15:24:39Z

pkg/apis/machine/validation/alicloudmachineclass.go

@@ -63,6 +66,43 @@ func validateAlicloudMachineClassSpec(spec *machine.AlicloudMachineClassSpec, fl
 		allErrs = append(allErrs, field.Required(fldPath.Child("keyPairName"), "KeyPairName is required"))
 	}

+	const dataDiskNameFmt string = `[a-zA-Z0-9\.\-_:]+`


it should starts with a letter. The regexp looks like [a-zA-Z][a-zA-Z0-9\.\-_:]+

dkistner

/lgtm

We will tackle the things with optional luns for Azure later in a subsequent issue and pr.
Thanks @guydaichs :)

jia-jerry

/lgtm

hardikdr · 2020-04-23T05:47:05Z

@ggaurav10 @prashanth26 @rewiko Can you please take one final look at PR, and approve if possible?

rewiko

Nice PR @guydaichs :)

alexwo and others added 2 commits January 29, 2020 20:01

Adding multiple disk attachment support for alibaba cloud

bdd9f0e

(cherry picked from commit 7c658e2911155d7acb49a6f54ade3190e4c76718)

Support Data Disks in AliCloud and Azure

66bebe9

guydc requested review from ggaurav10 and a team as code owners January 29, 2020 18:18

guydc added 2 commits February 2, 2020 19:41

resolved merge conflicts, removed comment, fmt

36b9b74

resolve merge conflicts

ffce209

hardikdr requested a review from dkistner February 4, 2020 18:40

hardikdr added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 6, 2020

gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 6, 2020

prashanth26 added platform/ali labels Feb 6, 2020

ggaurav10 reviewed Feb 6, 2020

View reviewed changes

pkg/apis/machine/validation/awsmachineclass.go Show resolved Hide resolved

pkg/driver/driver_alicloud.go Outdated Show resolved Hide resolved

pkg/driver/driver_azure.go Outdated Show resolved Hide resolved

prashanth26 added area/scalability kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) topology/seed Affects Seed clusters labels Feb 6, 2020

fix review comments, update tests and add validation tests

0df6b24

rewiko reviewed Feb 10, 2020

View reviewed changes

pkg/apis/machine/validation/awsmachineclass.go Outdated Show resolved Hide resolved

rewiko reviewed Feb 10, 2020

View reviewed changes

pkg/apis/machine/validation/awsmachineclass_test.go Show resolved Hide resolved

jia-jerry suggested changes Feb 11, 2020

View reviewed changes

pkg/apis/machine/types.go Outdated Show resolved Hide resolved

jia-jerry reviewed Feb 11, 2020

View reviewed changes

pkg/apis/machine/validation/alicloudmachineclass.go Outdated Show resolved Hide resolved

ghost removed the platform/az label Mar 7, 2020

guydc added 2 commits March 26, 2020 15:38

merge

c0a8e6b

fix review comments

74e5191

hardikdr reviewed Mar 29, 2020

View reviewed changes

pkg/apis/machine/v1alpha1/types.go Show resolved Hide resolved

pkg/apis/machine/v1alpha1/types.go Outdated Show resolved Hide resolved

pkg/driver/driver_alicloud.go Show resolved Hide resolved

pkg/driver/driver_azure.go Outdated Show resolved Hide resolved

hardikdr reviewed Apr 7, 2020

View reviewed changes

guydc added 3 commits April 14, 2020 11:59

merge

cc1477e

fix code review comments

4c011c5

review fixes

8614d2d

jia-jerry suggested changes Apr 20, 2020

View reviewed changes

dkistner approved these changes Apr 22, 2020

View reviewed changes

fix alicloud datadisk regex

ddd71ae

jia-jerry approved these changes Apr 23, 2020

View reviewed changes

fix test

ede44dd

hardikdr added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 23, 2020

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 23, 2020

ggaurav10 approved these changes Apr 23, 2020

View reviewed changes

hardikdr approved these changes Apr 23, 2020

View reviewed changes

rewiko approved these changes Apr 23, 2020

View reviewed changes

prashanth26 approved these changes Apr 26, 2020

View reviewed changes

hardikdr merged commit 1b1a9fa into gardener:master Apr 26, 2020

gardener-robot added the area/auto-scaling Auto-scaling (CA/HPA/VPA/HVPA, predominantly control plane, but also otherwise) related label Jun 5, 2020

prashanth26 mentioned this pull request Aug 13, 2020

Additional (+ encrypted) ephemeral worker/node disks #338

Closed

prashanth26 mentioned this pull request Feb 2, 2021

Reintroduces support for multi volume support for EC2 instances gardener/machine-controller-manager-provider-aws#21

Merged

hardikdr mentioned this pull request Jul 21, 2021

Make lun field optional in Azure provider. gardener/machine-controller-manager-provider-azure#42

Closed

himanshu-kun mentioned this pull request Apr 25, 2023

update vm delete to not wait for datadisk detachment gardener/machine-controller-manager-provider-azure#95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Data Disks in Azure and AliCloud #397

Added support for Data Disks in Azure and AliCloud #397

guydc commented Jan 29, 2020 •

edited by hardikdr

Loading

hardikdr commented Jan 31, 2020

hardikdr commented Feb 6, 2020

hardikdr commented Feb 6, 2020

prashanth26 commented Feb 6, 2020

dkistner commented Feb 7, 2020

guydc commented Feb 10, 2020

rewiko commented Feb 10, 2020

hardikdr left a comment

guydc commented Mar 30, 2020

hardikdr left a comment

hardikdr commented Apr 7, 2020

CLAassistant commented Apr 7, 2020

hardikdr commented Apr 7, 2020 •

edited

Loading

jia-jerry left a comment

jia-jerry Apr 20, 2020

dkistner left a comment

jia-jerry left a comment

hardikdr commented Apr 23, 2020

rewiko left a comment

Added support for Data Disks in Azure and AliCloud #397

Added support for Data Disks in Azure and AliCloud #397

Conversation

guydc commented Jan 29, 2020 • edited by hardikdr Loading

hardikdr commented Jan 31, 2020

hardikdr commented Feb 6, 2020

hardikdr commented Feb 6, 2020

prashanth26 commented Feb 6, 2020

dkistner commented Feb 7, 2020

guydc commented Feb 10, 2020

rewiko commented Feb 10, 2020

hardikdr left a comment

Choose a reason for hiding this comment

guydc commented Mar 30, 2020

hardikdr left a comment

Choose a reason for hiding this comment

hardikdr commented Apr 7, 2020

CLAassistant commented Apr 7, 2020

hardikdr commented Apr 7, 2020 • edited Loading

jia-jerry left a comment

Choose a reason for hiding this comment

jia-jerry Apr 20, 2020

Choose a reason for hiding this comment

dkistner left a comment

Choose a reason for hiding this comment

jia-jerry left a comment

Choose a reason for hiding this comment

hardikdr commented Apr 23, 2020

rewiko left a comment

Choose a reason for hiding this comment

guydc commented Jan 29, 2020 •

edited by hardikdr

Loading

hardikdr commented Apr 7, 2020 •

edited

Loading