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

Deploy binderHub and other K8s apps on AWS curvenote #2698

Merged
merged 32 commits into from
Nov 1, 2023

Conversation

manics
Copy link
Member

@manics manics commented Jul 12, 2023

Follow-on from #2652 for AWS Curvenote. Deploys

  • Some kube-system charts (load balancer, block storage)
  • BinderHub
  • ECR registry helper and cleaner charts

This has been manually deployed (./deploy.py ....)

@manics manics force-pushed the aws-curvenote-binder branch from 7eb89ad to 97f5400 Compare September 25, 2023 21:54
@manics manics force-pushed the aws-curvenote-binder branch 2 times, most recently from eb7c9b0 to b84322c Compare October 8, 2023 11:41
@manics manics force-pushed the aws-curvenote-binder branch from b84322c to d00589d Compare October 8, 2023 12:30
@manics manics force-pushed the aws-curvenote-binder branch from d00589d to 309d537 Compare October 8, 2023 12:35
Copy link
Member Author

@manics manics left a comment

Choose a reason for hiding this comment

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

I think this is ready for review!

The main missing things are:

  • Adding this to the redirector: I'd like to leave this for a later PR, and focus on getting the continuous deployment running first .
  • GOOGLE_APPLICATION_CREDENTIALS: Could be done now, or in a follow-up

# mountPath: /secrets
# readOnly: true
# extraEnv:
# GOOGLE_APPLICATION_CREDENTIALS: /secrets/service-account.json
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main thing missing- how do I create the credentials?

@@ -342,6 +398,13 @@ def main():
action="store_true",
help="Print commands, but don't run them",
)
stages = ["all", "auth", "networkbans", "kubesystem", "certmanager", "mybinder"]
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added an optional --stage argument to make it easier to run deploy.py locally. There should be no change to the default behaviour.

@@ -0,0 +1,12 @@
{{- range $name, $priority := .Values.priorityClasses }}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an alternative to using separate node pools.

@@ -0,0 +1,20 @@
# Enable NetworkPolicies on EKS
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added this directory as a record of what I did to enable network policies in EKS (which was painful......). Not sure if we need terraform/aws/curvenote/cni/aws-k8s-cni-us-east-2.yaml for completeness, or if it's enough just to have the readme?

@manics manics marked this pull request as ready for review October 8, 2023 13:00
from binderhub.registry import DockerRegistry


class ExternalRegistryHelper(DockerRegistry):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this will eventually go upstream into binderhub

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully! I think we should try it in production for a bit, and if there are no unforeseen issues add it upstream.

AWS_DEPLOYMENTS[cluster],
]
stdout = check_output(eks_kubeconfig, dry_run)
print(stdout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

check_output will hide stdout, so I think it's useful to print it out as it provides confirmation that auth worked, and it also mirrors the effect of running the command manually.

Copy link
Contributor

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

Given the lack of review time, @manics at this point I think you should self merge and iterate so we can add more capactiy. I think overall this looks good to me and we can obviously change after.

THANK YOU FOR WORKING ON THIS!

@manics
Copy link
Member Author

manics commented Oct 27, 2023

@yuvipanda Thanks! I'll merge when I have a decent block of time to deal with any problems.

@manics manics mentioned this pull request Oct 31, 2023
4 tasks
@manics
Copy link
Member Author

manics commented Nov 1, 2023

@yuvipanda I'm hoping to spend some time on this later today, but let me know if you'd rather I wait until the cryptnono work is finished.

@yuvipanda
Copy link
Contributor

@manics nah you can go ahead!

@manics manics merged commit fed3d1d into jupyterhub:main Nov 1, 2023
5 checks passed
@manics manics deleted the aws-curvenote-binder branch November 1, 2023 20:08
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