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

No Internal OCP registry would fail with current notebook setup. #112

Open
harshad16 opened this issue Jul 3, 2023 · 4 comments
Open

No Internal OCP registry would fail with current notebook setup. #112

harshad16 opened this issue Jul 3, 2023 · 4 comments
Labels
kind/bug Something isn't working priority/major Important issue that needs to be resolved asap.

Comments

@harshad16
Copy link
Member

For any non-master branch, referencePolicy of tags should be kept Local, though. Which it is not currently at branch v1.7.

You also have to differentiate between disconnected deployments with an internal openshift registry and without an openshift internal registry. If there is no internal openshift registry, then ImageContentSourcePolicy applies and it does not matter whether referencePolicy is set to Source or Local.

If there is an internal openshift registry, setting referencePolicy to Source only makes sense for the master branch, and even there, it only makes sense for the tags of an imagestream whose background image externally changes all the time. It does not make sense for stable, non-weekly images, unless you don't trust the cluster's image registry pruning settings.

Disconnected environments never directly pull from external locations.

Furthermore, imagePullPolicy of a container, especially when using digests, should always be set to IfNotPresent, leveraging the node image cache for a given image digest, or digest behind a tag.

Originally posted by @shalberd in opendatahub-io/odh-manifests#846 (comment)

@harshad16 harshad16 transferred this issue from opendatahub-io/odh-manifests Jul 3, 2023
@harshad16 harshad16 added the kind/bug Something isn't working label Jul 3, 2023
@shalberd
Copy link

shalberd commented Jul 5, 2023

Regarding imagePullPolicy:

For the ose-oauth-proxy-sidecar injected image, see odh notebook controller PR.

For the odh dashboard notebook container part, see odh dashboard PR.

For the odh-manifests part and imagePullPolicy: opendatahub-io/odh-manifests#868

@shalberd
Copy link

shalberd commented Jul 6, 2023

@harshad16 See also opendatahub-io/opendatahub-community#116 the two PRs would cover the issues at RHODS as well when they don't have an internal openshift registry. In that case, the external url would be put into the notebook container image field, regardless of whether referencePolicy is set to Source or Local, when no openshift registry is present.

@harshad16
Copy link
Member Author

Moving this over to the next sprint 1.32
for further investigation.

@harshad16 harshad16 added the priority/major Important issue that needs to be resolved asap. label Dec 7, 2023
@shalberd
Copy link

shalberd commented Jun 5, 2024

@harshad16 this issue can be closed.

solved with dashboard mods https://github.com/opendatahub-io/odh-dashboard/releases/tag/v2.23.0

and other mods to odh notebook controller delivered via / in ODH 2.13

@harshad16 @jiridanek @jstourac @atheo89 @lucferbux

notebook container image field value, as only updated in StatefulSet CRD, does not need to be excluded from reconciliation Notebook to StatefulSet anymore. We decided not to use the image change trigger annotation mechanism.

closing this issue here due to an alternative approach: odh odh notebook controller doing the notebook image field lookup in all cases directly in notebook image field spec (no openshift internal registry, openshift internal registry): #336 and #329 based on imagestream and tag status fields.

as well as dashboard changes which remove dependency on internal registry, supporting both use cases internal and external registry opendatahub-io/odh-dashboard#2867

Thank you all very much, neat work

minor remaining: changing notebook container imagePullPolicy: IfNotPresent from old imagePullPolicy: Always in a notebook CR podspec notebook container

as a nice-to-have feature for external registry and large images with a unique hash. Needs to be validated if that works with internal openshift registry, too. Saves times on workbench startup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working priority/major Important issue that needs to be resolved asap.
Projects
Status: 🔖Todo
Development

No branches or pull requests

2 participants