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

feat(core): add support for ssh connections into sessions #3318

Merged
merged 22 commits into from Feb 24, 2023

Conversation

Panaetius
Copy link
Member

@Panaetius Panaetius commented Feb 14, 2023

closes #3293
closes #3289
closes #3290

/deploy #persist renku-notebooks=master extra-values=notebooks.ssh.enabled=true,notebooks.ssh.service.type=NodePort,notebooks.ssh.service.port=32023

@coveralls
Copy link
Collaborator

coveralls commented Feb 16, 2023

Pull Request Test Coverage Report for Build 4261590825

  • 359 of 434 (82.72%) changed or added relevant lines in 34 files are covered.
  • 34 unchanged lines in 13 files lost coverage.
  • Overall coverage increased (+0.3%) to 86.873%

Changes Missing Coverage Covered Lines Changed/Added Lines %
renku/command/format/session.py 1 2 50.0%
renku/core/session/utils.py 2 3 66.67%
renku/core/util/ssh.py 75 77 97.4%
renku/infrastructure/repository.py 4 6 66.67%
renku/ui/cli/session.py 24 26 92.31%
renku/ui/service/views/v1/templates.py 15 18 83.33%
renku/core/errors.py 5 9 55.56%
renku/core/session/session.py 53 57 92.98%
renku/domain_model/session.py 13 17 76.47%
renku/core/session/docker.py 10 16 62.5%
Files with Coverage Reduction New Missed Lines %
renku/core/dataset/dataset.py 1 91.18%
renku/core/login.py 1 83.97%
renku/core/template/template.py 1 87.02%
renku/core/util/tabulate.py 1 96.3%
renku/core/workflow/activity.py 1 93.89%
renku/domain_model/project_context.py 1 96.37%
renku/domain_model/workflow/composite_plan.py 1 81.55%
renku/ui/service/controllers/datasets_add_file.py 1 95.24%
renku/ui/service/jobs/contexts.py 1 73.33%
renku/version.py 2 75.0%
Totals Coverage Status
Change from base Build 4234036165: 0.3%
Covered Lines: 25427
Relevant Lines: 29269

💛 - Coveralls

@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 16, 2023 10:40 — with GitHub Actions Inactive
@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-rp-3318.dev.renku.ch

@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 16, 2023 12:13 — with GitHub Actions Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 17, 2023 09:14 — with GitHub Actions Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 17, 2023 11:12 — with GitHub Actions Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 17, 2023 12:09 — with GitHub Actions Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 17, 2023 14:27 — with GitHub Actions Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 20, 2023 15:29 — with GitHub Actions Inactive
@Panaetius
Copy link
Member Author

Basically we created the keypair and pushed - so now, by definition, the image is building - but we tell the user "image is not there, would you like to build it with Renkulab?"

No, when it asks you this, the keypair is added, but not pushed. So, by definition, it isn't building yet

This code is provider independent, so this code is executed with docker and renkulab sessions

Why can't I connect with ssh to a session that I started in the UI?

That's added now

this sort of hostname is... big! renku-ci-rp-3318.dev.renku.ch-test-session-ssh-rokroskar--test-2dsession-2dssh-5c3af687 Why can't we make it shorter? A config like this works fine:

The original thinking was that this way you can distinguish what deployment a session is on based on the name, but since most users won't be on more than 1 deployment anyways, that's now a hard requirement.
One technical reason is that we have automated garbage collection based on the connection name (removing old connection configs if the corresponding session isn't running anymore) and that uses the connection name to identify the connections and I didn't want to delete SSH connections from other deployments just because we can't find them on the current deployment. We could probably also do that through the filename instead of through the connection name.
It's a bit of a leftover from before I realized that Include ... works well with many files, when I didn't want to litter a users SSH config with too much stuff, so the Wildcard match entry deduplicated a lot of the clutter.
If we want to change this then I would just use the session ID as connection name directly instead of having two different identifiers.

@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 21, 2023 16:38 — with GitHub Actions Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 21, 2023 17:54 — with GitHub Actions Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 22, 2023 07:46 — with GitHub Actions Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 22, 2023 10:44 — with GitHub Actions Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 23, 2023 12:04 — with GitHub Actions Inactive
@Panaetius Panaetius marked this pull request as ready for review February 24, 2023 08:28
@Panaetius Panaetius requested a review from a team as a code owner February 24, 2023 08:28
@Panaetius Panaetius temporarily deployed to renku-ci-rp-3318 February 24, 2023 08:28 — with GitHub Actions Inactive
olevski
olevski previously approved these changes Feb 24, 2023
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

I found two questionable doc strings... 😆

renku/ui/cli/session.py Outdated Show resolved Hide resolved
renku/ui/cli/session.py Outdated Show resolved Hide resolved
@Panaetius Panaetius merged commit c024650 into develop Feb 24, 2023
@Panaetius Panaetius deleted the feature/ssh-into-sessions branch February 24, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
6 participants