Skip to content

KF Notebooks: Reconcile Helper overwrites or collides with image-field changes from image policy admission plugin #98

Open
@shalberd

Description

@shalberd

/kind bug

What steps did you take and what happened:
Applied a Notebook CR in conjunction with Notebook controller. Notebook controller creates a StatefulSet from the Notebook CR, containing all the specs needed for e.g. the container, including the image-field.
With DeploymentConfigs, Deployments, Pods, StatefulSets, Openshift has a mechanism called the Image Policy admission plugin, image.openshift.io/ImagePolicy, that resolves imagestreams to image-references in sha256 digest format, in that function being a mutating type of plugin.

Here is what I did:

  • For same-namespace tests, I added an imagestream reference and tag to the image-field of a Notebook CR. The statefulset contains the image-field, but reconciliation of the StatefulSet keeps overwriting the image-field, thus never resolving the actual image-location. Reconcile documentation on basics

https://github.com/kubeflow/kubeflow/blob/master/components/notebook-controller/controllers/notebook_controller.go#L90

Related discussion at openshift/openshift-apiserver#339 (comment)

What did you expect to happen:

I expected the image-field of the StatefulSet to contain the resolved image reference

spec:
      containers:
        - name: jupyter
          image: >-
            quay.io/thoth-station/s2i-generic-data-science-notebook@sha256:3f619c61501218f03d39d97541336dee024f446e64f3a47e2bc7e62cddeb2e58

instead the image-field stays at

          image: 's2i-generic-data-science-notebook:v0.0.5'

for StatefulSets created from the Notebook CR.

Anything else you would like to add:
Error occuring in context of Open Data Hub Notebooks controller context.
Reconcilehelper should exclude the image-field from reconciliation. Currently, the whole template.spec part of StatefulSets is synced as a whole.

https://github.com/kubeflow/kubeflow/blob/master/components/common/reconcilehelper/util.go#L131

  • For referencing and resolving image-values across namespaces, an even better way is to use an annotation on the StatefulSet

I have tested this within-namespace (Notebook / StatefulSet and imagestream in same namespace) and across-namespaces and it is working beautifully for StatefulSets:

https://docs.openshift.com/container-platform/4.10/openshift_images/triggering-updates-on-imagestream-changes.html#images-triggering-updates-imagestream-changes-kubernetes-about_triggering-updates-on-imagestream-changes

When one of the core Kubernetes resources contains both a pod template and this annotation, OpenShift Container Platform attempts to update the object by using the image currently associated with the image stream tag that is referenced by trigger. The update is performed against the fieldPath specified.

Example, having a StatefulSet in odhsven2, referring to an imagestream in odhsven namespace, just adding an annotation and leaving the image-field empty image: " " or leaving it out from the StatefulSet pre-apply automatically fills in the correct image value in the image-field on the container in the template of the StatefulSet.

The StatefulSet annotation is like this, in metadata:

  annotations:
    image.openshift.io/triggers: '[{"from":{"kind":"ImageStreamTag","name":"s2i-generic-data-science-notebook:v0.0.5", "namespace":"odhsven"},"fieldPath":"spec.template.spec.containers[?(@.name==\"jupyter-nb-sven\")].image"}]'

That annotation could be added to the StatefulSet ObjectMeta on creation by the controller, adding this optional metadata.annotations from the Notebook to the StatefulSet as well

https://github.com/kubeflow/kubeflow/blob/e0daf70f04616f0eae9353659d5616525f9999f4/components/notebook-controller/controllers/notebook_controller.go#L418

What speaks for adding it here is that the deep copy of the image-field occurs here. The container image-field plus value is later on created and/or updated by the image policy admission plugin of Openshift on the StatefulSet.

So, notebook controller needs to add this annotation to the StatefulSet, if present in the Notebook CR, as well as exclude the image-field(s) in the StatefulSet PodSpec from reconciliation for the containers referenced in the annotation.

See my discussion with a key technical lead person here on the right approach

Environment:

  • Kubeflow version: (version number can be found at the bottom left corner of the Kubeflow dashboard):
  • kfctl version: (use kfctl version):
  • Kubernetes platform: OCP 4.10.3 ff.
  • Kubernetes version: (use kubectl version): 1.23
  • OS (e.g. from /etc/os-release):

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    Status

    Needs Triage

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions