feat: enable flexible SSD configuration #3872

jackfrancis · 2020-09-25T21:45:25Z

Reason for Change:

This PR addresses remaining gaps in enabling SSD flavors across OS and data disks.

Will fix #2243

Issue Fixed:

Credit Where Due:

Does this change contain code from or inspired by another project?

No
Yes

If "Yes," did you notify that project's maintainers and provide attribution?

No
Yes

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

acs-bot · 2020-09-25T21:48:53Z

[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

CecileRobertMichon · 2020-09-30T21:32:57Z

pkg/engine/virtualmachines.go

+		case api.Premium:
+			ssdType = compute.PremiumLRS
+		case api.Standard:
+			ssdType = compute.StandardLRS


Standard SSD should actually be StandardSSDLRS . StandardLRS is standard HDD:

aks-engine/vendor/github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-12-01/compute/models.go

Line 378 in b3c4f9e

StandardSSDLRS DiskStorageAccountTypes = "StandardSSD_LRS"

CecileRobertMichon · 2020-09-30T21:34:34Z

examples/e2e-tests/kubernetes/release/default/definition.json

        "osDiskCachingType": "ReadOnly",
-        "dataDiskCachingType": "ReadWrite"
+        "dataDiskCachingType": "ReadWrite",
+        "osDiskSSDType": "Standard",


instead of SSD type, have you considered calling this osDiskStorageAccountType? I would 1) be consistent with other usages across Azure and 2) allow for Standard_HDD and any other non-SSD types that may be added in the future

How about just osDiskType and dataDiskType?

Just reading this doc, that would be an accurate concise way to represent this with a property name:

https://docs.microsoft.com/en-us/azure/virtual-machines/disks-types

Possible values:

"StandardHDD"

"StandardSSD"

"PremiumSSD"

"UltraSSD"

Only caveat, you might want different types per Data disk so having one global dataDiskType might not be the most flexible

I think that inflexibility is already built into the AKS Engine config interface, as the nominal data disk config interface is simply an array of disk sizes; so the assumption is that they are all otherwise (besides size) similarly configured.

Definitely a gotcha for capz, tho

CecileRobertMichon · 2020-09-30T21:35:59Z

pkg/api/const.go

 // TLSStrongCipherSuitesKubelet is a kube-bench-recommended allowed cipher suites for kubelet
 const TLSStrongCipherSuitesKubelet = "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_RSA_WITH_AES_256_GCM_SHA384,TLS_RSA_WITH_AES_128_GCM_SHA256"
+
+// SSD Types


same comment here, this could be StorageAccountTypes and we should consider using the same syntax as Azure (Standard_LRS, StandardSSD_LRS, Premium_LRS etc)

Except that the standard Azure names are so human-offending... How does capz expose this menu?

CecileRobertMichon · 2020-09-30T21:36:48Z

pkg/api/defaults.go

