Skip to content
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

updated check for OS disks provisioned for AKS #2407 #2547

Merged

Conversation

varshneydevansh
Copy link
Contributor

Issue >

Infracost uses Premium disk for AKS even though it's not available for given VM family #2407

Changes made>

kubernetes_cluster_node_pool.go

in the below function, added the instaceType to the calling function of aksOSDiskSubResource()

func aksClusterNodePool()
{
    // rest of the code
    osDisk := aksOSDiskSubResource(region, diskSize, instanceType)

}

in the below function aksOSDiskSubResource() added the call to the new function aksGetStorageType() which returns a string whether the OS Disk provisioned is Premium or Standard based on the naming convention subfamily has any known premium prefixes (DS, GS, and M), and if so, it returns “Premium”. If not, it checks if the subfamily contains an 's' as an 'Additive Feature', as specified in the Azure VM naming conventions with the help of the regex. If it finds a match, it also returns “Premium”. Otherwise, it returns “Standard”.

func aksOSDiskSubResource(region string, diskSize int, instanceType string) *schema.Resource {
	diskType := aksGetStorageType(instanceType)
        // rest of the code
}

@aliscott aliscott self-requested a review July 5, 2023 16:12
@aliscott aliscott self-assigned this Jul 5, 2023
@aliscott
Copy link
Member

aliscott commented Jul 6, 2023

@varshneydevansh this looks great. Is it possible to add a test case for a non-premium instance type here: https://github.com/infracost/infracost/blob/master/internal/providers/terraform/azure/testdata/kubernetes_cluster_node_pool_test/kubernetes_cluster_node_pool_test.tf

@varshneydevansh
Copy link
Contributor Author

Somewhat like this?

resource "azurerm_kubernetes_cluster_node_pool" "dv3_standard_d2_v31" {
  name                  = "internal"
  kubernetes_cluster_id = azurerm_kubernetes_cluster.example.id
  vm_size               = "Standard_D2_v31"
}

or like this -

resource "azurerm_kubernetes_cluster_node_pool" "non_premium" {
  name                  = "internal"
  kubernetes_cluster_id = azurerm_kubernetes_cluster.example.id
  vm_size               = "Standard_D2_v31"
}

I will add the snippet after your confirmation in the file kubernetes_cluster_node_pool_test.tf

@aliscott
Copy link
Member

aliscott commented Jul 6, 2023

or like this -

This one looks good to me!

@aliscott
Copy link
Member

aliscott commented Jul 7, 2023

Thanks @varshneydevansh this looks good. I've pushed a commit to this just to update the golden files to make sure the tests will pass and then will merge 🚀

@aliscott aliscott merged commit bd9c9a6 into infracost:master Jul 7, 2023
10 checks passed
@varshneydevansh varshneydevansh deleted the branch-kube-cluster-node-push branch July 7, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants