search

opendatahub-io / kubeflow

Machine Learning Toolkit for Kubernetes
Apache License 2.0
10 stars 32 forks source link

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

Open harshad16 opened 1 year ago

harshad16 commented 1 year ago

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 https://github.com/opendatahub-io/odh-manifests/issues/846#issuecomment-1617966813

shalberd commented 1 year ago

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: https://github.com/opendatahub-io/odh-manifests/pull/868

shalberd commented 1 year ago

@harshad16 See also https://github.com/opendatahub-io/opendatahub-community/issues/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 commented 1 year ago

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

shalberd commented 4 months ago

@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): https://github.com/opendatahub-io/kubeflow/pull/336 and https://github.com/opendatahub-io/kubeflow/pull/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 https://github.com/opendatahub-io/odh-dashboard/pull/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.