-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
azurerm_kubernetes_cluster_node_pool
: Adds support for temporary_name_for_rotation
#27791
Conversation
Updates the NodePoolUpdate function to rotate the node pool. Removes the ForceNew flag on properties.
Restoring name as ForceNew.
Deleting obsolete methods. Renaming `retrySystemNodePoolCreation` to `retryNodePoolCreation`.
Test LogsTF_ACC=1 go test -v ./internal/services/containers -run=TestAccKubernetesClusterNodePool_ -timeout 180m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
--- PASS: TestAccKubernetesClusterNodePool_ultraSSD (1048.43s) |
Note on test cases
|
azurerm_kubernetes_cluster_node_pool
: Adds support for
temporary_name_for_rotation`azurerm_kubernetes_cluster_node_pool
: Adds support for temporary_name_for_rotation
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.
Thanks for this PR @CorrenSoft!
In addition to the comments and suggestions in-line, we also need to consider the behaviour when rotation of the node pool fails e.g. what happens if we fail to spin up the node pool with the new configuration and are left with the temporary node pool? It would be good if failures here are recoverable for the user.
The azurerm_kubernetes_cluster
handles this by falling back on the temporary_name_for_rotation
system node pool when we perform a read.
Test cases that simulate these failure scenarios should be added for this as well, I've linked two tests we wrote for the AKS resource below that can help you with those:
terraform-provider-azurerm/internal/services/containers/kubernetes_cluster_scaling_resource_test.go
Lines 102 to 164 in eeb928f
func TestAccKubernetesCluster_updateVmSizeAfterFailureWithTempWithoutDefault(t *testing.T) { | |
data := acceptance.BuildTestData(t, "azurerm_kubernetes_cluster", "test") | |
r := KubernetesClusterResource{} | |
data.ResourceTest(t, r, []acceptance.TestStep{ | |
{ | |
Config: r.basicWithTempName(data), | |
Check: acceptance.ComposeTestCheckFunc( | |
check.That(data.ResourceName).ExistsInAzure(r), | |
// create the temporary node pool and delete the old default node pool to simulate the case where resizing fails when trying to bring up the new node pool | |
data.CheckWithClientForResource(func(ctx context.Context, clients *clients.Client, state *terraform.InstanceState) error { | |
if _, ok := ctx.Deadline(); !ok { | |
var cancel context.CancelFunc | |
ctx, cancel = context.WithTimeout(ctx, 1*time.Hour) | |
defer cancel() | |
} | |
client := clients.Containers.AgentPoolsClient | |
id, err := commonids.ParseKubernetesClusterID(state.Attributes["id"]) | |
if err != nil { | |
return err | |
} | |
defaultNodePoolId := agentpools.NewAgentPoolID(id.SubscriptionId, id.ResourceGroupName, id.ManagedClusterName, state.Attributes["default_node_pool.0.name"]) | |
resp, err := client.Get(ctx, defaultNodePoolId) | |
if err != nil { | |
return fmt.Errorf("retrieving %s: %+v", defaultNodePoolId, err) | |
} | |
if resp.Model == nil { | |
return fmt.Errorf("retrieving %s: model was nil", defaultNodePoolId) | |
} | |
tempNodePoolName := "temp" | |
profile := resp.Model | |
profile.Name = &tempNodePoolName | |
profile.Properties.VMSize = pointer.To("Standard_DS3_v2") | |
tempNodePoolId := agentpools.NewAgentPoolID(id.SubscriptionId, id.ResourceGroupName, id.ManagedClusterName, tempNodePoolName) | |
if err := client.CreateOrUpdateThenPoll(ctx, tempNodePoolId, *profile); err != nil { | |
return fmt.Errorf("creating %s: %+v", tempNodePoolId, err) | |
} | |
if err := client.DeleteThenPoll(ctx, defaultNodePoolId); err != nil { | |
return fmt.Errorf("deleting default %s: %+v", defaultNodePoolId, err) | |
} | |
return nil | |
}, data.ResourceName), | |
), | |
// the plan will show that the default node pool name has been set to "temp" and we're trying to set it back to "default" | |
ExpectNonEmptyPlan: true, | |
}, | |
{ | |
Config: r.updateVmSize(data, "Standard_DS3_v2"), | |
Check: acceptance.ComposeTestCheckFunc( | |
check.That(data.ResourceName).ExistsInAzure(r), | |
), | |
}, | |
data.ImportStep("default_node_pool.0.temporary_name_for_rotation"), | |
}) | |
} |
terraform-provider-azurerm/internal/services/containers/kubernetes_cluster_scaling_resource_test.go
Lines 44 to 99 in eeb928f
func TestAccKubernetesCluster_updateVmSizeAfterFailureWithTempAndDefault(t *testing.T) { | |
data := acceptance.BuildTestData(t, "azurerm_kubernetes_cluster", "test") | |
r := KubernetesClusterResource{} | |
data.ResourceTest(t, r, []acceptance.TestStep{ | |
{ | |
Config: r.basic(data), | |
Check: acceptance.ComposeTestCheckFunc( | |
check.That(data.ResourceName).ExistsInAzure(r), | |
// create the temporary node pool to simulate the case where both old default node pool and temp node pool exist | |
data.CheckWithClientForResource(func(ctx context.Context, clients *clients.Client, state *terraform.InstanceState) error { | |
if _, ok := ctx.Deadline(); !ok { | |
var cancel context.CancelFunc | |
ctx, cancel = context.WithTimeout(ctx, 1*time.Hour) | |
defer cancel() | |
} | |
client := clients.Containers.AgentPoolsClient | |
id, err := commonids.ParseKubernetesClusterID(state.Attributes["id"]) | |
if err != nil { | |
return err | |
} | |
defaultNodePoolId := agentpools.NewAgentPoolID(id.SubscriptionId, id.ResourceGroupName, id.ManagedClusterName, state.Attributes["default_node_pool.0.name"]) | |
resp, err := client.Get(ctx, defaultNodePoolId) | |
if err != nil { | |
return fmt.Errorf("retrieving %s: %+v", defaultNodePoolId, err) | |
} | |
if resp.Model == nil { | |
return fmt.Errorf("retrieving %s: model was nil", defaultNodePoolId) | |
} | |
tempNodePoolName := "temp" | |
profile := resp.Model | |
profile.Name = &tempNodePoolName | |
profile.Properties.VMSize = pointer.To("Standard_DS3_v2") | |
tempNodePoolId := agentpools.NewAgentPoolID(id.SubscriptionId, id.ResourceGroupName, id.ManagedClusterName, tempNodePoolName) | |
if err := client.CreateOrUpdateThenPoll(ctx, tempNodePoolId, *profile); err != nil { | |
return fmt.Errorf("creating %s: %+v", tempNodePoolId, err) | |
} | |
return nil | |
}, data.ResourceName), | |
), | |
}, | |
{ | |
Config: r.updateVmSize(data, "Standard_DS3_v2"), | |
Check: acceptance.ComposeTestCheckFunc( | |
check.That(data.ResourceName).ExistsInAzure(r), | |
), | |
}, | |
data.ImportStep("default_node_pool.0.temporary_name_for_rotation"), | |
}) |
I hope that makes sense, let me know if you have any questions!
internal/services/containers/kubernetes_cluster_node_pool_resource.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource.go
Outdated
Show resolved
Hide resolved
Hey @CorrenSoft we have some customers requesting this feature. Would you be able to let me know whether you're planning to work through the review feedback that was left? I'm happy to take this over to get it in if you don't find yourself with time/energy at the moment to get back to this, just checking before I step on your toes here 🙂 |
I see the point. I will do further test to evaluate how is behaving and what can do about it. |
@stephybun, Regarding the fallback in case of failure, in this case, it would not be necessary since these additional nodes do not need (nor should they) be marked as default. |
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.
Thanks for adding those test cases @CorrenSoft! I think some old tests that were removed because they're no longer relevant post 4.0 crept their way into your PR. Could you please remove those and take a look at the additional comments I left in-line?
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
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.
Thanks for taking the time to work on this @CorrenSoft! I know this will be much appreciated by the community and our customers.
The tests look good, I think this is ready to go in. LGTM 🚀
* update for #27680 * Update CHANGELOG.md for #28465 * Update CHANGELOG.md #27932 * Update CHANGELOG.md for #28505 * Update CHANGELOG.md for #28474 * Update CHANGELOG.md #28516 * Update CHANGELOG for #28456 * Update CHANGELOG.md for #28472 * Update CHANGELOG.md #28307 * Update CHANGELOG.md for #27859 * Update for #28519 * Update for #27791 #27528 * Update CHANGELOG.md for #28527 * update changelog links and generate provider schema --------- Co-authored-by: jackofallops <[email protected]> Co-authored-by: catriona-m <[email protected]> Co-authored-by: sreallymatt <[email protected]> Co-authored-by: Matthew Frahry <[email protected]> Co-authored-by: jackofallops <[email protected]>
…ame_for_rotation` (hashicorp#27791) * Adds property. Updates the NodePoolUpdate function to rotate the node pool. Removes the ForceNew flag on properties. * Updating tests. Restoring name as ForceNew. * Updating Docs. * Fixing value assignment. Deleting obsolete methods. Renaming `retrySystemNodePoolCreation` to `retryNodePoolCreation`. * Updating properties values from HCL definition. * Remove unused function (schemaNodePoolSysctlConfigForceNew) * Fixing docs * Update pointer's function. * Improving subnet assignment * Fixing zones not being updated when value was set to null. * Fixing assigment when value is null * Restoring files lose on merge. * Linting * Adds 'TestAccKubernetesClusterNodePool_updateVmSizeAfterFailureWithTempAndOriginal' * Adds TestAccKubernetesCluster_updateVmSizeAfterFailureWithTempWithoutOriginal * Fix test's name. * Removing deprecated test and applying feedback. * Applying feedback. * Removing obsolete code.
* update for hashicorp#27680 * Update CHANGELOG.md for hashicorp#28465 * Update CHANGELOG.md hashicorp#27932 * Update CHANGELOG.md for hashicorp#28505 * Update CHANGELOG.md for hashicorp#28474 * Update CHANGELOG.md hashicorp#28516 * Update CHANGELOG for hashicorp#28456 * Update CHANGELOG.md for hashicorp#28472 * Update CHANGELOG.md hashicorp#28307 * Update CHANGELOG.md for hashicorp#27859 * Update for hashicorp#28519 * Update for hashicorp#27791 hashicorp#27528 * Update CHANGELOG.md for hashicorp#28527 * update changelog links and generate provider schema --------- Co-authored-by: jackofallops <[email protected]> Co-authored-by: catriona-m <[email protected]> Co-authored-by: sreallymatt <[email protected]> Co-authored-by: Matthew Frahry <[email protected]> Co-authored-by: jackofallops <[email protected]>
Community Note
Description
temporary_name_for_rotation
property (optional, not persisted).TestAccKubernetesClusterNodePool_manualScaleVMSku
andTestAccKubernetesClusterNodePool_ultraSSD
test cases.schemaNodePoolSysctlConfigForceNew
,schemaNodePoolKubeletConfigForceNew
andschemaNodePoolLinuxOSConfigForceNew
as they are no longer used.retrySystemNodePoolCreation
toretryNodePoolCreation
, as now is being used for both cases.PR Checklist
Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_kubernetes_cluster_node_pool
- adds thetemporary_name_for_rotation
property to allow to cycle the node pool instead of a direct recreation [azurerm_kubernetes_cluster_node_pool
: Adds support fortemporary_name_for_rotation
#27791]This is a (please select all that apply):
Related Issue(s)
Fixes #22265
Note
If this PR changes meaningfully during the course of review please update the title and description as required.