opendatahub-io / odh-dashboard

Dashboard for ODH
Apache License 2.0
28 stars 160 forks source link

[KFNBC] Add support for groups config to ODH Dashboard Config CR #383

Closed anishasthana closed 2 years ago

anishasthana commented 2 years ago

Feature description

We need to update the ODHDashboardConfig CR to include support for groups configs and some jupyterhub-cfg flags.

In Downstream RHODS, we have a configmap with the following data section:

data:
  admin_groups: "group_one"
  allowed_groups: "group_two"

as well as

data:
  jupyterhub_config.py: ""

  jupyterhub_admins: "admin"
  gpu_mode: ""
  singleuser_pvc_size: 20Gi
  notebook_destination: <notebook_destination>
  culler_timeout: "31536000" # 1 year in seconds

I would update the CRD to look something like the following:

spec:
  dashboardConfig:
     ...
  groupsConfig:
    admin_groups: "<admin_groups>"   #Will we support multiple groups for admin and allowed?
    allowed_groups: "<allowed_groups>"
  notebookController:
    cullerTimeout: <timeout>
    defaultPersistentVolumeClaimSize: <size>
    notebookNamespace: rhods-notebooks
    ...

Similarly, we need to add support for default PVC size and culler time.

Describe alternatives you've considered

No response

Anything else?

No response

maroroman commented 2 years ago

FYI: The pvcSize value will be handled by this PR: https://github.com/opendatahub-io/odh-dashboard/pull/348 The cullerTmeout is done by this PR: https://github.com/opendatahub-io/odh-dashboard/pull/269 The namespace is done by this PR: https://github.com/opendatahub-io/odh-dashboard/pull/350

andrewballantyne commented 2 years ago

Is there a reason we are moving away from the existing ConfigMaps to putting it in ODH Dashboard Config? @samuelvl @cfchase @DaoDaoNoCode

Surely there is a limit to what the ODH Dashboard Config will manage -- eg. we moved env vars out of this into their own configmaps/secrets. Are we looking to have one stop shop for all configurations (that are not individual notebook directly related)?

andrewballantyne commented 2 years ago

If we want a one-stop shop to locate things that are general configs -- I recommend we do not add the content directly to the ODH Dashboard Config -- we can add just a "what ConfigMap" to look at for more information. I'm sensing a slippery slope here for configuring the dashboard

andrewballantyne commented 2 years ago

https://github.com/opendatahub-io/odh-dashboard/issues/387 will address my comments above.

andrewballantyne commented 2 years ago

@anishasthana How important is it for us to mirror those names perfectly?

spec:
  dashboardConfig:
     ...
  groupsConfig:
    admin_groups: "<admin_groups>"   #Will we support multiple groups for admin and allowed?
    allowed_groups: "<allowed_groups>"
  notebookController:
    cullerTimeout: <timeout>
    defaultPersistentVolumeClaimSize: <size>
    notebookNamespace: rhods-notebooks
    ...

This is not currently what our CR looks like (even with the on-going PR). notebookNamespace is there, but we have pvcSize (instead of defaultPersistentVolumeClaimSize) and no cullingTimeout in the CR. cullingTimeout is stored in a ConfigMap (notebook-controller-culler-config).

Is it just important to know this information or do we need to be consistent for migration or training or something?

anishasthana commented 2 years ago

I picked the names based on what I thought made sense as I was working on the migration script -- I'm fine with whatever names you folks like.

re: cullingTimeout, I thought the plan was for that to go in the CR as well. Could you point me at information on the culler config configmap? The migration script will need to be updated to account for it.

andrewballantyne commented 2 years ago

@anishasthana The name of the configmap is here

The uses are:

samuelvl commented 2 years ago

@anishasthana Here you have the docs for notebook controller culling https://github.com/red-hat-data-services/odh-manifests/tree/master/odh-notebook-controller#notebook-culling

If JH culling is enabled, then create the following configmap in redhat-ods-applications namespace:

apiVersion: v1
kind: ConfigMap
metadata:
  name: notebook-controller-culler-config
data:
  ENABLE_CULLING: "true"
  CULL_IDLE_TIME: "X" # <---- Change with JH value (in minutes)
  IDLENESS_CHECK_PERIOD: "1"

Finally, restart the notebook controller (only kf-notebook-controller) to pick the configmap values:

$ oc rollout restart deploy/notebook-controller-deployment

We are not adding the cullerTimeout value to the dashboard CR.

anishasthana commented 2 years ago

"If culling is enabled" basically if the old configuration has a non zero value for timeout?

anishasthana commented 2 years ago

Any update on the groups config stuff folks?

andrewballantyne commented 2 years ago

As in any update on the https://github.com/opendatahub-io/odh-dashboard/pull/385? It's being reviewed. But looking almost ready to merge.

samuelvl commented 2 years ago

"If culling is enabled" basically if the old configuration has a non zero value for timeout?

@anishasthana This is the configuration by default (culling disabled):

$ oc get cm jupyterhub-cfg -o yaml                   
apiVersion: v1
data:
  culler_timeout: "31536000"
  gpu_mode: ""
  jupyterhub_admins: admin
  jupyterhub_config.py: ""
  notebook_destination: rhods-notebooks
  singleuser_pvc_size: 20Gi

Culling is enabled if the culler_timeout field is not set to 31536000. @maroroman Can you confirm this?

maroroman commented 2 years ago

"If culling is enabled" basically if the old configuration has a non zero value for timeout?

@anishasthana This is the configuration by default (culling disabled):

$ oc get cm jupyterhub-cfg -o yaml                   
apiVersion: v1
data:
  culler_timeout: "31536000"
  gpu_mode: ""
  jupyterhub_admins: admin
  jupyterhub_config.py: ""
  notebook_destination: rhods-notebooks
  singleuser_pvc_size: 20Gi

Culling is enabled if the culler_timeout field is not set to 31536000. @maroroman Can you confirm this?

Yes, I tried to keep backwards compatibility by taking the Default value for "disabled" culling from the jh culler