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: cache amalthea sessions #1983

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Conversation

olevski
Copy link
Member

@olevski olevski commented Sep 17, 2024

Adapts the k8s watcher to cache the new amalthea sessions CRD.

/deploy

@olevski
Copy link
Member Author

olevski commented Sep 17, 2024

This change is part of the following stack:

Change managed by git-spice.

Panaetius
Panaetius previously approved these changes Sep 18, 2024
@olevski olevski force-pushed the feat-jupyter-free-sessions branch from 3e9cb0e to 7dc4049 Compare September 27, 2024 14:28
Base automatically changed from feat-jupyter-free-sessions to master October 2, 2024 12:28
@olevski olevski dismissed Panaetius’s stale review October 2, 2024 12:28

The base branch was changed.

@olevski olevski force-pushed the feat-add-amalthea-session-cache branch from def99a7 to b13ad8c Compare October 2, 2024 12:28
@olevski olevski requested a review from Panaetius October 2, 2024 12:47
leafty
leafty previously approved these changes Oct 2, 2024
Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

Code looks good. Small suggestions here:

k8s-watcher/cache_collection.go Outdated Show resolved Hide resolved
Comment on lines 11 to 21
s.router.HandlerFunc("GET", "/servers", s.jsGetAll)
s.router.HandlerFunc("GET", "/servers/:serverID", s.jsGetOne)
s.router.HandlerFunc("GET", "/users/:userID/servers", s.jsUserID)
s.router.HandlerFunc("GET", "/users/:userID/servers/:serverID", s.jsUserIDServerID)
s.router.HandlerFunc("GET", "/sessions", s.asGetAll)
s.router.HandlerFunc("GET", "/sessions/:serverID", s.asGetOne)
s.router.HandlerFunc("GET", "/users/:userID/sessions", s.asUserID)
s.router.HandlerFunc("GET", "/users/:userID/sessions/:serverID", s.asUserIDServerID)
Copy link
Member

Choose a reason for hiding this comment

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

Note: the routing scheme may be confusing 😅 .

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 agree. But I was thinking that we will purge the old notebooks endpoints soon and after that it will be better. I can add a comment to explain the difference between server/sessions.

Copy link
Member

Choose a reason for hiding this comment

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

"Soon" will not be soon. Renku 1.0 will have to be supported for a long time still.

Adapts the k8s watcher to cache the new amalthea sessions CRD.
@RenkuBot
Copy link
Contributor

RenkuBot commented Oct 3, 2024

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

@olevski
Copy link
Member Author

olevski commented Oct 10, 2024

This PR needed an update to the RBAC for the k8s watcher and notebooks. This has been done already and released. So the PR can be merged.

This is the commit that added RBAC: SwissDataScienceCenter/renku@dfbb191

It has been released in renku 0.58.0

@olevski olevski merged commit 7f320d7 into master Oct 10, 2024
32 of 33 checks passed
@olevski olevski deleted the feat-add-amalthea-session-cache branch October 10, 2024 13:45
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.

4 participants