-
Notifications
You must be signed in to change notification settings - Fork 511
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
[k8s] Share SSH jump pod #2826
[k8s] Share SSH jump pod #2826
Conversation
was causing concatentation issues
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.
Awesome, thanks @kbrgl! Took a skim through the code and it looks good. Left some nits. Will test it and approve if all is good.
Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>
Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>
Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>
Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>
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.
Thanks @kbrgl! Did some manual testing - seems to be working nicely. Backward compatibility has some issues, see comments.
Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>
Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>
1be67fc
to
e125c05
Compare
…ilot into kabir/issue-2598-jump # Conflicts: # sky/clouds/kubernetes.py # sky/templates/kubernetes-ray.yml.j2
…o kabir/issue-2598-jump # Conflicts: # sky/provision/kubernetes/instance.py # sky/provision/kubernetes/utils.py
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.
Thanks @kbrgl! Pushed some nits and merge conflict fixes. Manually tested backward compat, now running smoke tests.
Once everything is ready, will merge this PR and push new container images to our image registry.
…o kabir/issue-2598-jump
…o kabir/issue-2598-jump
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.
Thanks @kbrgl! Smoke tests pass. Manually tested backward compatibility. Building and updating our docker image, will merge after that's done.
|
This PR closes #2598 and adds plumbing to share the SSH jump pod across a Kubernetes namespace.
sky-ssh-key
, that is created once and patched when future attempts to setup Kubernetes authentication are made.cat /etc/secret-volume/ssh-key-* > ~/.ssh/authorized_keys
to set up the initial user's SSH key.cat /etc/secret-volume/ssh-key-* > ~/.ssh/authorized_keys
.The LCM script now uses 2 threads to poll: one to manage the lifecycle and terminate the jump pod, and one to reload keys on an interval.
Testing
Tested (run the relevant ones):
bash format.sh
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
bash tests/backward_compatibility_tests.sh
Tested locally.
cc @romilbhardwaj