Add 'priority' property for VM scale set #1250

CoRfr · 2018-05-17T04:17:32Z

Purpose is to support low-priority VMs, which should partially
address #1249.

This is simply adding a 'priority' parameter that can be set
to Low or Regular (=default).

The evictionPolicy is not supported yet as it was introduced
in a more recent version of the compute API (2018-04-01).

Cf https://docs.microsoft.com/en-us/azure/virtual-machine-scale-sets/virtual-machine-scale-sets-use-low-priority

katbyte

Thank you for adding this property! It LGTM aside from one minor comment on the test. Once that is fixed up i look forward to getting this merged 🙂

katbyte · 2018-05-19T06:40:45Z

azurerm/resource_arm_virtual_machine_scale_set_test.go

@@ -820,7 +854,7 @@ func testCheckAzureRMVirtualMachineScaleSetSinglePlacementGroup(name string, exp
 		}

 		if *resp.SinglePlacementGroup != expectedSinglePlacementGroup {
-			return fmt.Errorf("Bad: Overprovision should have been %t for scale set %v", expectedSinglePlacementGroup, name)
+			return fmt.Errorf("Bad: SinglePlacementGroup should have been %t for scale set %v", expectedSinglePlacementGroup, name)


Thank you for fixing this too!

katbyte · 2018-05-19T06:48:31Z

azurerm/resource_arm_virtual_machine_scale_set_test.go

+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMVirtualMachineScaleSetExists("azurerm_virtual_machine_scale_set.test"),
+					testCheckAzureRMVirtualMachineScaleSetPriority("azurerm_virtual_machine_scale_set.test", "Low"),


Unless there is a reason for us to query Azure directly could we change this to check the state? like:

resource.TestCheckResourceAttr(resourceName, "azurerm_virtual_machine_scale_set.test. priority", "low"),

I know this isn't like many of the tests in the resource, but without good reason this is how we should be verifying the new property as it also checks the state.

CoRfr · 2018-05-22T20:50:56Z

@katbyte I updated the test and also rebased.

katbyte

Hey @CoRfr,

Thanks for the timely changes. Re-reviewed and left some comment inline. It looks like you left in the old test function.

Also could we update the documentation to reflect the new property?

katbyte · 2018-05-23T05:55:26Z

azurerm/resource_arm_virtual_machine_scale_set_test.go

@@ -812,6 +829,21 @@ func testCheckAzureRMVirtualMachineScaleSetOverprovision(name string) resource.T
 	}
 }

+func testCheckAzureRMVirtualMachineScaleSetPriority(name string, expectedPriority string) resource.TestCheckFunc {


I think you forgot to remove this.

Oops, removed.

katbyte · 2018-05-23T05:58:00Z

azurerm/resource_arm_virtual_machine_scale_set.go

+			"priority": {
+				Type:     schema.TypeString,
+				Optional: true,
+			},


Is this possible to validate against the VirtualMachinePriorityTypes enum?

katbyte · 2018-05-23T05:59:36Z

azurerm/resource_arm_virtual_machine_scale_set_test.go

+		Steps: []resource.TestStep{
+			{
+				Config: config,
+				Check:  resource.TestCheckResourceAttr(resourceName, "priority", "Low"),


could we also check that the resource exists:

Check: resource.ComposeTestCheckFunc( testCheckAzureRMVirtualMachineExists(resourceName, &afterCreate), resource.TestCheckResourceAttr(resourceName, "priority", "Low"), ),

djsly · 2018-05-23T11:32:05Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -115,6 +115,11 @@ func resourceArmVirtualMachineScaleSet() *schema.Resource {
 				ForceNew: true,
 			},

+			"priority": {
+				Type:     schema.TypeString,
+				Optional: true,


could we add "ForceNew" here since a scale set cannot have a mix of Low and Regular VM. Unless Azure will update all pre-existing VM if you change your scaleset from Regular to Low?

Seems like we would need a new scale set, so added ForceNew as suggested.

djsly · 2018-05-23T11:33:49Z

azurerm/resource_arm_virtual_machine_scale_set_test.go

+  }
+}
+
+`, rInt, location, rInt, rInt, rInt, rInt, rInt)


can we reuse the first rInt using %[1]d, I cleaned up all the previous tests in my pending PR

Sounds good, I had a look at https://github.com/terraform-providers/terraform-provider-azurerm/pull/1209/files#diff-62e6c44ba2b5a3617bb9c78744b7a84bR515-R533 and did something similar.

Purpose is to support low-priority VMs, which should partially address hashicorp#1249. This is simply adding a 'priority' parameter that can be set to Low or Regular (=default). The evictionPolicy is not supported yet as it was introduced in a more recent version of the compute API (2018-04-01). Cf https://docs.microsoft.com/en-us/azure/virtual-machine-scale-sets/virtual-machine-scale-sets-use-low-priority

katbyte

@CoRfr, LGTM! 👍 running tests

katbyte · 2018-05-24T16:36:34Z

tests pass (with 2 known failures)

Thank you for the contribution @CoRfr 🙂

ghost · 2020-03-31T14:56:18Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

katbyte self-requested a review May 18, 2018 02:41

katbyte requested changes May 19, 2018

View reviewed changes

katbyte added enhancement service/vmss Virtual Machine Scale Sets labels May 19, 2018

katbyte added this to the 1.6.0 milestone May 19, 2018

CoRfr force-pushed the lowpriorityscalesets branch from 4f2a5ef to eac4932 Compare May 22, 2018 20:49

katbyte requested changes May 23, 2018

View reviewed changes

katbyte mentioned this pull request May 23, 2018

Additional VMSS properties (issues #1210 & #1226 #1209

Merged

djsly reviewed May 23, 2018

View reviewed changes

CoRfr force-pushed the lowpriorityscalesets branch from eac4932 to 3860a2e Compare May 24, 2018 07:23

katbyte approved these changes May 24, 2018

View reviewed changes

katbyte merged commit a2ea8a0 into hashicorp:master May 24, 2018

katbyte added a commit that referenced this pull request May 24, 2018

Update CHANGELOG.MD to include #1250

ba72f4f

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'priority' property for VM scale set #1250

Add 'priority' property for VM scale set #1250

CoRfr commented May 17, 2018

katbyte left a comment

katbyte May 19, 2018

CoRfr May 22, 2018

katbyte May 19, 2018

CoRfr commented May 22, 2018

katbyte left a comment •

edited

Loading

katbyte May 23, 2018

CoRfr May 24, 2018

katbyte May 23, 2018

katbyte May 23, 2018

djsly May 23, 2018

CoRfr May 24, 2018

djsly May 23, 2018

CoRfr May 24, 2018

katbyte left a comment

katbyte commented May 24, 2018

ghost commented Mar 31, 2020

Add 'priority' property for VM scale set #1250

Add 'priority' property for VM scale set #1250

Conversation

CoRfr commented May 17, 2018

katbyte 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

CoRfr commented May 22, 2018

katbyte left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

katbyte commented May 24, 2018

ghost commented Mar 31, 2020

katbyte left a comment •

edited

Loading