Spot Priority support for AKS #7145

pbrit · 2020-05-31T03:04:05Z

Support of Spot Priority for azurerm_kubernetes_cluster_node_pool
node_taints and node_labels were marked as computed and they must force a pool recreation.

The initial work was started as #6231

Co-Authored-By: Alok Kumar [email protected]

TODO

Add test coverage
Update documentation
Rename spot_price to max_bid_price

pbrit · 2020-05-31T03:15:04Z

@tombuildsstuff Although I'm still working on the remaining items, I'd like to know your opinion regarding the implementation. Thank you.

* Support of Spot Priority for `azurerm_kubernetes_cluster_node_pool` * `node_taints` and `node_labels` were marked as *computed* and they must force a new pool resource * Add `max_bid_price` validation for `windows_virtual_machine_scale_set` and `linux_virtual_machine_scale_set` The initial work was started as hashicorp#6231 Fixes hashicorp#6086 Co-Authored-By: Alok Kumar <[email protected]>

pbrit · 2020-06-04T06:16:02Z

@tombuildsstuff, @jackofallops, @katbyte As far as I'm concerned the implementation is ready. Please let me know what I can do to make it merged.

tombuildsstuff

hey @pbrit

Thanks for this PR - apologies for the delayed review here!

Since there's a number of other Pull Requests currently open for AKS - to be able to merge all of these without them all conflicting, I've first reviewed each PR - and then worked to pull these into a single branch locally (before making any necessary fixes from the comments).

Taking a look through this PR is looking pretty good and I've left some comments inline - however we need to take a slightly different approach for a few of the fields (e.g. making the node labels/taints fields non-computed, and remove the customizeDiff).

I've spent some time trying to integrate these commits into the "consolidation" branch I've got locally and unfortunately had issues doing so. As such to be able to ship support for this with these changes, I've ended up having to re-implement these changes atop this branch. Whilst I appreciate that isn't ideal (and we'd like to retain authorship information for both yourself and @rajalokan) - that does allow these changes to ship.

As such, whilst I'd like to thank you for opening this Pull Request, I'm going to close this in favour of #7233 which adds support for both these changes (and the other open AKS PR's - and includes some of the other open Feature Requests).

Thanks!

tombuildsstuff · 2020-06-03T08:31:36Z

azurerm/internal/services/containers/kubernetes_cluster_data_source.go

+
+						"spot_price": {
+							Type:     schema.TypeFloat,
+							Optional: true,


since this is a Data Source these fields are read-only - as such can we make this:

Suggested change

Optional: true,

Computed: true,

tombuildsstuff · 2020-06-03T08:31:53Z

azurerm/internal/services/containers/kubernetes_cluster_data_source.go

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


since this is a Data Source these fields are read-only - as such can we make this:

Suggested change

Optional: true,

Computed: true,

tombuildsstuff · 2020-06-03T08:31:55Z

azurerm/internal/services/containers/kubernetes_cluster_data_source.go

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


since this is a Data Source these fields are read-only - as such can we make this:

Suggested change

Optional: true,

Computed: true,

tombuildsstuff · 2020-06-03T08:49:59Z

azurerm/internal/services/containers/kubernetes_cluster_node_pool_resource.go

@@ -113,15 +113,16 @@ func resourceArmKubernetesClusterNodePool() *schema.Resource {
 			"node_labels": {
 				Type:     schema.TypeMap,
 				Optional: true,
+				Computed: true,


per the documentation users should expect a label of kubernetes.azure.com/scalesetpriority:spot when provisioning - as such I think we can remove Computed here since users should have to specify this - instead we can document this in the examples

Suggested change

Computed: true,

tombuildsstuff · 2020-06-03T08:50:11Z

azurerm/internal/services/containers/kubernetes_cluster_node_pool_resource.go

 			},

 			"node_taints": {
 				Type:     schema.TypeList,
 				Optional: true,
+				Computed: true,


(same here) with the taint kubernetes.azure.com/scalesetpriority=spot:NoSchedule

Suggested change

Computed: true,

tombuildsstuff · 2020-06-03T08:51:20Z

azurerm/internal/services/containers/kubernetes_cluster_node_pool_resource.go

+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,
+				Default:  string(containerservice.Delete),


since this default value is only applicable when using Spot instances (and this field is Optional) - can we remove this?

Suggested change

Default: string(containerservice.Delete),

tombuildsstuff · 2020-06-03T08:52:20Z

azurerm/internal/services/containers/kubernetes_nodepool.go

-					Elem: &schema.Schema{
-						Type: schema.TypeString,
-					},
+					Computed: true,


(as above) we want to remove Computed here

Suggested change

Computed: true,

tombuildsstuff · 2020-06-03T08:52:29Z

azurerm/internal/services/containers/kubernetes_nodepool.go

 					Optional: true,
+					Computed: true,


(as above) we want to remove Computed here:

Suggested change

Computed: true,

tombuildsstuff · 2020-06-03T10:15:00Z

azurerm/internal/services/containers/validate/spot_price.go

+
+	if fractionalPart > 0 {
+		errors = append(errors, fmt.Errorf("%q can only include up to 5 digits after the radix point, got %g", key, v))
+	}


this'll work when a users input 0.000001 - but won't work for 0.000010 which the API will reject; as such unfortunately I think we need to make this:

// either -1 (the current VM price) if v == -1.0 { return } // at least 0.00001 if v < 0.00001 { errors = append(errors, fmt.Errorf("expected %q to be > 0.00001 but got %.5f", k, v)) return }

for it to be consistent

tombuildsstuff · 2020-06-05T14:38:42Z

azurerm/internal/services/containers/kubernetes_cluster_node_pool_resource.go

@@ -39,6 +40,8 @@ func resourceArmKubernetesClusterNodePool() *schema.Resource {
 			Delete: schema.DefaultTimeout(60 * time.Minute),
 		},

+		CustomizeDiff: customizeDiff,


can we remove the customizeDiff here? unfortunately this is broken when a field is interpolated (most of the time)

ghost · 2020-07-05T14:51:25Z

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!

ghost added the size/L label May 31, 2020

This was referenced May 31, 2020

WIP : Support for spot node pool #6231

Closed

Support for spot node pool in azurerm_kubernetes_cluster #6086

Closed

pbrit force-pushed the aks-spot branch from 33522aa to 0ab0a7a Compare May 31, 2020 03:36

jackofallops added enhancement service/kubernetes-cluster labels Jun 1, 2020

tombuildsstuff self-assigned this Jun 2, 2020

pbrit force-pushed the aks-spot branch from 0ab0a7a to 85de2e9 Compare June 4, 2020 06:12

ghost added size/XL documentation and removed size/L labels Jun 4, 2020

tombuildsstuff added the preview label Jun 5, 2020

tombuildsstuff mentioned this pull request Jun 5, 2020

Azure Kubernetes Service: 2020-06 updates #7233

Merged

tombuildsstuff suggested changes Jun 5, 2020

View reviewed changes

tombuildsstuff closed this Jun 5, 2020

ghost locked and limited conversation to collaborators Jul 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spot Priority support for AKS #7145

Spot Priority support for AKS #7145

pbrit commented May 31, 2020 •

edited

Loading

pbrit commented May 31, 2020

pbrit commented Jun 4, 2020

tombuildsstuff left a comment

tombuildsstuff Jun 3, 2020

tombuildsstuff Jun 3, 2020

tombuildsstuff Jun 3, 2020

tombuildsstuff Jun 3, 2020

tombuildsstuff Jun 3, 2020

tombuildsstuff Jun 3, 2020

tombuildsstuff Jun 3, 2020

tombuildsstuff Jun 3, 2020

tombuildsstuff Jun 3, 2020

tombuildsstuff Jun 5, 2020

ghost commented Jul 5, 2020

Spot Priority support for AKS #7145

Spot Priority support for AKS #7145

Conversation

pbrit commented May 31, 2020 • edited Loading

TODO

pbrit commented May 31, 2020

pbrit commented Jun 4, 2020

tombuildsstuff 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

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

ghost commented Jul 5, 2020

pbrit commented May 31, 2020 •

edited

Loading