-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add allen-swdb hub #1585
Add allen-swdb hub #1585
Conversation
Support and Staging deployments
Production deployments
|
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.
A couple of comments but LGTM
- enc-support.secret.values.yaml | ||
hubs: | ||
- name: staging | ||
display_name: "Staging" |
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.
display_name: "Staging" | |
display_name: "Allen Institute (staging)" |
```bash | ||
terraform output -raw continuous_deployer_creds > ../../config/clusters/<your-cluster-name>/deployer-credentials.secret.json | ||
sops --output config/clusters/<your-cluster-name>/enc-deployer-credentials.secret.json --encrypt ../../config/clusters/<your-cluster-name>/deployer-credentials.secret.json | ||
terraform output -raw continuous_deployer_creds > ../../config/clusters/<your-cluster-name>/enc-deployer-credentials.secret.json | ||
sops --in-place --encrypt ../../config/clusters/<your-cluster-name>/enc-deployer-credentials.secret.json | ||
``` |
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.
Maybe we can keep the original command and provide your suggestion as an alternative? I like to not use --in-place
and explicitly add the enc-
through --output
because our .gitignore
file prevents files that contain secret
in their name but are not prefixed with enc-
from being accidentally committed to the repo. I can then clean up unencrypted files using git clean
.
https://infrastructure.2i2c.org/en/latest/topic/secrets.html
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.
@sgibson91 sorry Sarah, I know this isn't the first time I'd changed this to --in-place
- I had forgotten the last time you had reminded me of it! I've just reverted this change to go back to using --output
instead as it was before, and will keep this in mind for the future!
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.
Lovely to see the templating idea already in motion!!!
GitHubOAuthenticator: | ||
allowed_organizations: | ||
- 2i2c-org:tech-team | ||
- alleninstitute |
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.
Please add authorization for GitHub Org SWDB2022: https://github.com/SWDB2022
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.
@mabuice try now?
initContainers: | ||
# Need to explicitly fix ownership here, since EFS doesn't do anonuid | ||
- name: volume-mount-ownership-fix | ||
image: busybox | ||
command: | ||
[ | ||
"sh", | ||
"-c", | ||
"id && chown 1000:1000 /home/jovyan /home/jovyan/shared && ls -lhd /home/jovyan", | ||
] | ||
securityContext: | ||
runAsUser: 0 | ||
volumeMounts: | ||
- name: home | ||
mountPath: /home/jovyan | ||
subPath: "{username}" | ||
- name: home | ||
mountPath: /home/jovyan/shared | ||
subPath: _shared |
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.
This part can go after #1591 gets merged, right?
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.
@GeorgianaElena yep! I just merged that, will rebase and get rid of this!
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.
Removed!
# Expllicitly unset mem_limit, so it overrides the default memory limit we set in | ||
# basehub/values.yaml | ||
mem_limit: null |
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.
So what this means is that there a mem guarantee, but no upper limit, right? Can I ask what's the reasoning behind this? (I'm planning to open an issue about resource allocation best practices and I'm gathering info 😅 )
I'm gonna work on getting this merged! Things to do:
|
d9718f4
to
d922e97
Compare
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.
I have a small suggestion regarding CLI arguments. Also, the deployer's README should be updated
deployer/cli.py
Outdated
generate_cluster_parser.add_argument( | ||
"--cloud-provider", | ||
choices=["aws"], | ||
help="Which cloud provider to generate a cluster for", | ||
) |
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.
I would make this a required argument, rather than optional, because we will always have to specify it and typing out --cloud-provider
would get repetitive
|
||
```bash | ||
sops --in-place --encrypt eksctl/ssh-keys/<cluster-name>.key | ||
python3 deployer generate-cluster --cloud-provider aws <cluster-name> |
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.
See above suggestion to make --cloud-provider
a required arg
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.
@sgibson91 done!
@yuvipanda, do you think we can get this one merged soon (once the review comments and the conflict are fixed)? |
@damianavila yes i'll try to get that done asap. |
Is this PR ready to be merged? I believe this is the last step to close this issue. |
There are still some comments to be addressed and branch conflicts to be resolved before merging it. |
@yuvipanda confirmed to me he is going to take a look at the provided feedback and merge this one soon (most likely early next week). |
- Also add a generate-cluster command to our deployer that generates the jsonnet & tfvars files from a template Ref 2i2c-org#1440
Turns out there is literally one thing that's valid JSON but not YAML - hard tabs. Guess what golang's JSON outputter loves?!
Co-authored-by: Sarah Gibson <[email protected]>
We use jupyterhub-roothooks in the container to drop privileges after starting as root and running s3fs mounting code.
Per request!
Matches what we pass in allowed_organizations in GitHubOAuthenticator
d922e97
to
fe1c9ea
Compare
Co-authored-by: Sarah Gibson <[email protected]>
I've made all the changes suggested by @sgibson91, so gonna merge this one (and immediately decom it). Apologies everyone that this took so long! |
🎉🎉🎉🎉 Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/3161567755 |
the jsonnet & tfvars files from a template
Ref #1440