first try at storageclasses get and promises

[shalberd]

first try at storageclasses get and promises
early stage ... runs and build without errors

related to #1701

Description

trying to get all storageclasses via api call, working nicely.

never mind any 404 on /notebooks routes in screenshots, for local tests together with remote AWS Rosa cluster, I did not bother to install kubeflow and odh notebook controller, which create the routes from kind: Notebook as well as the CRD for Notebook, as part of ODH on my test Rosa cluster. PVC creation by ODH dashboard can be checked with that setup, too.

Q: @andrewballantyne since storage classes of the cluster are not namespace-specific, would it make sense to put the cluster storage classes into the React JS appContext instead of the ProjectDetailsContext? I don't think we need to watch the cluster storage classes for changes, it is enough to load them at application startup or on hard browser refreshs

I did just that in this PR.

How Has This Been Tested?

npm run-script dev

Test Impact

Request review criteria:

Self checklist (all need to be checked):

• The developer has manually tested the changes and verified that the changes work
• Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

• 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 (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

• The developer has tested their solution on a cluster by using the image produced by the PR to main

[openshift-ci]

Hi @shalberd. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[manaswinidas]

/ok-to-test

[shalberd]

tests with a Rosa cluster and two storage classes worked.
Worked though all cases, behavior as described:

List all storage classes (all users should have access RBAC wise)
Look for a default storage class
    if one is found, do nothing (continue with the flow today)
    If one is not found, read the spec.notebookController.storageClassName value
        if set & in the list, use it on the name on the PVC
        if not set, do nothing (continue with the flow today)
        if set & not in the list, print an error to the dev console console.error(...) and do nothing else (continue with the flow      

today)

tested with an without default annotation set to true on a storageclass. Working beautifully in all cases.
Forgive the relatively verbose if logic once I got all the information needed in the code ... I think it is better readable when it's not too terse.

$ oc get storageclasses
NAME      PROVISIONER       RECLAIMPOLICY   VOLUMEBINDINGMODE      ALLOWVOLUMEEXPANSION   AGE
gp2-csi   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   17h
gp3-csi   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   17h

On every moving-in from other parts of the application to the New Workbench page, the api call logic for getting the storageclasses is executed exactly once.

When no default storage class is set and the name from notebookController.storageClassName is not in the list of StorageClasses, the console error message appears and no storageClassName field is applied in the PVC.

To be looked at:

with dashboard running locally and communicating with an AWS Rosa Cluster, it takes about 2-3 seconds on first visit of the workbench / SpawnerFooter page for the list of storageClasses to be complete. Once done, though, no more API calls happen, so ok in terms of cluster traffic. For the console statements, there seems to be a 1-2 seconds tick interval, they keep repeating each other.

[shalberd]

@andrewballantyne moved logic for getting the cluster's storageclasses from SpawnerFooter to App / AppContext, thereby making the list of storageclasses available across the board, not just in Data Science Projects New Workbench.

List of storageclasses including their details is only fetched once at app start or on browser refresh.

Test result still the same from manual tests, verified

• if putting in a notebook.storageClassName that is not in the list of cluster storage classes and if number of cluster-level default storage classes is 0, then no storageClassName field is put in PVC. Also, a browser console error message is printed.

(base) svens-mbp-2:odh-dashboard sthoms$ oc get odhdashboardconfig odh-dashboard-config -o yaml | grep -A5 
  name: odh-dashboard-config
  namespace: opendatahub
  resourceVersion: "281407"
--
  notebookController:
    enabled: true
    notebookNamespace: opendatahub
    storageClassName: gp2-csibla
  notebookSizes:
  - name: Small
  
(base) svens-mbp-2:odh-dashboard sthoms$ oc get storageclasses
NAME      PROVISIONER       RECLAIMPOLICY   VOLUMEBINDINGMODE      ALLOWVOLUMEEXPANSION   AGE
gp2-csi   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   44h
gp3-csi   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   44h

kind: PersistentVolumeClaim
apiVersion: v1
metadata:
  annotations:
    openshift.io/description: ''
    openshift.io/display-name: 09071_1
  resourceVersion: '714435'
  name: 090711
  uid: 5e3e6274-4155-4b2e-87fc-990738adb232
  creationTimestamp: '2023-09-07T18:44:52Z'
  managedFields:
    - manager: unknown
      operation: Update
      apiVersion: v1
      time: '2023-09-07T18:44:52Z'
      fieldsType: FieldsV1
      fieldsV1:
        'f:metadata':
          'f:annotations':
            .: {}
            'f:openshift.io/description': {}
            'f:openshift.io/display-name': {}
          'f:labels':
            .: {}
            'f:opendatahub.io/dashboard': {}
        'f:spec':
          'f:accessModes': {}
          'f:resources':
            'f:requests':
              .: {}
              'f:storage': {}
          'f:volumeMode': {}
  namespace: odhds
  finalizers:
    - kubernetes.io/pvc-protection
  labels:
    opendatahub.io/dashboard: 'true'
spec:
  accessModes:
    - ReadWriteOnce
  resources:
    requests:
      storage: 20Gi
  volumeMode: Filesystem
status:
  phase: Pending

• if OdhDashboardConfig notebook.storageClassName is undefined / empty string fallback / not set && if number of cluster-level default storage classes is 0, then no storageClassName field is put in PVC. No further browser console log entry.

(base) Svens-MacBook-Pro-2:odh-dashboard sthoms$ oc get odhdashboardconfig odh-dashboard-config -o yaml | grep -i storageClassName
(base) Svens-MacBook-Pro-2:odh-dashboard sthoms$ oc get storageclasses
NAME      PROVISIONER       RECLAIMPOLICY   VOLUMEBINDINGMODE      ALLOWVOLUMEEXPANSION   AGE
gp2-csi   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   2d9h
gp3-csi   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   2d9h

PVC is created without storageClassName, which in this scenario leads to silent failure of the PVC not getting created, much like the first case, but with no console log entry.

(base) Svens-MacBook-Pro-2:odh-dashboard sthoms$ oc get pvc no-clus-default-no-config-entry -n odhds -o yaml | grep storageClassName
(base) Svens-MacBook-Pro-2:odh-dashboard sthoms$ oc get pvc no-clus-default-no-config-entry -n odhds -o yaml
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  annotations:
    openshift.io/description: ""
    openshift.io/display-name: no-clus-default-no-config-entry
  creationTimestamp: "2023-09-08T07:38:34Z"
  finalizers:
  - kubernetes.io/pvc-protection
  labels:
    opendatahub.io/dashboard: "true"
  name: no-clus-default-no-config-entry
  namespace: odhds
  resourceVersion: "918449"
  uid: 51954790-de87-4361-9ff7-2323b9900641
spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 20Gi
  volumeMode: Filesystem
status:
  phase: Pending

• if a default storage class name is set at cluster level, then that that one is used for storageClassName field of PVC.

(base) svens-mbp-2:odh-dashboard sthoms$ oc patch storageclass gp3-csi  -p '{"metadata": {"annotations":{"storageclass.kubernetes.io/is-default-class":"true"}}}'
storageclass.storage.k8s.io/gp3-csi patched
(base) svens-mbp-2:odh-dashboard sthoms$ oc get storageclasses
NAME                PROVISIONER       RECLAIMPOLICY   VOLUMEBINDINGMODE      ALLOWVOLUMEEXPANSION   AGE
gp2-csi             ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   45h
gp3-csi (default)   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   45h

note I had to do a browser refresh for AppContext and the info on StorageClasses to be reloaded.

(base) svens-mbp-2:odh-dashboard sthoms$ oc get pvc clusterdefaultscset -n odhds -o yaml | grep storageClassName
  storageClassName: gp3-csi

• if no default storageclass is set at cluster level and if a notebookController.storageClassName is set that is also in the array of cluster StorageClasses in AppContext, then that one is used for storageClassName field of PVC.

(base) svens-mbp-2:odh-dashboard sthoms$ oc get storageclasses
NAME      PROVISIONER       RECLAIMPOLICY   VOLUMEBINDINGMODE      ALLOWVOLUMEEXPANSION   AGE
gp2-csi   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   45h
gp3-csi   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   45h

  creationTimestamp: "2023-09-05T22:39:49Z"
  generation: 6
  name: odh-dashboard-config
  namespace: opendatahub
  resourceVersion: "727770"
--
  notebookController:
    enabled: true
    notebookNamespace: opendatahub
    storageClassName: gp2-csi
  notebookSizes:
  - name: Small

Here, too, as I patched the storageclass to be non-default at cluster-level, I had to do a browser refresh to get a fresh list of all StorageClassKind array members in AppContext.

Worked as expected as well.

(base) svens-mbp-2:odh-dashboard sthoms$ oc get pvc gp2csi-no-cluster-default -n odhds -o yaml | grep storageClassName
  storageClassName: gp2-csi

For now, the decision logic is only placed in the assemblePVC logic of new frontend workbenches, not used in jupyter tile.
https://github.com/opendatahub-io/odh-dashboard/pull/1740/files#diff-3001e38aa8502c4ea3b2f89b139357b16f89bb7402ebe7ceef988d8ca1a39878R57

[shalberd]

Briefly after loading the app / refreshing the web page, the following message appears very briefly and then disappears

I am checking whether the number of storage classes in the array fetched is 0 and then showing this message. It could be that it takes 1-2 seconds for the call to api storageclasses get to complete (async logic, promise-based).

So maybe that error message is not even necessary, also in terms of what you all would require. I can remove that GUI message part, if so wished. It wasn't part of Andrew's original ticket scope.

https://github.com/opendatahub-io/odh-dashboard/pull/1740/files#diff-00493e7d46c535f8351491dad02492847e6507297378cb46baf46937c6aca436R43

Also, on hard browser refresh and on first app load, the get request to api storageClasses shows up twice in the backend, a few seconds after each other, but then no more during the entire app lifecycle

req-b3 not completed on first call, called second time as req-b4 which is then completed

[09:54:25.888] INFO (5257): Making GET proxy request to https://api.svenstest.0i04.p3.openshiftapps.com:443/apis/storage.k8s.io/v1/storageclasses
[09:54:25.890] INFO (5257): incoming request
    reqId: "req-b3"
    req: {
      "method": "GET",
      "url": "/api/status",
      "hostname": "localhost:4010",
      "remoteAddress": "127.0.0.1",
      "remotePort": 55536
    }

[09:54:27.224] INFO (5257): incoming request
    reqId: "req-b4"
    req: {
      "method": "GET",
      "url": "/api/k8s/apis/storage.k8s.io/v1/storageclasses",
      "hostname": "localhost:4010",
      "remoteAddress": "127.0.0.1",
      "remotePort": 55553
    }
...
[09:54:27.225] INFO (5257): Making GET proxy request to https://api.svenstest.0i04.p3.openshiftapps.com:443/apis/storage.k8s.io/v1/storageclasses
[09:54:27.560] INFO (5257): Successful request, returning data to caller.
[09:54:27.560] INFO (5257): request completed
    reqId: "req-b4"
    res: {
      "statusCode": 200
    }
    responseTime: 335.3531400002539
[09:54:27.784] INFO (5257): request completed
    reqId: "req-b0"
    res: {
      "statusCode": 200
    }
    responseTime: 1898.3208109997213

[andrewballantyne]

Sorry for the prolonged delay @shalberd -- I got to this finally. I'll prioritize feedback cycles as this was smaller than I expected 😅

[shalberd]

will be testing the new changes and behavior with ROSA OpenShift: 4.12.34
again using all three scenarios and their impact on PVC creation

• default storage class at cluster level set - no storageClassName set in PVC, implicitly handled by cluster
• no default storage class set at cluster level, and notebookController.storageClassName not found in array of cluster storage classes - no storageClassName set in PVC and concole.error message in browser
• no default storage class set at cluster level, but notebookController.storageClassName found in array of cluster storage classes - so use this storageclass's name as pvc storageClassName

[shalberd]

• scenario default storage class set at cluster-level, name gp3, no notebookcontroller.storageClassName value

$ oc get storageclasses
NAME            PROVISIONER             RECLAIMPOLICY   VOLUMEBINDINGMODE      ALLOWVOLUMEEXPANSION   AGE
gp2             kubernetes.io/aws-ebs   Delete          WaitForFirstConsumer   true                   7h36m
gp2-csi         ebs.csi.aws.com         Delete          WaitForFirstConsumer   true                   7h32m
gp3 (default)   ebs.csi.aws.com         Delete          WaitForFirstConsumer   true                   7h36m
gp3-csi         ebs.csi.aws.com         Delete          WaitForFirstConsumer   true                   7h32m`

storageclassname gp3 automatically filled-in by cluster in pvc

$ oc get pvc -n odhds -o yaml
apiVersion: v1
items:
- apiVersion: v1
  kind: PersistentVolumeClaim
  metadata:
    annotations:
      openshift.io/description: ""
      openshift.io/display-name: workbenchwclusterdefaultstorageclasspvc
    creationTimestamp: "2023-09-30T05:51:07Z"
    finalizers:
    - kubernetes.io/pvc-protection
    labels:
      opendatahub.io/dashboard: "true"
    name: workbenchwclusterdefaultstorageclasspvc
    namespace: odhds
    resourceVersion: "293114"
    uid: c99e5b60-6e4b-4713-89a4-32afb0782bd4
  spec:
    accessModes:
    - ReadWriteOnce
    resources:
      requests:
        storage: 20Gi
    storageClassName: gp3
    volumeMode: Filesystem

• scenario default storage class set at cluster-level, name gp3, a different notebookcontroller.storageClassName value set, gp2, which is in list of cluster storage classes
  expected behavior: still cluster-set default storageClassName should be set gp3

$ oc get storageclasses | grep default
gp3 (default)   ebs.csi.aws.com         Delete          WaitForFirstConsumer   true                   7h59m

$ oc get  odhdashboardconfig odh-dashboard-config -n opendatahub -o yaml  | grep storageClassName
    storageClassName: gp2

$ oc get pvc defaultclusterscandvaluesetindashboardconfig -n odhds -o yaml
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  annotations:
    openshift.io/description: ""
    openshift.io/display-name: defaultclusterscandvaluesetindashboardconfig
  creationTimestamp: "2023-09-30T06:02:15Z"
  finalizers:
  - kubernetes.io/pvc-protection
  labels:
    opendatahub.io/dashboard: "true"
  name: defaultclusterscandvaluesetindashboardconfig
  namespace: odhds
  resourceVersion: "299212"
  uid: 62e78872-27b3-41c0-a3ae-cc7a1140b1d4
spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 20Gi
  storageClassName: gp3
  volumeMode: Filesystem

test ok, expected as specified, default cluster storage class is what counts and overrules all, if set

current logic:

(using the exit-early strat of functions) Logic can be something like:
    Do we have a default? yes, return undefined
    Do we have a storageClass value in our DashboardConfig? no, return undefined
    Do we have a matching storageClass to the value in the DashboardConfig? no, console.error & return undefined
    Return the Storage Class value you have matching (this allows flexibility of usage, we can determine at this point that we only want the metadata.name if we want)

@andrewballantyne We might talk about whether that is really wished, i.e. should odhDashBoardConfig storageClassName, if set, be able to override cluster default storageClass or not.

Logic in https://github.com/opendatahub-io/odh-dashboard/pull/1740/files#diff-919e2df35bd1299179bab541aa9609ac0dbf6bde2e5dcc0f70849180ecebec63R21

basically, undefined is always returned when there is a cluster-level default storageClass set, leading to the cluster filling in its default storageClassName during PVC creation. notebookController.storageClassName currently cannot override the cluster default.

@guimou once mentioned elsewhere and it went into P0 requirement of story

Must be able to configure a 'default' storage for RHODS. This does not have to be the OpenShift default.

my suggestion to achieve that would be a logic manupulation to

if (defaultClusterStorageClasses.length != 0 && configStorageClassName == '') {
    return undefined;
}

Usually, application-set values are allowed to override cluster-level defaults. Simplest example: resource requests and limits in application container deployments :-)

• scenario no default storage class set at cluster-level, and notebookcontroller.storageClassName not in list of cluster storage classes
  expected behavior: console.error entry in browser, event in storageclass
  refreshing application web page so that new details on cluster-level storageclasses get loaded into appContext

unsetting cluster-level default storageclass

$ oc get storageclasses | grep default
gp3 (default)   ebs.csi.aws.com         Delete          WaitForFirstConsumer   true                   8h

$ oc patch storageclass gp3 -p '{"metadata": {"annotations": {"storageclass.kubernetes.io/is-default-class": "false"}}}'
storageclass.storage.k8s.io/gp3 patched

$ oc get storageclasses | grep default

$ oc get  odhdashboardconfig odh-dashboard-config -n opendatahub -o yaml  | grep storageClassName
    storageClassName: gp-2gibberish

important: hitting enter on odh-dashboard application url, GET, application web page so that new details on cluster-level storageclasses get loaded into appContext (not just page refresh) so that api call to get the storageClasses into AppContext happens

$ oc get pvc noclusterdefaultscandodhdashboardscnotinclusterscs  -n odhds -o yaml
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  annotations:
    openshift.io/description: ""
    openshift.io/display-name: noclusterdefaultscandodhdashboardscnotinclusterscs
  creationTimestamp: "2023-09-30T06:19:44Z"
  finalizers:
  - kubernetes.io/pvc-protection
  labels:
    opendatahub.io/dashboard: "true"
  name: noclusterdefaultscandodhdashboardscnotinclusterscs
  namespace: odhds
  resourceVersion: "308636"
  uid: 130a57b1-1c81-4d98-921d-e5e68b180867
spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 20Gi
  volumeMode: Filesystem

We see no explicit storageClassName set, should lead to an event in openshift

 oc get events -n odhds  | grep noclusterdefaultscandodhdashboardscnotinclusterscs
14s         Normal   FailedBinding          persistentvolumeclaim/noclusterdefaultscandodhdashboardscnotinclusterscs   no persistent volumes available for this claim and no storage class is set

behavior ok and just as expected. To be determined: why the console.error message appears multiple times and why it already appears before Create Workbech button is clicked, before PVC is even assembled. ok for me, just wondering in terms of application logic flow. It looks like the hook usePreferredStorageClass.ts is called at page load time, correct? What I don't understand is why the console message repeats itself.

• last scenario: no cluster-level default storageClassSet, and notebookController.storageClassName is set and in list of cluster storage classes

$ oc get storageclasses
NAME      PROVISIONER             RECLAIMPOLICY   VOLUMEBINDINGMODE      ALLOWVOLUMEEXPANSION   AGE
gp2       kubernetes.io/aws-ebs   Delete          WaitForFirstConsumer   true                   8h
gp2-csi   ebs.csi.aws.com         Delete          WaitForFirstConsumer   true                   8h
gp3       ebs.csi.aws.com         Delete          WaitForFirstConsumer   true                   8h
gp3-csi   ebs.csi.aws.com         Delete          WaitForFirstConsumer   true                   8h

$ oc get  odhdashboardconfig odh-dashboard-config -n opendatahub -o yaml  | grep storageClassName
    storageClassName: gp2

working as expected, gp2 set as storageClassName of PVC

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  annotations:
    openshift.io/description: ""
    openshift.io/display-name: sc-from-config-no-cluster-default-set
  creationTimestamp: "2023-09-30T07:12:44Z"
  finalizers:
  - kubernetes.io/pvc-protection
  labels:
    opendatahub.io/dashboard: "true"
  name: sc-from-config-no-cluster-default-set
  namespace: odhds
  resourceVersion: "337045"
  uid: 29bf9d1c-6790-4473-a117-d74bf4f27b4d
spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 20Gi
  storageClassName: gp2
  volumeMode: Filesystem

[andrewballantyne]

Let me test it out, these should be fixed, but I think given the scenario we can probably go ahead without them and swing back with another tidying up PR.

[andrewballantyne]

This works as expected (see my last review comments). I'm going to approve this and get it into incubation. We should look to address those items (I'll create a follow up).

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

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:

• OWNERS [andrewballantyne]

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

[shalberd]

I added some comments, the only one I 100% can agree with it the one regarding strict or loose equality comparison. I really think the other comments don't need to be adressed. See my comments in the code.

[andrewballantyne]

I added some comments, the only one I 100% can agree with it the one regarding strict or loose equality comparison. I really think the other comments don't need to be adressed. See my comments in the code.

Well you are looking at the wrong docs for .includes -- see my comment. #1740 (comment)

It will need to be fixed, but it's not the end of the world given the small coverage you do today.