Containerservices agentpoolprofile maxpods #1753
Conversation
There was a problem hiding this comment.
hey @lfshr
Thanks for this PR :)
I've left a comment inline about the default value - but this otherwise LGTM; if we're able to confirm that and add a test for this (to ensure the max_pods value is set as expected) then this otherwise LGTM 👍
Thanks!
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| ForceNew: true, | ||
| Default: 30, |
There was a problem hiding this comment.
given the discussion in #1729 where it sounds like this limit can change - as such do we need to verify this value in both conditions to be sure (e.g. not attached to a network and attached to a network?) - if it changes it could make more sense to make this field Computed? 🤔
There was a problem hiding this comment.
Of course it does... Doh!
There was a problem hiding this comment.
Fixed via 8a40400
In my defense, I'm still in holiday mode! 😝
Thanks Tom!
There was a problem hiding this comment.
Just going to check the fact it's an int isn't going to cause problems. I may need to conditionally supply this to the SDK (if value > 0)
There was a problem hiding this comment.
Hold off on this for now. I can't seem to login to the az client this morning. @tombuildsstuff could you possibly try and spin up a cluster with no max_pods specified and tell me if it works? 😕
There was a problem hiding this comment.
In my defense, I'm still in holiday mode! 😝
heh no worries, thanks for fixing it - hope you enjoyed it :)
I've spun one up using the existing resource (which sets nothing) - which defaults this to 110 - so I think this should be fine 👍
There was a problem hiding this comment.
Hey @lfshr
Thanks for pushing those changes - if we can update the acceptance tests to check this functionality then this otherwise LGTM 👍
Thanks!
| @@ -691,6 +702,10 @@ func expandAzureRmKubernetesClusterAgentProfiles(d *schema.ResourceData) []conta | |||
| OsType: containerservice.OSType(osType), | |||
| } | |||
|
|
|||
| if maxPods := int32(config["max_pods"].(int)); maxPods > 0 { | |||
| profile.MaxPods = utils.Int32(maxPods) | |||
| } | |||
There was a problem hiding this comment.
could we update the acceptance tests (for the internal network probably makes the most sense, rather than a new test?) to set this value?
| @@ -209,6 +209,7 @@ The following arguments are supported: | |||
| * `os_disk_size_gb` - (Optional) The Agent Operating System disk size in GB. Changing this forces a new resource to be created. | |||
| * `os_type` - (Optional) The Operating System used for the Agents. Possible values are `Linux` and `Windows`. Changing this forces a new resource to be created. Defaults to `Linux`. | |||
| * `vnet_subnet_id` - (Optional) The ID of the Subnet where the Agents in the Pool should be provisioned. Changing this forces a new resource to be created. | |||
| * `max_pods` - (Optional) The maximum number of pods that can run on each agent. Default is 30. | |||
There was a problem hiding this comment.
This no longer defaults to 30 - so can we remove this?
|
@tombuildsstuff That should be everything :) |
There was a problem hiding this comment.
hey @lfshr
Thanks for pushing the latest changes - this now LGTM 👍 - I'll kick off the tests shortly :)
Thanks!
|
Tests pass - thanks again for this @lfshr :)
|
|
Thank you |
|
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
Added max_pods to agent_pool_profile in azurerm_kubernetes_cluster. This fixes #1729