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

Support Cloud Storage #1518

Merged
merged 9 commits into from
Mar 1, 2022
Merged

Support Cloud Storage #1518

merged 9 commits into from
Mar 1, 2022

Conversation

ciyer
Copy link
Contributor

@ciyer ciyer commented Oct 12, 2021

Add support to the UI for mounting cloud storage buckets.

The interaction with the backend still needs to be sorted out. Error handling/information is non-existent at the moment. See SwissDataScienceCenter/renku-notebooks#729.

TODO

  • use server_options to decide if the cloud storage is available or not

Screenshots

image

image

image

image

image

Testing

Things that should be tested

  • Enter just an endpoint or just a bucket name, that should not be allowed
  • Trying to use the same bucket name is used multiple times should be prevented

Follow-up

  • Validate cloud storage credentials before start: Currently, if the credentials are wrong, the session just does not come up and no explanation is given why. We should validate the credentials in the ui-server before start, so we can prevent this situation from arising.

Fix #1499

/deploy renku-notebooks=cloud-storage-naming-tweaks-for-testing #persist

@ciyer ciyer force-pushed the 1499-s3-support branch 3 times, most recently from 29e6da4 to eb6b5b1 Compare October 14, 2021 12:40
@RenkuBot
Copy link
Contributor

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

@ciyer ciyer changed the base branch from master to 1.0-next November 1, 2021 10:54
@ciyer ciyer force-pushed the 1.0-next branch 3 times, most recently from 3c60a3f to 1477658 Compare December 3, 2021 09:57
@ciyer ciyer temporarily deployed to renku-ci-ui-1518 February 16, 2022 10:33 Inactive
@olevski
Copy link
Member

olevski commented Feb 16, 2022

Btw after talking to Andrea it just occured to me that we should mention somewhere in the UI that:

  1. The buckets that you specify will be mounted under /cloudstorage/<bucket_name>
  2. If you mount multiple buckets their names have to be unique - among all buckets you specify (regardless of provider) the bucket names have to be unique. This is simply because you cannot have two mountpoint/folders with the same name under /cloudstorage where all the buckets are mounted

And one more thing that Andrea rightfully brought up. The error messages from the notebook service do not trickle down to the UI. This is not a new thing, or something that has been introduced with this PR. I think the best way forward here is to merge this PR - the S3 feature is off by default so we can decide to keep it off until we roll out proper error messaging.

Here is the PR that will standardize error messages in the notebook service: SwissDataScienceCenter/renku-notebooks#849. I just need to do a few changes/cleanup on it and we can merge this on the notebook side. But the API that the ui will use will not change at all with these edits that I mention. So if you want you can start to adapt the ui to this new format of error messages from the notebook service.

@ciyer
Copy link
Contributor Author

ciyer commented Feb 16, 2022

Btw after talking to Andrea it just occured to me that we should mention somewhere in the UI that:

  1. The buckets that you specify will be mounted under /cloudstorage/<bucket_name>
  2. If you mount multiple buckets their names have to be unique - among all buckets you specify (regardless of provider) the bucket names have to be unique. This is simply because you cannot have two mountpoint/folders with the same name under /cloudstorage where all the buckets are mounted

Those are good suggestions, I will add them to the UI.

And one more thing that Andrea rightfully brought up. The error messages from the notebook service do not trickle down to the UI. This is not a new thing, or something that has been introduced with this PR. I think the best way forward here is to merge this PR - the S3 feature is off by default so we can decide to keep it off until we roll out proper error messaging.

That makes sense. The other part of the solution is mentioned in the main text of the description above: we want to validate the credentials in the ui-server when the user clicks "Save." Then we can immediately provide feedback that the user can fix. But this PR is already big enough, and that feature can be added in later.

@ciyer ciyer temporarily deployed to renku-ci-ui-1518 February 17, 2022 17:22 Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-1518 February 18, 2022 06:41 Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-1518 February 18, 2022 06:46 Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-1518 February 18, 2022 16:55 Inactive
@ciyer ciyer requested a review from olevski February 28, 2022 09:04
@ciyer ciyer temporarily deployed to renku-ci-ui-1518 February 28, 2022 09:04 Inactive
Copy link
Member

@olevski olevski left a comment

Choose a reason for hiding this comment

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

@ciyer I only tested out launching a session and looked at how my previous two comments were addressed. So all of that is fine.

I did not review the code at all. Since I am not familiar with the code that all I will leave that to someone else from the UI team.

Copy link
Contributor

@andre-code andre-code left a comment

Choose a reason for hiding this comment

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

Great feature 🎉👏
My only minor suggestion would be to rename the session test file to newSession.spec.ts since there is another PR that is adding a session file to display the session list.

🚀

@ciyer
Copy link
Contributor Author

ciyer commented Mar 1, 2022

Great feature 🎉👏 My only minor suggestion would be to rename the session test file to newSession.spec.ts since there is another PR that is adding a session file to display the session list.

🚀

Sure, good idea -- maybe that rename can be in the other PR though. :)

@ciyer ciyer temporarily deployed to renku-ci-ui-1518 March 1, 2022 10:07 Inactive
@ciyer ciyer deployed to renku-ci-ui-1518 March 1, 2022 11:17 Active
@ciyer ciyer merged commit b054df5 into master Mar 1, 2022
@ciyer ciyer deleted the 1499-s3-support branch March 1, 2022 11:27
@RenkuBot
Copy link
Contributor

RenkuBot commented Mar 1, 2022

Tearing down the temporary RenkuLab deplyoment for this PR.

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.

UI for mounting S3 buckets
5 participants