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

WIP: Populate Elyra runtime images configMap from runtime images imagestream manifests and mounts it on the Notebook CR #513

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atheo89
Copy link
Member

@atheo89 atheo89 commented Feb 20, 2025

Related to: https://issues.redhat.com/browse/RHOAIENG-19051

Related PRs for this workload:
New set of runtime ImageStreams: opendatahub-io/notebooks#888
Unwire .json files from the Datascience Notebook: opendatahub-io/notebooks#909

Description

This PR add a new notebook_runtime script where includes the logic to create/watch/update a new ConfigMap the pipeline-runtime-images from the runtime images imagesStreams (related PR) and mount it as volume with the name /opt/app-root/pipeline-runtimes/ on the notebook CR when is getting created.

Moreover, on this PR has been added the SET_RUNTIMES_CM DevFlag to enable/disable this feature.

TODO items:

  • Update/Write tests to cover the new changes
  • Update the GHA to update the runtimes SHAs to the new place
  • Remove from this PR Makefile and on the param.env changes

How Has This Been Tested?

NOTE: Please don't pay attention on the Makefile and on the param.env changes. I will revert them, they are there for testing purposes

  1. Update the RHOAI DSC with the following:
    workbenches:
      devFlags:
        manifests:
          - contextDir: components/odh-notebook-controller/config
            sourcePath: ''
            uri: 'https://github.com/atheo89/kubeflow/archive/rhoaieng-19050-configmap-generator.tar.gz'
          - contextDir: manifests
            sourcePath: ''
            uri: 'https://github.com/atheo89/notebooks/archive/rhoaieng-19050.tar.gz'
      managementState: Managed
      
  1. Import the quay.io/rh_ee_atheodor/workbench-images@sha256:f2bc105afab97575007a5743e39cc4c045fa276ba8612b6a8241758d48711913 notebook (Includes the changes from this PR) This is a notebook without the .json runtimes in it.

  2. Spin up this Custom Datascience notebook via UI
    Click on the left side menu and click this icon.
    image

You should see the runtimes mounted like below:
image

On the Back-end side:
4. Inspect the logs on the odh-notebook-controller to find the related logs on the configMap creation and the mounting, like below.

...
{"level":"info","ts":"2025-02-20T10:13:31Z","logger":"controllers.Notebook","msg":"Created new ConfigMap for runtime images","namespace":"atheo","ConfigMap.Name":"pipeline-runtime-images"}
...
{"level":"info","ts":"2025-02-20T10:13:31Z","logger":"controllers.Notebook","msg":"Injecting runtime-images volume into notebook","notebook":"t1","namespace":"atheo","notebook":"t1","namespace":"atheo"}
...

  1. Check on the notebook pod statefullset for the config map, press on it to inspect also the data from this configMap
    image

  2. Check as well on the notebook CR to evaluate the mounting, as is on the bellow image:
    image

image

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • 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

@openshift-ci openshift-ci bot requested review from caponetto and jstourac February 20, 2025 10:51
Copy link

openshift-ci bot commented Feb 20, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from atheo89. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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-ci openshift-ci bot added size/l and removed size/l labels Feb 20, 2025
@openshift-ci openshift-ci bot added size/l and removed size/l labels Feb 20, 2025
@atheo89
Copy link
Member Author

atheo89 commented Feb 20, 2025

/hold
Until investigate if we have any issue with notebook tile

@openshift-ci openshift-ci bot added do-not-merge/hold Do not merge this PR size/l and removed size/l labels Feb 20, 2025
@atheo89 atheo89 changed the title Populate Elyra runtime images configMap from runtime images imagestream manifests and mounts it on the Notebook CR WIP: Populate Elyra runtime images configMap from runtime images imagestream manifests and mounts it on the Notebook CR Feb 20, 2025
@atheo89 atheo89 force-pushed the rhoaieng-19050-configmap-generator branch from 1ad60c1 to 3402caf Compare February 20, 2025 15:12
@openshift-ci openshift-ci bot added size/l and removed size/l labels Feb 20, 2025
@atheo89 atheo89 force-pushed the rhoaieng-19050-configmap-generator branch from 3402caf to 9dc2dd8 Compare February 20, 2025 15:19
@openshift-ci openshift-ci bot added size/l and removed size/l labels Feb 20, 2025
@openshift-ci openshift-ci bot added size/l and removed size/l labels Feb 24, 2025
@atheo89 atheo89 force-pushed the rhoaieng-19050-configmap-generator branch from 3dbf072 to e48c239 Compare February 25, 2025 08:45
@openshift-ci openshift-ci bot added size/l and removed size/l labels Feb 25, 2025
@atheo89 atheo89 force-pushed the rhoaieng-19050-configmap-generator branch from 5d63864 to 89d996c Compare February 25, 2025 14:27
@openshift-ci openshift-ci bot added size/l and removed size/l labels Feb 25, 2025
@atheo89 atheo89 force-pushed the rhoaieng-19050-configmap-generator branch from 89d996c to a87eed0 Compare February 25, 2025 14:28
…watch/update a new ConfigMap the `pipeline-runtime-images` for runtime images and mount it as volume on the notebook when is getting created

Move ConfigMapName, mountPath and volumeName as global vars

Add DevFlag SET_RUNTIMES_CM to enable/diable the feature

Add test cases for the new configMap configuration

Remove devflag

Revert changes on makefile
@atheo89 atheo89 force-pushed the rhoaieng-19050-configmap-generator branch from a87eed0 to 999d76a Compare February 25, 2025 14:28
@openshift-ci openshift-ci bot added size/l and removed size/l labels Feb 25, 2025
Comment on lines +41 to +51
// Create a dynamic client
config, err := rest.InClusterConfig()
if err != nil {
log.Error(err, "Error creating cluster config")
return err
}
dynamicClient, err := dynamic.NewForConfig(config)
if err != nil {
log.Error(err, "Error creating dynamic client")
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Create a dynamic client
config, err := rest.InClusterConfig()
if err != nil {
log.Error(err, "Error creating cluster config")
return err
}
dynamicClient, err := dynamic.NewForConfig(config)
if err != nil {
log.Error(err, "Error creating dynamic client")
return err
}
// Create a dynamic client
dynamicClient, err := dynamic.NewForConfig(r.Config)
if err != nil {
log.Error(err, "Error creating dynamic client")
return err
}

@@ -60,6 +61,7 @@ type OpenshiftNotebookReconciler struct {
Namespace string
Scheme *runtime.Scheme
Log logr.Logger
Config *rest.Config
Copy link
Member

Choose a reason for hiding this comment

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

set this in main.go?

Config:    mgr.GetConfig(),

and also in suite_test.go for tests?

Copy link
Member

Choose a reason for hiding this comment

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

just a note that code in this file is not run in envtest, according to codecov

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you Jiri for your review! I will try to fix that, maybe i will reach you out to at some point to consult me with this new test framework.

Copy link

openshift-ci bot commented Feb 25, 2025

@atheo89: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/odh-notebook-controller-unit 999d76a link true /test odh-notebook-controller-unit
ci/prow/odh-notebook-controller-e2e 999d76a link true /test odh-notebook-controller-e2e

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@atheo89
Copy link
Member Author

atheo89 commented Feb 26, 2025

FYI folks I will close this PR and i will work for the tests in my fork branch in order to don't spam you all the time with notifications. 🤣

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.

4 participants