-
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
- support for the source_snapshot_id
property
#21511
azurerm_kubernetes_cluster_node_pool
- support for the source_snapshot_id
property
#21511
Conversation
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 @ms-henglu. Overall this is off to a good start - once the comments left in-line have been addressed we can take another look through.
internal/services/containers/kubernetes_cluster_node_pool_resource.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
Thanks @stephybun for the code review, I've updated this PR as suggested, would you please take anther look? Thanks! |
I am testing like this:
This is the snapshot I am using:
I am failing with the following error:
What does it mean |
Hi @zioproto , Thank you for testing this feature, I've confirmed it with the service team, it seems that the snapshot is taken when the node pool is using Ubuntu 18.04, now the node pool uses 20.04, so it failed to create the node pool. Would you please try to create a new snapshot then restore it? |
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 making those changes @ms-henglu, I left a few more comments but once they're resolved and the tests are passing this should be good to go 👍
internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_snapshot_data_source.go
Outdated
Show resolved
Hide resolved
@ms-henglu I will test again but I am confident the snapshot was taken from a cluster running AKS 1.26.3 You can see it from the output I attached in the previous comment from
There is no Ubuntu 18 involved here. |
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 @ms-henglu LGTM 🍉
@stephybun @ms-henglu I tested again and this is still broken for me. Here are my repro steps: Create a cluster with azcli:
Get the nodepool_id
Create a snapshot:
Get the snapshot id
Run the following Terraform code, where in
I run Once the aks cluster resource is created, the nodepool resource fails immediately, with this error:
This should instead work. After the Terraform fails I have the aks cluster that Terraform created successfully. If I use azcli to create an additional nodepool using that same snapshot ID it works as expected:
Do we need to open a new PR now ? Do you want me to open a new GitHub Issue ? Thanks |
@ms-henglu I am confused, I dont understand what you used in the code between Can you confirm you are using the nodepool snapshot API ? |
Kudos to @stephybun for figuring out I am mixing |
Kudos to @ms-henglu for explaining to me that there are 2 cli commands:
they're using a same API called "Microsoft.ContainerService/snapshots@2023-02-02-preview", there's a field called snapshotType = "NodePool" | "ManagedCluster" which is used to differentiate the snapshot type. |
if the |
@zioproto based on above, it appears that your original issue was that you're were mixing different generations of Virtual Machine - you can't restore a snapshot from a Gen2 compute node onto a Gen1 compute note - unfortunately this is a misleading error message coming back from the API. The
|
This functionality has been released in v3.55.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #21442