+			p.MasterProfile.DataDiskSSDType = Premium
+		}
+		if p.MasterProfile.DataDiskSSDType == Ultra {
+			if p.MasterProfile.UltraSSDEnabled == nil {


what happens if UltraSSDEnabled is false and DataDiskSSDType is Ultra?

That will be disallowed in validation (// TODO)

CecileRobertMichon · 2020-09-30T21:38:49Z

test/e2e/test_cluster_configs/everything.json

 					"2"
-				]
+				],
+				"ultraSSDEnabled": true


what about regions that don't support this yet? is it guaranteed that e2e won't run in those regions?

The test config interface supports specific regions, so yeah, we'll need to make sure it is set to only include regions that support ultra ssd for cluster configs that are testing ultra ssd (like this one)

CecileRobertMichon · 2020-09-30T21:42:48Z

examples/e2e-tests/kubernetes/release/default/definition.json

+        "vnetSubnetId": "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME",
+        "availabilityZones": ["1", "2"],
+        "osDiskSSDType": "Standard",
+        "ultraSSDEnabled": true


I think we should consider deprecating this altogether since it's redundant information and simply assume that if dataDisk type is ultraSSD it should be enabled on the VM. Is there a use case for enabling it but not using it for any disks?

Well, it's complicated, as it is in fact a discrete configuration for the VM (arguably, unfortunately). This configuration tells Azure to "get me a VM that is capable of attaching to an Ultra SSD disk resource". There's a different configuration to actually connect an Ultra SSD disk.

Is there a use-case for getting a ultra ssd-enabled VM, but not actually using ultra ssd for the k8s IaaS bootstrapped by AKS Engine? I don't know... I think not knowing definitively that that will never make sense combined with the fact that that config property already exists... so yeah, just working with it, so to speak.

stale · 2020-11-16T03:44:57Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

pierluigilenoci · 2020-11-16T08:25:48Z

@jackfrancis could you please take care of this PR?

codecov · 2020-12-15T21:35:26Z

Codecov Report

Merging #3872 (58180a9) into master (ad20028) will increase coverage by 0.00%.
The diff coverage is 74.46%.

@@           Coverage Diff            @@
##           master    #3872    +/-   ##
========================================
  Coverage   73.27%   73.27%            
========================================
  Files         135      135            
  Lines       20720    20883   +163     
========================================
+ Hits        15183    15303   +120     
- Misses       4562     4600    +38     
- Partials      975      980     +5

Impacted Files	Coverage Δ
pkg/api/types.go	`92.72% <ø> (ø)`
pkg/api/vlabs/types.go	`73.04% <ø> (ø)`
pkg/engine/armtype.go	`85.00% <ø> (ø)`
pkg/engine/virtualmachinescalesets.go	`77.91% <15.38%> (-2.81%)`	⬇️
pkg/engine/virtualmachines.go	`80.46% <66.23%> (-2.33%)`	⬇️
pkg/api/converterfromapi.go	`95.71% <100.00%> (+0.03%)`	⬆️
pkg/api/convertertoapi.go	`94.07% <100.00%> (+0.04%)`	⬆️
pkg/api/defaults.go	`93.97% <100.00%> (+0.53%)`	⬆️
pkg/api/vlabs/validate.go	`81.90% <100.00%> (+0.05%)`	⬆️
pkg/engine/masterarmresources.go	`68.88% <100.00%> (+0.70%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83b3b57...58180a9. Read the comment docs.

jackfrancis · 2021-01-20T19:28:54Z

/hold

This is stalled due to backwards incompatibility (e.g., aks-engine upgrade)

stale · 2021-06-11T01:44:20Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

pierluigilenoci · 2021-06-11T07:45:15Z

@jackfrancis any update on this?

bridgetkromhout · 2022-03-16T22:48:57Z

Closing per @jackfrancis, given AKS Engine deprecation.

acs-bot added do-not-merge/work-in-progress size/M labels Sep 25, 2020

acs-bot added the approved label Sep 25, 2020

jackfrancis changed the title ~~[WIP] feat: enable flexible SSL configuration~~ [WIP] feat: enable flexible SSD configuration Sep 28, 2020

acs-bot added size/L and removed size/M labels Sep 28, 2020

jackfrancis force-pushed the ssd-rationalize branch from 72328e8 to 4ae5e0b Compare September 30, 2020 17:06

CecileRobertMichon reviewed Sep 30, 2020

View reviewed changes

jackfrancis force-pushed the ssd-rationalize branch from 0f7ccbf to 31147a4 Compare September 30, 2020 21:40

CecileRobertMichon reviewed Sep 30, 2020

View reviewed changes

acs-bot added size/XL and removed size/L labels Oct 1, 2020

jackfrancis force-pushed the ssd-rationalize branch from 066a2f1 to 4db0dd8 Compare October 12, 2020 19:54

jackfrancis mentioned this pull request Oct 29, 2020

I'm not able to create a cluster using Ultra SSD #2243

Closed

stale bot added the stale label Nov 16, 2020

stale bot removed the stale label Nov 16, 2020

jackfrancis force-pushed the ssd-rationalize branch from 4db0dd8 to 8433c51 Compare December 15, 2020 17:45

jackfrancis changed the title ~~[WIP] feat: enable flexible SSD configuration~~ feat: enable flexible SSD configuration Dec 15, 2020

jackfrancis force-pushed the ssd-rationalize branch from 01ef3ac to d0b787a Compare December 15, 2020 21:21

jackfrancis force-pushed the ssd-rationalize branch from 5804a39 to 8152ff4 Compare January 6, 2021 17:47

jackfrancis added 2 commits January 20, 2021 10:28

feat: enable flexible SSL configuration

674c165

update test api models

06e3101

jackfrancis added 15 commits January 20, 2021 10:28

move to func

077871f

defaults

eac0476

on the way

acb67f2

more defaults

adc3a5e

update test configs

16950d2

JSON formatting

77782b7

feedback

adaa772

test configs, and rationalize ephemeral

cee5a5e

update to osDiskType and dataDiskType

ae3bfa4

add isUpgrade to setMasterProfileDefaults

192a6fa

update UT

63fd928

additive UT

a1b434f

enable no-vnet E2E config as well

450b092

update test configs

0b5ed2f

add docs reference for iops/mbps dictionary

58180a9

jackfrancis force-pushed the ssd-rationalize branch from 8152ff4 to 58180a9 Compare January 20, 2021 18:28

CecileRobertMichon mentioned this pull request Jan 21, 2021

Add support for Ultra Disks for data disks kubernetes-sigs/cluster-api-provider-azure#1134

Closed

stale bot added the stale label Jun 11, 2021

stale bot removed the stale label Jun 11, 2021

bridgetkromhout closed this Mar 16, 2022

feat: enable flexible SSD configuration #3872

feat: enable flexible SSD configuration #3872

Uh oh!

Conversation

jackfrancis commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acs-bot commented Sep 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Nov 16, 2020

Uh oh!

pierluigilenoci commented Nov 16, 2020

Uh oh!

codecov bot commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jackfrancis commented Jan 20, 2021

Uh oh!

stale bot commented Jun 11, 2021

Uh oh!

pierluigilenoci commented Jun 11, 2021

Uh oh!

bridgetkromhout commented Mar 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jackfrancis commented Sep 25, 2020 •

edited

Loading

codecov bot commented Dec 15, 2020 •

edited

Loading