-
Notifications
You must be signed in to change notification settings - Fork 39
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
/hold |
1ad60c1
to
3402caf
Compare
3402caf
to
9dc2dd8
Compare
3dbf072
to
e48c239
Compare
5d63864
to
89d996c
Compare
89d996c
to
a87eed0
Compare
…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
a87eed0
to
999d76a
Compare
// 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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@atheo89: The following tests failed, say
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. |
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. 🤣 |
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:
Makefile
and on theparam.env
changesHow Has This Been Tested?
NOTE: Please don't pay attention on the
Makefile
and on theparam.env
changes. I will revert them, they are there for testing purposesImport 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.Spin up this

Custom Datascience
notebook via UIClick on the left side menu and click this icon.
You should see the runtimes mounted like below:

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.
Check on the notebook pod statefullset for the config map, press on it to inspect also the data from this configMap

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

Merge criteria: