-
Notifications
You must be signed in to change notification settings - Fork 65
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
Update Azure Terraform config to actually create dask pools #839
Conversation
@@ -117,7 +117,7 @@ resource "azurerm_kubernetes_cluster_node_pool" "user_pool" { | |||
resource "azurerm_kubernetes_cluster_node_pool" "dask_pool" { | |||
# If dask_nodes is set, we use that. If it isn't, we use notebook_nodes. | |||
# This lets us set dask_nodes to an empty array to get no dask nodes | |||
for_each = try(var.dask_nodes, var.notebook_nodes) | |||
for_each = length(var.dask_nodes) == 0 ? var.notebook_nodes : var.dask_nodes |
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.
The reason I used try
instead in #834 is that without that, I can't tell JIL to not have any dask nodes - nil or null weren't accepted as variables values, and the default of {}
meant it wasn't possible to figure out a way to not have any nodes :( Maybe if the default for dask_nodes
is set to something other than {}
might help? Not sure.
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.
Yeah, I'm not sure how to fix this. Because without the suggested change I'm struggling to tell carbon plan to create the nodes. Unless we switch to not automatically creating a dask pool from defined notebook pools
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.
Given that terraform is not a procedural language, we may be trying to do something that is too complex for it's capabilities
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.
ok, how about we:
- Remove the one liner setting
dask_nodes = {}
in the JIL tfvars, - Merge this as is?
This means that when I tf apply
JIL later, it'll also create those extra nodepools, but they should be empty and cost nothing. Let's just do that?
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.
Seems ok to me for the time being - I've pushed that change
While this creates nodepools, they should be empty and cost nothing. This helps to unblock the CarbonPlan deployment.
@sgibson91 longer term, if you think we should decouple these two lists, am very happy for you to pursue that instead. |
@yuvipanda I think that might be the only way without doing something really hacky and tough to understand, but I've only had a half hour's worth of thought on it :D |
Terraform apply complete! |
This PR fixes a conditional bug in our Azure terraform config that was preventing our dask node pools from actually being created, and also edits the node labels and taints to match that in our GKE config and allow dask pods to be scheduled to nodes. I ran this against the Azure CarbonPlan cluster. This should fix the bug I was seeing in #838.
Output of Terraform Plan: