Bug 1836337: Azure: Add functionality to change Azure Machine Disk Types #3520

rna-afk · 2020-04-28T19:59:04Z

Azure disk types for the master and worker nodes is by default
set to Premium SSD(Premium_LRS) setting. Made the changes to add
the disktype of the master and worker nodes in the install config
by adding an extra field in MachinePool DiskType to the OSDisk struct
which is part of the DefaultMachinePlatform attribute in the Azure
Platform type. To change the disk type, the install config must be
changed by adding the diskType under platform->Azure property by adding
the defaultMachinePlatform->osDisk->diskType and setting the value to
one of the following, Premium_LRS, Standard_LRS and StandardSSD_LRS.

Setting Standard_LRS is not recommended for the master nodes and hence,
if the disktype is set to it then the Master node is defaulted to Premium_LRS
and the worker nodes will take up the Standard_LRS.

Also, DefaultMachinePlatform can only have both Premium_LRS or
StandardSSD_LRS for the above reason.

Currently, the UltraSSD_LRS is under construction as the terraform version
needs to be updated.

patrickdillon · 2020-04-28T20:13:50Z

/test e2e-azure

patrickdillon · 2020-04-28T20:14:13Z

This PR should update docs as well.

rna-afk · 2020-04-28T21:14:27Z

/test e2e-azure

rna-afk · 2020-04-28T23:38:32Z

/test images

patrickdillon

Validation pieces needs to be moved to install config validation.

patrickdillon · 2020-04-29T01:35:07Z

docs/user/azure/customization.md

@@ -18,6 +18,7 @@ The following options are available when using Azure:

 * `osDisk` (optional object):
    * `diskSizeGB` (optional integer): The size of the disk in gigabytes (GB).
+    * `diskType` (optional string): The type of the disk. (Values allowed are Premium_LRS, Standard_LRS, StandardSSD_LRS)


Suggested change

* `diskType` (optional string): The type of the disk. (Values allowed are Premium_LRS, Standard_LRS, StandardSSD_LRS)

* `diskType` (optional string): The type of disk (allowed values are: `Premium_LRS`, `Standard_LRS`, and `StandardSSD_LRS`).

patrickdillon · 2020-04-29T01:38:24Z

pkg/asset/machines/master.go

@@ -263,6 +263,11 @@ func (m *Master) Generate(dependencies asset.Parents) error {
 		}
 		pool.Platform.Azure = &mpool

+		if mpool.OSDisk.DiskType == "Standard_LRS" {


This doesn't seem like the right place for validation. Shouldn't this be validated in the install config?

This validation is only for the master. The worker will still take up the value. Since the value for the master alone cannot be HDD but it can be for the workers and since there is only one diskType for both master and worker, I thought placing it here would minimize code.

See #3520 (comment)

The code in this file is for creating machine assets. The code in the link above is where we validate install configs

patrickdillon · 2020-04-29T01:40:02Z

pkg/asset/machines/master.go

@@ -263,6 +263,11 @@ func (m *Master) Generate(dependencies asset.Parents) error {
 		}
 		pool.Platform.Azure = &mpool

+		if mpool.OSDisk.DiskType == "Standard_LRS" {
+			logrus.Warnf("%s disktype not recommended for master, switching to Premium_LRS", mpool.OSDisk.DiskType)
+			mpool.OSDisk.DiskType = "Premium_LRS"


I think it's better to error out and let users determine course of action.

patrickdillon · 2020-04-29T01:54:15Z

pkg/types/azure/machinepool.go

@@ -38,4 +44,18 @@ func (a *MachinePool) Set(required *MachinePool) {
 	if required.OSDisk.DiskSizeGB != 0 {
 		a.OSDisk.DiskSizeGB = required.OSDisk.DiskSizeGB
 	}
+
+	a.OSDisk.DiskType = validateDiskType(required.OSDisk.DiskType)


validation should not be performed here. Check out https://github.com/openshift/installer/blob/master/pkg/types/validation/installconfig.go#L302-L331 but I think some modification may be necessary.

If we were to error out if invalid value is given then I think this code should be in here since it's azure specific.

installer/pkg/types/azure/validation/machinepool.go

Lines 9 to 16 in 6f9a0b8

func ValidateMachinePool(p *azure.MachinePool, fldPath *field.Path) field.ErrorList {

allErrs := field.ErrorList{}

if p.OSDisk.DiskSizeGB < 0 {

allErrs = append(allErrs, field.Invalid(fldPath.Child("diskSizeGB"), p.OSDisk.DiskSizeGB, "Storage DiskSizeGB must be positive"))

}

return allErrs

}

Making the changes.

patrickdillon · 2020-04-29T02:02:56Z

pkg/types/azure/machinepool.go

+
+	a.OSDisk.DiskType = validateDiskType(required.OSDisk.DiskType)
+}
+func validateDiskType(diskType string) string {


validating and setting defaults are two different functions

Couldn't find an appropriate function name for this. It validates the value and sets the default when there is empty or invalid value.

patrickdillon · 2020-04-29T02:06:33Z

pkg/types/azure/machinepool.go

+	if diskType == "" {
+		return "Premium_LRS"
+	}
+	_, found := map[string]string{"Standard_LRS": "", "StandardSSD_LRS": "", "Premium_LRS": ""}[ //"UltraSSD_LRS": "",


This is hard to read. UltraSSD_LRS even if commented out should not be within the indexing operator.

gofmt formatted it that way when I tried to create a new line and comment it. I think I'll move it to the top of this line.

rna-afk · 2020-04-30T17:26:20Z

With the new commit, I have kind of fixed the top level Machine Pool validation that the master workers in Azure must not use the Standard HDD disks but the top level Platform where the defaultMachinePlatform value is not fixed specifically for the master machines.

abhinavdahiya · 2020-04-30T18:30:55Z

pkg/types/azure/validation/machinepool.go

+		//"UltraSSD_LRS": "",
+		_, found := map[string]string{"Standard_LRS": "", "StandardSSD_LRS": "", "Premium_LRS": ""}[p.OSDisk.DiskType]


nit:
keep a list of known valid values and then run sets.NewString(knownDiskTypes...).Has(p.OSDisk.DiskType)

that helps with readability as a constant list is easy to track and maintain outside the function.

abhinavdahiya · 2020-04-30T18:32:51Z

pkg/types/azure/validation/machinepool.go

+		//"UltraSSD_LRS": "",
+		_, found := map[string]string{"Standard_LRS": "", "StandardSSD_LRS": "", "Premium_LRS": ""}[p.OSDisk.DiskType]
+		if !found {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("diskType"), p.OSDisk.DiskType, "Storage DiskType value invalid"))


include the list of supported values in the message.

Also i think there is https://www.godoc.org/k8s.io/apimachinery/pkg/util/validation/field#NotSupported

abhinavdahiya · 2020-04-30T18:38:26Z

pkg/types/validation/installconfig.go

+	if pool.Platform.Azure != nil && pool.Name == masterPoolName && pool.Platform.Azure.OSDisk.DiskType == "Standard_LRS" {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("diskType"), pool.Platform.Azure.OSDisk.DiskType, "DiskType not compatible with master machines."))
+	}


i think this is redundant, as you already have https://github.com/openshift/installer/pull/3520/files#diff-e8044151bf8d8c8e84c04c49911da440R64-R66

abhinavdahiya · 2020-04-30T18:40:24Z

pkg/types/validation/installconfig.go

@@ -355,6 +358,9 @@ func validatePlatform(platform *types.Platform, fldPath *field.Path, openStackVa
 		validate(azure.Name, platform.Azure, func(f *field.Path) field.ErrorList {
 			return azurevalidation.ValidatePlatform(platform.Azure, c.Publish, f)
 		})
+		if platform.Azure.DefaultMachinePlatform != nil && platform.Azure.DefaultMachinePlatform.OSDisk.DiskType == "Standard_LRS" {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child(azure.Name), platform.Azure.DefaultMachinePlatform.OSDisk.DiskType, "Disk type \"Standard_LRS\" not recommended as default disk for machines."))


nit, don't use .
also try not to use \", you can use `string with "quotes" here`

abhinavdahiya · 2020-04-30T18:42:04Z

pkg/types/validation/machinepools.go

@@ -61,6 +61,9 @@ func ValidateMachinePool(platform *types.Platform, p *types.MachinePool, fldPath
 	if !validArchitectures[p.Architecture] {
 		allErrs = append(allErrs, field.NotSupported(fldPath.Child("architecture"), p.Architecture, validArchitectureValues))
 	}
+	if p.Platform.Azure != nil && p.Name == masterPoolName && p.Platform.Azure.OSDisk.DiskType == "Standard_LRS" {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("diskType"), p.Platform.Azure.OSDisk.DiskType, "DiskType not compatible with master machines."))


nit : don;'t have the fieldName in the error message too, it's already part of fldPath.Child("diskType")

something like Standard_LRS is not compatible with control plane machines

abhinavdahiya · 2020-04-30T18:43:03Z

docs/user/azure/customization.md

@@ -65,6 +66,7 @@ controlPlane:
      type: Standard_DS4_v2
      osDisk:
        diskSizeGB: 512
+        diskType: Standard_LRS


this is not a valid setting with your validations :P

abhinavdahiya · 2020-04-30T18:49:08Z

since it's still in WIP.

I think we have to allow users to set UltraSSD_LRS
I think you can add an API validation in pkg/asset/installconfig/azure/validation.go
such that you can use the https://godoc.org/github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-10-01/compute#ResourceSkusClient.List
to check if that disk type is available in the location where the cluster is being used.
https://docs.microsoft.com/en-us/azure/virtual-machines/windows/disks-enable-ultra-ssd#vms-with-no-redundancy-options

That would be very helpful imo.

patrickdillon

I requested some changes. I think the biggest issue is we don't know how to handle defaultMachinePool. Master machines cannot have Standard_LRS. So would the ideal way to handle validating the defaultMachinePool be to not allow Standard_LRS for the default pool unless a control plane machine pool was provided?

There are certainly some challenges to achieving this. For one, defaultMachinePools do not seem to have a predictable name. We might be able to check based on name is not worker or master... This might also require some not insignificant replumbing of machinepool validation.

patrickdillon · 2020-04-30T18:14:22Z

pkg/types/azure/validation/machinepool.go

+		//"UltraSSD_LRS": "",
+		_, found := map[string]string{"Standard_LRS": "", "StandardSSD_LRS": "", "Premium_LRS": ""}[p.OSDisk.DiskType]
+		if !found {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("diskType"), p.OSDisk.DiskType, "Storage DiskType value invalid"))
+		}


What about something like:

diskTypes := map[string]string{ "Standard_LRS": "", "StandardSSD_LRS": "", "Premium_LRS": "", //"UltraSSD_LRS": "", // currently in preview } if _, found := diskTypes[p.OSDISK.DiskType]; !found { allErrs = append(allErrs, field.Invalid(fldPath.Child("diskType"), p.OSDisk.DiskType, "Storage DiskType value invalid")) }

patrickdillon · 2020-04-30T18:17:28Z

pkg/types/validation/installconfig.go

@@ -307,6 +307,9 @@ func validateControlPlane(platform *types.Platform, pool *types.MachinePool, fld
 	if pool.Replicas != nil && *pool.Replicas == 0 {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), pool.Replicas, "number of control plane replicas must be positive"))
 	}
+	if pool.Platform.Azure != nil && pool.Name == masterPoolName && pool.Platform.Azure.OSDisk.DiskType == "Standard_LRS" {


All Azure-specific validation should happen in https://github.com/openshift/installer/blob/master/pkg/types/azure/validation/machinepool.go

If you need variables that are not available, you should plumb them.

patrickdillon · 2020-04-30T18:21:34Z

pkg/types/validation/machinepools.go

@@ -61,6 +61,9 @@ func ValidateMachinePool(platform *types.Platform, p *types.MachinePool, fldPath
 	if !validArchitectures[p.Architecture] {
 		allErrs = append(allErrs, field.NotSupported(fldPath.Child("architecture"), p.Architecture, validArchitectureValues))
 	}
+	if p.Platform.Azure != nil && p.Name == masterPoolName && p.Platform.Azure.OSDisk.DiskType == "Standard_LRS" {


Isn't this the same check as https://github.com/openshift/installer/pull/3520/files#diff-02200a3eac2ac223b4117e690cf2d0c7R310?

As with the comment for that line, this should be consolidated into Azure-specific validation.

patrickdillon · 2020-04-30T18:25:47Z

pkg/types/validation/installconfig.go

@@ -355,6 +358,9 @@ func validatePlatform(platform *types.Platform, fldPath *field.Path, openStackVa
 		validate(azure.Name, platform.Azure, func(f *field.Path) field.ErrorList {
 			return azurevalidation.ValidatePlatform(platform.Azure, c.Publish, f)
 		})
+		if platform.Azure.DefaultMachinePlatform != nil && platform.Azure.DefaultMachinePlatform.OSDisk.DiskType == "Standard_LRS" {


All Azure-specific validation should be the in the azure validation package.

abhinavdahiya · 2020-04-30T19:35:06Z

I requested some changes. I think the biggest issue is we don't know how to handle defaultMachinePool. Master machines cannot have Standard_LRS. So would the ideal way to handle validating the defaultMachinePool be to not allow Standard_LRS for the default pool unless a
control plane machine pool was provided?

i think it is okay to validate where the information is available completely, if the definition of check is platform specific we can start with keeping it in general function, and later move it to platform specific location when there is more need.

for now, we could also just move all the check for control-plane disk to asset/machines/master.go to keep this simpler.

patrickdillon · 2020-04-30T20:05:24Z

pkg/types/validation/machinepools.go

@@ -61,6 +61,9 @@ func ValidateMachinePool(platform *types.Platform, p *types.MachinePool, fldPath
 	if !validArchitectures[p.Architecture] {
 		allErrs = append(allErrs, field.NotSupported(fldPath.Child("architecture"), p.Architecture, validArchitectureValues))
 	}
+	if p.Platform.Azure != nil && p.Name == masterPoolName && p.Platform.Azure.OSDisk.DiskType == "Standard_LRS" {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("diskType"), p.Platform.Azure.OSDisk.DiskType, "DiskType not compatible with master machines."))


master machines -> control plane
Since this is validating the default machine pool perhaps the error message could point the user that this is a valid selection for the control plane machine pool

rna-afk · 2020-05-01T17:21:59Z

Fixed most of it (I think). Added validation checks for UltraSSD support but could not create a disk with it because the support for it in Azure terraform is available in version 2.0. There are some deprecation issues that need to be solved if terraform is to be updated. An example of deprecation is

Warning: "resource_group_name": [DEPRECATED] This field has been deprecated and is no longer used - will be removed in 2.0 of the Azure Provider (during normal cluster creation)

rna-afk · 2020-05-05T15:24:29Z

Will wait for #3526 to complete and then work on the changes for the Ultra SSD in a separate pull request. Pushing code with inclusion of other disk type support.

abhinavdahiya · 2020-05-18T18:11:35Z

this is missing https://github.com/openshift/installer/pull/3595/files#diff-c2be8487621063db19a181fa83200aa3 like updates to customization.md @rna-afk

abhinavdahiya · 2020-05-18T20:25:04Z

/test e2e-azure

abhinavdahiya · 2020-05-18T23:26:17Z

/retest

abhinavdahiya · 2020-05-19T04:13:34Z

/lgtm

/retest

openshift-bot · 2020-05-19T04:34:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T05:26:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T05:52:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T06:31:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T07:23:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T07:36:34Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T07:49:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T08:02:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T08:15:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T08:28:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T08:41:35Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T08:54:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T09:10:33Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T09:33:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-19T09:46:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-05-19T09:47:00Z

@rna-afk: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-azure	`22db280`	link	`/test e2e-azure`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

openshift-bot · 2020-05-19T10:00:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-05-19T10:05:25Z

@rna-afk: All pull requests linked via external trackers have merged: openshift/installer#3520. Bugzilla bug 1836337 has been moved to the MODIFIED state.

In response to this:

Bug 1836337: Azure: Add functionality to change Azure Machine Disk Types

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.

rna-afk requested review from abhinavdahiya and patrickdillon April 28, 2020 19:59

openshift-ci-robot requested a review from jcpowermac April 28, 2020 19:59

rna-afk force-pushed the azure-disktype-change branch from d571086 to ee4ab7f Compare April 28, 2020 20:20

patrickdillon requested changes Apr 29, 2020

View reviewed changes

rna-afk changed the title ~~Azure: Add functionality to change Azure Machine Disk Types~~ [WIP] Azure: Add functionality to change Azure Machine Disk Types Apr 29, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2020

rna-afk force-pushed the azure-disktype-change branch from ee4ab7f to f59a394 Compare April 30, 2020 17:23

rna-afk force-pushed the azure-disktype-change branch from f59a394 to f5f2b3b Compare April 30, 2020 18:17

abhinavdahiya reviewed Apr 30, 2020

View reviewed changes

patrickdillon reviewed Apr 30, 2020

View reviewed changes

rna-afk force-pushed the azure-disktype-change branch from f5f2b3b to 5b86cb3 Compare May 1, 2020 17:18

rna-afk force-pushed the azure-disktype-change branch 2 times, most recently from 64f6653 to 01affa6 Compare May 4, 2020 12:57

rna-afk changed the title ~~[WIP] Azure: Add functionality to change Azure Machine Disk Types~~ Azure: Add functionality to change Azure Machine Disk Types May 5, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2020

rna-afk force-pushed the azure-disktype-change branch 2 times, most recently from 4cc16bd to 4072b43 Compare May 18, 2020 19:08

Merge branch 'master' into azure-disktype-change

22db280

rna-afk force-pushed the azure-disktype-change branch from 4072b43 to 22db280 Compare May 18, 2020 19:23

openshift-ci-robot assigned abhinavdahiya May 19, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2020

openshift-merge-robot merged commit 8303d48 into openshift:master May 19, 2020

	* `diskType` (optional string): The type of the disk. (Values allowed are Premium_LRS, Standard_LRS, StandardSSD_LRS)
	* `diskType` (optional string): The type of disk (allowed values are: `Premium_LRS`, `Standard_LRS`, and `StandardSSD_LRS`).

	func ValidateMachinePool(p azure.MachinePool, fldPath field.Path) field.ErrorList {
	allErrs := field.ErrorList{}

	if p.OSDisk.DiskSizeGB < 0 {
	allErrs = append(allErrs, field.Invalid(fldPath.Child("diskSizeGB"), p.OSDisk.DiskSizeGB, "Storage DiskSizeGB must be positive"))
	}
	return allErrs
	}

		//"UltraSSD_LRS": "",
		_, found := map[string]string{"Standard_LRS": "", "StandardSSD_LRS": "", "Premium_LRS": ""}[p.OSDisk.DiskType]

Bug 1836337: Azure: Add functionality to change Azure Machine Disk Types #3520

Bug 1836337: Azure: Add functionality to change Azure Machine Disk Types #3520

Conversation

rna-afk commented Apr 28, 2020 • edited

patrickdillon commented Apr 28, 2020

patrickdillon commented Apr 28, 2020

rna-afk commented Apr 28, 2020

rna-afk commented Apr 28, 2020

patrickdillon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rna-afk Apr 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rna-afk commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya commented Apr 30, 2020 • edited

patrickdillon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya commented Apr 30, 2020

Choose a reason for hiding this comment

rna-afk commented May 1, 2020

rna-afk commented May 5, 2020 • edited

abhinavdahiya commented May 18, 2020

abhinavdahiya commented May 18, 2020

abhinavdahiya commented May 18, 2020

abhinavdahiya commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-ci-robot commented May 19, 2020

openshift-bot commented May 19, 2020

openshift-ci-robot commented May 19, 2020

rna-afk commented Apr 28, 2020 •

edited

rna-afk Apr 29, 2020 •

edited

abhinavdahiya commented Apr 30, 2020 •

edited

rna-afk commented May 5, 2020 •

edited