-
Notifications
You must be signed in to change notification settings - Fork 514
feat: enable flexible SSD configuration #3872
Changes from all commits
674c165
06e3101
077871f
eac0476
acb67f2
adc3a5e
16950d2
77782b7
adaa772
cee5a5e
ae3bfa4
192a6fa
63fd928
a1b434f
450b092
0b5ed2f
58180a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -518,3 +518,11 @@ const TLSStrongCipherSuitesAPIServer = "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,TLS | |
|
|
||
| // 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment here, this could be StorageAccountTypes and we should consider using the same syntax as Azure (
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Except that the standard Azure names are so human-offending... How does capz expose this menu? |
||
| const ( | ||
| UltraSSD string = "UltraSSD" | ||
| PremiumSSD string = "PremiumSSD" | ||
| StandardSSD string = "StandardSSD" | ||
| StandardHDD string = "StandardHDD" | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,7 @@ func (cs *ContainerService) SetPropertiesDefaults(params PropertiesDefaultsParam | |
|
|
||
| // Set master profile defaults if this cluster configuration includes master node(s) | ||
| if cs.Properties.MasterProfile != nil { | ||
| properties.setMasterProfileDefaults() | ||
| properties.setMasterProfileDefaults(params.IsUpgrade) | ||
| } | ||
|
|
||
| properties.setAgentProfileDefaults(params.IsUpgrade, params.IsScale) | ||
|
|
@@ -389,6 +389,23 @@ func (cs *ContainerService) setOrchestratorDefaults(isUpgrade, isScale bool) { | |
| } | ||
| } | ||
|
|
||
| if !isUpgrade && !isScale { | ||
| if cs.Properties.MasterProfile != nil { | ||
| if cs.Properties.MasterProfile.DataDiskType == UltraSSD { | ||
| if o.KubernetesConfig.EtcdDiskIOPS == 0 || o.KubernetesConfig.EtcdDiskMBPS == 0 { | ||
| etcdSizeGB, _ := strconv.Atoi(o.KubernetesConfig.EtcdDiskSizeGB) | ||
| ultraSSDConfig := getDefaultUltraSSDConfig(etcdSizeGB) | ||
| if o.KubernetesConfig.EtcdDiskIOPS == 0 { | ||
| o.KubernetesConfig.EtcdDiskIOPS = ultraSSDConfig.iops | ||
| } | ||
| if o.KubernetesConfig.EtcdDiskMBPS == 0 { | ||
| o.KubernetesConfig.EtcdDiskMBPS = ultraSSDConfig.mbps | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if a.OrchestratorProfile.KubernetesConfig.EtcdStorageLimitGB == 0 { | ||
| a.OrchestratorProfile.KubernetesConfig.EtcdStorageLimitGB = DefaultEtcdStorageLimitGB | ||
| } | ||
|
|
@@ -657,7 +674,7 @@ func (p *Properties) setExtensionDefaults() { | |
| } | ||
| } | ||
|
|
||
| func (p *Properties) setMasterProfileDefaults() { | ||
| func (p *Properties) setMasterProfileDefaults(isUpgrade bool) { | ||
| // set default to VMAS for now | ||
| if p.MasterProfile.AvailabilityProfile == "" { | ||
| p.MasterProfile.AvailabilityProfile = AvailabilitySet | ||
|
|
@@ -695,6 +712,20 @@ func (p *Properties) setMasterProfileDefaults() { | |
| if p.MasterProfile.OSDiskCachingType == "" { | ||
| p.MasterProfile.OSDiskCachingType = string(compute.CachingTypesReadWrite) | ||
| } | ||
|
|
||
| if !isUpgrade { | ||
| if p.MasterProfile.DataDiskType == "" { | ||
| p.MasterProfile.DataDiskType = PremiumSSD | ||
| } | ||
| if p.MasterProfile.DataDiskType == UltraSSD { | ||
| if p.MasterProfile.UltraSSDEnabled == nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That will be disallowed in validation (// TODO) |
||
| p.MasterProfile.UltraSSDEnabled = to.BoolPtr(true) | ||
| } | ||
| } | ||
| if p.MasterProfile.OSDiskType == "" { | ||
| p.MasterProfile.OSDiskType = PremiumSSD | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func (p *Properties) setAgentProfileDefaults(isUpgrade, isScale bool) { | ||
|
|
@@ -764,13 +795,19 @@ func (p *Properties) setAgentProfileDefaults(isUpgrade, isScale bool) { | |
| profile.EnableVMSSNodePublicIP = to.BoolPtr(DefaultEnableVMSSNodePublicIP) | ||
| } | ||
|
|
||
| if profile.OSDiskCachingType == "" { | ||
| if profile.IsEphemeral() { | ||
| if profile.IsEphemeral() { | ||
| if profile.OSDiskCachingType == "" { | ||
| profile.OSDiskCachingType = string(compute.CachingTypesReadOnly) | ||
| } else { | ||
| profile.OSDiskCachingType = string(compute.CachingTypesReadWrite) | ||
| } | ||
| if profile.OSDiskType == "" { | ||
| profile.OSDiskType = StandardHDD | ||
| } | ||
| } | ||
|
|
||
| if profile.OSDiskCachingType == "" { | ||
| profile.OSDiskCachingType = string(compute.CachingTypesReadWrite) | ||
| } | ||
|
|
||
| if profile.DataDiskCachingType == "" { | ||
| profile.DataDiskCachingType = string(compute.CachingTypesReadOnly) | ||
| } | ||
|
|
@@ -1167,3 +1204,65 @@ func (cs *ContainerService) setCSIProxyDefaults() { | |
| } | ||
| } | ||
| } | ||
|
|
||
| type ssdConfig struct { | ||
| iops int | ||
| mbps int | ||
| } | ||
|
|
||
| // getDefaultUltraSSDConfig returns a known-working IOPS + MBPS config based on the size of the etcd disk | ||
| // See https://docs.microsoft.com/en-us/azure/virtual-machines/disks-types#ultra-disk | ||
| func getDefaultUltraSSDConfig(etcdDiskSizeGB int) ssdConfig { | ||
| if etcdDiskSizeGB >= 1024 { | ||
| return ssdConfig{ | ||
| iops: 160000, | ||
| mbps: 2000, | ||
| } | ||
| } | ||
| if etcdDiskSizeGB >= 512 { | ||
| return ssdConfig{ | ||
| iops: 80000, | ||
| mbps: 2000, | ||
| } | ||
| } | ||
| if etcdDiskSizeGB >= 256 { | ||
| return ssdConfig{ | ||
| iops: 76800, | ||
| mbps: 2000, | ||
| } | ||
| } | ||
| if etcdDiskSizeGB >= 128 { | ||
| return ssdConfig{ | ||
| iops: 38400, | ||
| mbps: 2000, | ||
| } | ||
| } | ||
| if etcdDiskSizeGB >= 64 { | ||
| return ssdConfig{ | ||
| iops: 19200, | ||
| mbps: 2000, | ||
| } | ||
| } | ||
| if etcdDiskSizeGB >= 32 { | ||
| return ssdConfig{ | ||
| iops: 9600, | ||
| mbps: 2000, | ||
| } | ||
| } | ||
| if etcdDiskSizeGB >= 16 { | ||
| return ssdConfig{ | ||
| iops: 4800, | ||
| mbps: 1200, | ||
| } | ||
| } | ||
| if etcdDiskSizeGB >= 8 { | ||
| return ssdConfig{ | ||
| iops: 2400, | ||
| mbps: 600, | ||
| } | ||
| } | ||
| return ssdConfig{ | ||
| iops: 1200, | ||
| mbps: 300, | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.