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

update infrastructure module #216

Merged
merged 1 commit into from
Feb 23, 2024
Merged

update infrastructure module #216

merged 1 commit into from
Feb 23, 2024

Conversation

umeshkumhar
Copy link
Collaborator

  • simplify infra module

@@ -35,7 +35,7 @@ create_service_account = true
gcp_service_account = "jupyter-service-account"

# Jupyterhub with IAP
add_auth = true
add_auth = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

why false?


all_node_pools_tags = ["gke-node", "ai-on-gke"]

create_network = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

create_network = true
network_name = "ml-network"
subnetwork_name = "ml-subnet1"
subnetwork_cidr = "10.100.0.0/16"
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this still have the limitation of only 1 cluster per network due to IP space?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it can have more clusters as well in same network

@@ -37,7 +37,7 @@ resource "helm_release" "iap_jupyter" {
chart = "${path.module}/charts/iap_jupyter/"
namespace = var.namespace
create_namespace = !contains(data.kubernetes_all_namespaces.allns.namespaces, var.namespace)

timeout = 1200
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe leave a comment explaining why it's needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

helm install takes time.. because of autopilot pod scheduling may take addtional time..

and if pods are not running within 5 minutes tf apply will fail

@@ -90,6 +90,7 @@ resource "helm_release" "jupyterhub" {
namespace = var.namespace
create_namespace = !contains(data.kubernetes_all_namespaces.allns.namespaces, var.namespace)
cleanup_on_fail = "true"
timeout = 1200
Copy link
Collaborator

Choose a reason for hiding this comment

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

same, leave a comment

@umeshkumhar umeshkumhar merged commit 96fe068 into main Feb 23, 2024
5 of 6 checks passed
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.

2 participants