search

opendatahub-io / opendatahub-community

Apache License 2.0
26 stars 34 forks source link

Configurable Storage Class #56

Open jkoehler-redhat opened 1 year ago

jkoehler-redhat commented 1 year ago

This is a tracker for all the various bits we will need to track to complete the feature work to add ability to configure storage classes

Requirements @jedemo please add all the requirements here.

Add requirements

Individual Efforts

shalberd commented 1 year ago

Also a topic in odh data science pipelines operator There, not immediately related to dashboard, I think.

shalberd commented 1 year ago

the key thing to keep in mind that it is not at all unusual to have Openshift Clusters without any default storage class set at cluster-level.

a) So, having a way in Data Science Projects to set a default storageclass name via ODH dashboard config and odh dashboard config CR, evaluating that and setting that when creating the PVC, would be the first step.

b) A second step could be to list all storageclasses on workbench creation GUI.

@andrewballantyne @lucferbux However, given that data science projects PVC creation do not work at all on clusters without default storage class set in Openshift config, e.g. because the CSI driver does not allow it, would you agree that me making a PR for case a) first would be ok?

andrewballantyne commented 1 year ago

@shalberd We are aware of the limitations -- we were hoping to have avoided doing a hack to get around the default storage class issue for DS Projects -- but this feature got bumped a bit for other engagements. I'm a little weary of a "default storage" setting being done up today without properly following through -- as it will likely be throwaway code in its current form (likely we will merge this functionality in with this feature in the long run).

Since I don't really have an idea when we will be able to get to this feature among our other immediate objectives... I'm not "against" a PR to do move this along for the time being. Not sure what is the most future proof idea to setting this up though, since this feature is still in the pre-design stage (architecturally).

I'm taking it you have a more immediate need for a default storage class outside of the Jupyter Tile? I think we have a workaround due to JupyterHub's implementations.

shalberd commented 1 year ago

@andrewballantyne yes, I have an immediate or relatively immediate need for per-data-science-project, dynamically-sized PVCs with an explicit storageClassName. I have seen the UI and flow discussions. My suggestion would be to take the value from notebookController.storageClassName, if set.

   Must be able to configure storage classes other than the OpenShift default.
        Must be able to configure a 'default' storage for RHODS. This does not have to be the OpenShift default.
        Must be able to configure the name of the storage class as it should appear for users if exposed in the end user UI. Note: per Guillaume's comment below, the end user UI should show the admin configured display name as well as the real name.

Meaning, flow-wise, as un-hacky as possible ;-)

Background: Both default tag as well as description/display name are in the storageClass metadata annotations, if present https://docs.openshift.com/container-platform/4.10/post_installation_configuration/storage-configuration.html#storage-class-annotations_post-install-storage-configuration

A typical storageClass looks like this. Annotations are optional, not always present, i.e. on our cluster, showing this for completeness, though

kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: netapp-file-eco-ndr-level-test
  annotations:
    storageclass.kubernetes.io/is-default-class: "true"
    kubernetes.io/description: "This is a potentially very lengthy, optional description of the storage class and what is it about"
provisioner: csi.trident.netapp.io
parameters:
  selector: >-
    storageType=file;storageTier=eco;disasterRecovery=ndr;customer=level;environment=test
reclaimPolicy: Retain
allowVolumeExpansion: true
volumeBindingMode: Immediate

Also, interestingly, with oc get storageclass, the name column has (default) in case the default annotation is present. In future work, maybe once could make use of that to quickly get the default storageclass, if any. If I were using bash scripting, I'd just parse that or get that default one, if present, with oc get storageclasses -o jsonpath='{range .items[*].metadata}{.....

Bildschirmfoto 2023-08-24 um 21 30 58

I'd add a dashboard type storageClass with name, isDefault, description fields at the least. Will start with that. We can talk about whether the other fields are necessary app-wise. It could be informative, for example, to know whether a storage class uses file or block storage, but I am not sure how universal that selector is in the parameters.

odhStorageClasses array of type odhStorageClass, I don't see a display name per se, but an (optional) description definitely. Will make that part of the type. Later on, UI engineers could make that description hover over the selected item in the dropdown, for example.

Now, for my part, I could imagine doing anything that is not GUI-related, i.e. even the odhStorageClasses type array at typescript level. I just would not add a default-selected dropdown yet at workbench-level, so later on GUI engineering could expand on that functionality.

I also had a look at the k8sTypes at https://github.com/shalberd/odh-dashboard/blob/main/frontend/src/k8sTypes.ts#L231

The DisplayNameAnnotation don't work for PersistentVolumeClaimKind, like PVC has kubernetes.io/description key, not openshift.io/description and so on. However, I can fix that and make it work correctly, also it having a key storageclass.kubernetes.io/is-default-class in the annotations.

Would that be a feasible way for my PR? In short:

andrewballantyne commented 1 year ago

Spoke with Sven and have created https://github.com/opendatahub-io/odh-dashboard/issues/1701 to implement a minor version of this feature while we do other tasks.