Navigation Menu

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

Volumes Management UI #5684

Merged

Conversation

kimwnasptd
Copy link
Member

This PR is for #4758
I don't want to close this issue as we can link other issues to it and have it as an umbrella one.

This web app is a slightly modified version from the one MiniKF has for the last year now. It should allow users to manage their lifecycle of their PVCs in the cluster via a UI. This is a basic UI that can act as a first iteration on how we manage our volumes/PVCs on Kubeflow.

It's structure is almost identical to our Jupyter web app and Tensorboards web app. It also utilizes the common code we have for both the frontend and the backend.

@elikatsis @StefanoFioravanzo feel free to take another look at it
/cc @elikatsis
/cc @StefanoFioravanzo
/assign @kimwnasptd

The next steps will be:

  • setting up the manifests for this app, cc @yanniszark
  • integrating it in the CentralDashboard side panel, cc @StefanoFioravanzo
  • adding it to our CI/CD

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimwnasptd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@davidspek
Copy link
Contributor

@kimwnasptd Do you have a link to a design doc for this? (I'm only partially joking).

@davidspek
Copy link
Contributor

@kimwnasptd I believe I noticed the PVCviewer resource only support Rok at the moment. Is that correct? Could you maybe quickly provide details what the current differences in capabilities are between Rok and other storage classes?

@davidspek
Copy link
Contributor

@kimwnasptd I think it is important that you rebase on master. It doesn't seem like 28af771 created a new image for central-dashboard because of this.

@davidspek
Copy link
Contributor

@kimwnasptd Could we have a quick chat on Slack about this PR sometime soon? I have some ideas for it that I would like to discuss.

@kimwnasptd
Copy link
Member Author

@davidspek thank you for taking a look and providing comments! I'll also address your other comments

@kimwnasptd Do you have a link to a design doc for this? (I'm only partially joking).

Unfortunately no, and we will need to fix this for all the other web apps we have as well after 1.3. Although we will need to first define our processes for how we will organize our design docs, have design proposals for new features etc. I'm working on some prototypes for these processes and will make some proposals once we get a breather from 1.3. Thanks for bringing it up!

@kimwnasptd I believe I noticed the PVCviewer resource only support Rok at the moment. Is that correct? Could you maybe quickly provide details what the current differences in capabilities are between Rok and other storage classes?

Indeed, this feature is only available in Rok right now so we couldn't integrate it in the default flavor of the app in this iteration. This functionality relies on some of Rok's functionalities and code, which we are evaluating if/how we could implement them in scope of Kubeflow.

@kimwnasptd I think it is important that you rebase on master. It doesn't seem like 28af771 created a new image for central-dashboard because of this.

Good point, I'll rebase my commits on top of the latest master

@kimwnasptd Could we have a quick chat on Slack about this PR sometime soon? I have some ideas for it that I would like to discuss.

Lets go over it in the following WG meetings, where we will also be discussing what features we could work on after 1.3. And then we can either create new issues for feature requests or add the discussed proposals in our tracking issue #4758

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
@kimwnasptd kimwnasptd force-pushed the feature-kimwnasptd-vwa-upstream branch from ef87a8d to 5ae90dc Compare March 16, 2021 13:18
@davidspek
Copy link
Contributor

@kimwnasptd I just realized the CI/CD for the volumes web app isn't in the prow config yet. Should I open a PR for that, then we can merge it quickly and rebase this PR again? Or do you want to add the CI/CD to this PR (and prow will pick it up here)?

@kimwnasptd
Copy link
Member Author

@kimwnasptd I just realized the CI/CD for the volumes web app isn't in the prow config yet. Should I open a PR for that, then we can merge it quickly and rebase this PR again? Or do you want to add the CI/CD to this PR (and prow will pick it up here)?

Yeap, I have it in my mind. I'll create the CI/CD PR later on after we merge this one.

@davidspek
Copy link
Contributor

If you do it after merging this PR the image won't be built and pushed. You'd then need to exploit the way the current setup works with another temporary PR that changes the Prow config again to have it build and push the image. I think it's better to merge the CI/CD PR before this one (and rebase this one after CI/CD has been merged).

I've been doing the CI/CD for all the notebook servers the past days so I have the copy/paste action in my muscle memory and can create the PR for this in a few minutes if you like.

@kimwnasptd
Copy link
Member Author

I think it's better to merge the CI/CD PR before this one (and rebase this one after CI/CD has been merged).

We can do that as well. Feel free to open the PR if you have the cycles

@davidspek
Copy link
Contributor

@kimwnasptd Created the PR.

@davidspek
Copy link
Contributor

@kimwnasptd #5702 with the CI/CD for the Volumes Web App was just merged. If you rebase this PR on master we should see it running the image building test with Argo.

@kimwnasptd
Copy link
Member Author

@kimwnasptd #5702 with the CI/CD for the Volumes Web App was just merged. If you rebase this PR on master we should see it running the image building test with Argo.

Nice! Lets see if @elikatsis and @StefanoFioravanzo have any other comments for the PR and I'll rebase on the latest master and then we can merge it

@StefanoFioravanzo
Copy link
Member

Thanks @kimwnasptd ! I don't have anything to add on top of David's comments
/lgtm

@google-oss-robot google-oss-robot merged commit ffae5dd into kubeflow:master Mar 18, 2021
Notebooks WG automation moved this from In progress to Done Mar 18, 2021
@kimwnasptd kimwnasptd deleted the feature-kimwnasptd-vwa-upstream branch May 19, 2021 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants