Add support for Ultra Disks as Persistent Volumes

[damdo]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Following the design conversations held on #1852
we are introducing a new AdditionalCapabilities field in the Machine spec which will allow us to specify these types of capabilities on Azure Machines.
More specifically, this change is introduced to allow users to specify the UltraSSDEnabled Additional Capability, which will instruct Azure to allow or disallow the use of Ultra Disks with an host instance. More on this on the linked issue.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1852

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

Add support for Ultra Disks as Persistent Volumes

[k8s-ci-robot]

Welcome @damdo!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @damdo. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jackfrancis]

So I see that the Azure SDK type spec defines this AdditionalCapabilities struct to contain the UltraSSD config... I wonder if we want to expose that to capz users, or simply add UltraSSDEnabled to the existing flat configuration property structure in AzureMachineSpec.

Thoughts @CecileRobertMichon @mboersma

[jackfrancis]

lol I see this was discussed at length in the linked issue

[damdo]

Yeah :) we agreed with @CecileRobertMichon on going with this design.

[jackfrancis]

Thanks for this @damdo!

[jackfrancis]

/ok-to-test

[jackfrancis]

@damdo can you do the following:

1. Add this new configuration to the base template that we use to test by adding to the AzureMachineTemplate under templates/flavors/default/machine-deployment.yaml?
2. Run hack/gen-flavors.sh to render the changes across all templates that re-use that base flavor partial

This will allow us to get E2E test signal on this PR before it merges, and to better ensure its functionality going forward.

Thanks!

[CecileRobertMichon]

Add this new configuration to the base template

@jackfrancis the base template is published as a reference template for users and is the default flavor that clusterctl uses to create a CAPZ cluster, we shouldn't turn on arbitrary features unless they are blessed "best practice" configurations that we recommend users turn on by default. This doesn't apply to ultra disks, especially since Ultra disk support is quite limited it won't work for every region/zone/VM size.

If we want to test it in e2e, we should add it to one of the test templates as a kustomize patch.

[JoelSpeed]

Thanks for putting this together @damdo, looks good, just one nit in the documentation I found

[jackfrancis]

@damdo I tried to test this manually and got this error:

E0627 10:02:14.628416      45 controller.go:317] controller/azuremachine "msg"="Reconciler error" "error"="failed to reconcile AzureMachine: failed to reconcile AzureMachine service virtualmachine: failed to create resource default-11551/default-11551-control-plane-wbsz8 (service: virtualmachine): compute.VirtualMachinesClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code=\"InvalidParameter\" Message=\"Storage Account type UltraSSD_LRS is not supported with caching type ReadWrite. Please use caching type None.\" Target=\"dataDisk.caching\"" "name"="default-11551-control-plane-wbsz8" "namespace"="default" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="AzureMachine" 

We'll need to automatically set the dataDisk.cachingType value to None if "UltraSSD_LRS" is the chosen managedDisk.storageAccountType value and the cachingType val is empty. And, if it's not empty and != None we'll need to throw a webhook validation error.

[jackfrancis]

@damdo I confirmed that the following template addition to a dataDisk spec works in this PR:

        - nameSuffix: mydata
          diskSizeGB: 256
          lun: 1
          managedDisk:
            storageAccountType: "UltraSSD_LRS"
          cachingType: None

So let's get the cachingType foo incorporated (including mentioning that in docs) and I think we're good to go. Thanks so much!

[jackfrancis]

@CecileRobertMichon I cross-referenced your docs with the regions we test and all of our testing regions support zones + ultrassd. After this PR merges I'll start testing a change where we confiure our etcd data disk to use ultrassd in reference templates. I think that is in fact a "best practice" configuration.

[damdo]

Hey @jackfrancis, thanks for manually testing this.

If I'm understanding the error correctly here, this is strictly related to specifying a Data Disk of type UltraSSD_LRS when also choosing a cachingType other than None.

I think is something not strictly related or introduced in this PR, even though part of the same overall big feature (Ultra Disks).
I suspect the issue you are talking about would be reproducible even without this PR as that'd be in the domain of the: "Ultra disks as data disks" feature, rather than the "Ultra Disks as Persistent Volumes" feature (which this PR introduces).

That said I completely agree with your suggestions for fixing this.
So my only ask is whether we should instead do it in another PR to be merged before this one, to treat what you are flagging as a bug of an existing behaviour, and merge this as an additional feature after that.

Thanks!

[damdo]

/test pull-cluster-api-provider-azure-e2e

[damdo]

@jackfrancis I've created a bug issue #2429 to track that cachingType validation issue and opened a pr #2430 to fix it.

[jackfrancis]

@damdo sgtm!

thanks for the add'l PR!

[jackfrancis]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jackfrancis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment