-
Notifications
You must be signed in to change notification settings - Fork 2
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: use cloud storage from amalthea #387
Conversation
This change is part of the following stack: Change managed by git-spice. |
0e6daaf
to
7000553
Compare
4210ac6
to
9e7120a
Compare
You can access the deployment of this PR at https://renku-ci-ds-387.dev.renku.ch |
2cb2137
to
a42b430
Compare
a42b430
to
3594911
Compare
3594911
to
4cfd95a
Compare
4cfd95a
to
7d1d24c
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.
General Question: there's still #347 that does a bit of cleanup in cloud storage and probably will cause quite a bit of conflict with this, so I'm wondering what the best way would be for us to bring this together with your changes. There's also lots of changes @leafty is working on in regards to cloud storage, and 3 people working on the same bits of code at the same time is bound to cause issues. maybe we should sync?
name=None, | ||
) | ||
for s in body.cloudstorage or [] | ||
if s.storage_id is not None |
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.
question: historically, users could mount arbitrary, unsaved cloud storage and the UI would just post a body here, in which case it wouldn't have a storage_id. I don't think this is around in v2 but v1 might still use this. So I'm not entirely sure if this if statement will break things and we'd have to generate ad-hoc ids or if this is fine?
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.
For this first implementation of the cloud storage in V2 lets keep things simple and not allow people to mount cloud storage that is unsaved. Then if needed we can add this functionality. This also needs some design because in theory we could allow this in many other places in the API where we currently expect something that exists in the DB. So I will make the storage ID required in the post request to launch a session. If a payload is provided then it just overrides the cloud storage saved in our DB.
9941e20
to
9e88bad
Compare
e3ad92f
to
5328479
Compare
30923b5
to
a45bec2
Compare
Note: waiting for a rebase, so I can try to work from this branch. |
but the API is currently broken and that it works is mostly an accident, any update to sanic or one of its components could break it. So yes, it's a breaking API change but at the same time it's more of a bugfix and unbreaking the API. I wouldn't hold off on this. And it is needed for that PR. Also SwissDataScienceCenter/renku-ui#3299 addressed those changes on the UI AND addresses the changes to storage, so there's a bit of a triangle dependency here. |
Co-authored-by: Samuel Gaist <[email protected]> squashme: resolve package version conflicts
squashme: use the amalthea session cache
fa13825
to
bf8efeb
Compare
We have shared errors we raise everywhere which we should use not that we moved the notebooks code. And we should avoid using the notebooks errors that came with the old code. We will fully clean out these old error classes in some follow up work.
bf8efeb
to
04553ec
Compare
Tearing down the temporary RenkuLab deplyoment for this PR. |
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]>
Adds cloud storage for the new amalthea operator and with the new csi rclone version.
/deploy renku=feat-jupyter-free-sessions amalthea-sessions=feat-add-cloud-storage renku-notebooks=feat-jupyter-free-sessions renku-gateway=feat-add-renku-refresh-token-for-data-service renku-ui=andrea/custom-launcher-jupyter-free extra-values=amalthea-sessions.deployCrd=false,amalthea-sessions.rcloneStorageClass=csi-rclone-387-secret-annotation #notest