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

Add storageClassName field to dashboard config for notebook controller #1055

Merged

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Mar 30, 2023

Closes #1029

Description

Add storageClassName field under notebookController settings in dashboard config CR, which allows the user to specify the storageClass they want to use for the PVC of the Jupyter notebook they spawned with the notebook controller.

How Has This Been Tested?

  1. Edit odhDashboardConfig CR and add storageClassName as the storage class you want to set under the notebookController field
  2. Wait for 2 mins for the dashboard to read the new settings
  3. Go to the Jupyter tile to spawner a notebook (If you spawned one before, please delete the PVC of your notebook in the OpenShift console to make sure we can create a new PVC)
  4. Go to the OpenShift console to check the new PVC storageClassName, it should change to the one you set just now
  5. Remove the storageClassName in the dashboard config CR and wait for another 2 mins, and delete the PVC
  6. Stop the notebook and start it again
  7. Check the PVC and it should use the default storage class again

Test Impact

Tests are not applicable because there is no way for us to check the PVC storage class in the dashboard, you can only manually check it on the OpenShift console.

Request review criteria:

  • The commits have meaningful messages (squashes happen on merge by the bot).
  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

@openshift-ci openshift-ci bot requested review from alexcreasy and lucferbux March 30, 2023 08:49
@shalberd
Copy link
Contributor

Yes, that is very useful, as StorageClass default is not always set at a cluster-level and having the customizability / expressive functionality is a great idea.

@andrewballantyne
Copy link
Member

Yes, that is very useful, as StorageClass default is not always set at a cluster-level and having the customizability / expressive functionality is a great idea.

@shalberd This is also just our first pass at it -- this is to cover a gap we caused in the JupyterHub => Jupyter transition. Projects view will get a more robust version with admin options and everything at some point in the future (date TBD -- but you can keep your eyes on the ODH 2023 Goals if you want to see when we set things up).

Glad to hear you're also interested in storage classes -- we have had a lot of feedback about the desired nature to get more control in this area.

Copy link
Member

@Gkrumbach07 Gkrumbach07 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. lgtm

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

We missed updating all the types for Dashboard (silly dupes between backend and frontend).

This looks good and operates as expected -- only on create of PVC, modifications are not allowed after the fact.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 31, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, Gkrumbach07

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

@openshift-merge-robot openshift-merge-robot merged commit 74b408e into opendatahub-io:main Mar 31, 2023
@shalberd
Copy link
Contributor

shalberd commented Jun 3, 2023

@andrewballantyne the PVC is created with the correct storage class name in it, which is ok.

@DaoDaoNoCode
But at least in Jupyter tile notebook creation, the size is wrong in the PVC. Should that not be 20Gi?

spec:
  accessModes:
    - ReadWriteOnce
  resources:
    requests:
      storage: '20'
  storageClassName: netapp-file-eco-test
  volumeMode: Filesystem
status:
  phase: Pending

Events for that PVC show:

encountered error(s) in creating the volume: [Failed to create volume pvc-7235494c-8111-493b-a8f6-90eb4cf5a648 on storage pool file-eco-test_pool_0 from backend file-eco-ndr-ofp-test: unsupported capacity range; requested volume size (20 bytes) is too small; the minimum volume size is 20971520 bytes]

This error then leads to the notebook not being started correctly from Jupyter tile.

@andrewballantyne
Copy link
Member

Wow, impressive catch @shalberd -- how the heck did we make it this far from that code inception without seeing that. Yeah, 20 is 20 bytes. It's our code constant. You can work around this by using the OdhDashboardConfig and specifying a PVC size (or using the admin screen to set one).

Have you logged a bug about this? If not, I'd be happy to.

@shalberd
Copy link
Contributor

shalberd commented Jun 5, 2023

@andrewballantyne yes, I created a bug for that at #1333 Workaround done as you described. I suggested, cause that size is always needed, adding it in the code that creates a new odhdashboardconfig when none is present yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Add config in OdhDasboardConfig setting for notebookController for PVC Storage Type
5 participants