-
Notifications
You must be signed in to change notification settings - Fork 66
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
Setup nodepool for neurohackademy #2758
Conversation
Reconstruction/reversion of PR 2i2c-org#1726
for more information, see https://pre-commit.ci
terraform plan output under the fold. I seemed to have picked up some climatematch related changes which could either be related to #2757 or just an out-of-date state. I will stash my changes and try a refresh only plan first. tf plan output``` Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: + create ~ update in-placeTerraform will perform the following actions: google_container_node_pool.dask_worker["worker"] will be updated in-place~ resource "google_container_node_pool" "dask_worker" {
google_container_node_pool.notebook["climatematch"] will be updated in-place~ resource "google_container_node_pool" "notebook" {
google_container_node_pool.notebook["neurohackademy"] will be created
google_container_node_pool.notebook["user"] will be updated in-place~ resource "google_container_node_pool" "notebook" {
Plan: 1 to add, 3 to change, 0 to destroy. Changes to Outputs:
|
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.
LGTM
Please add requested comment to refer to the issue tracking this event then I will approve
@@ -35,6 +35,15 @@ notebook_nodes = { | |||
resource_labels : { | |||
"community" : "climatematch" | |||
} | |||
}, | |||
"neurohackademy" : { | |||
# We expect around 120 users |
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.
Can you add a comment linking to the issue
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.
Done!
Co-authored-by: Erik Sundell <[email protected]>
New tf plan output below. Still seeing some unrelated changes, but I think they are non-destructive.
|
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.
LGTM
Ok, the changes to the climatematch, user and worker nodepools are causing issues for my changes
|
I have now defined the expected zones for those nodepools and the new plan output is below, which I suspect will apply cleanly now
|
Ok, it did not apply cleanly and I will have to define |
Nope, I am still seeing the error even when I have defined a zone for the nodepool
|
@sgibson91 I'm also seeing issues with node pools and terraform, I'll try debug some this morning. |
@sgibson91 @pnasrat I've fixed this up, with the following sets of changes:
I've applied this as well, so it's ready to merge. |
The reason this was not caught as part of #2406 is that regional clusters do have |
Given that the event only starts on August 7th, I've set the minimum nodepool size to 0, not 1, as otherweise we'll be spending a lot of money on n1-highmem-16 in that time period. I'll document this shortly. |
Thank you @yuvipanda! |
Reconstruction/reversion of PR #1726
Working towards #2681