Skip to content

Add support for nodepool's gpu_instance#519

Merged
lonegunmanb merged 2 commits intomainfrom
e-517
Mar 5, 2024
Merged

Add support for nodepool's gpu_instance#519
lonegunmanb merged 2 commits intomainfrom
e-517

Conversation

@lonegunmanb
Copy link
Copy Markdown
Member

Describe your changes

Issue number

#517

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

Thanks for your cooperation!

@lonegunmanb
Copy link
Copy Markdown
Member Author

Hi @zioproto , would you please give this pr a review? Thanks!

Comment thread main.tf Outdated
enable_host_encryption = var.enable_host_encryption
enable_node_public_ip = var.enable_node_public_ip
fips_enabled = var.default_node_pool_fips_enabled
gpu_instance = var.default_node_pool_gpu_instance
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you would need GPUs in the system nodepool ?

Comment thread variables.tf Outdated
description = " (Optional) Should the nodes in this Node Pool have Federal Information Processing Standard enabled? Changing this forces a new resource to be created."
}

variable "default_node_pool_gpu_instance" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default nodepool is a system node pool that contains also AKS system workloads. I don't see the value of adding a GPU workload on this nodepool.

I suggest to enable the GPU features only on the extra nodepools.

Why your use case needs a GPU on the system node pool ? Could you please share more about your scenario ? Thanks

@lonegunmanb lonegunmanb merged commit bd52700 into main Mar 5, 2024
@lonegunmanb lonegunmanb deleted the e-517 branch December 2, 2024 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants