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

Pin and pre-load images #1481

Merged
merged 20 commits into from
Nov 1, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
232 changes: 232 additions & 0 deletions enhancements/machine-config/pin-and-pre-load-images.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
---
title: pin-and-pre-load-images
authors:
- "@jhernand"
reviewers:
- "@avishayt"
- "@danielerez"
- "@mrunalp"
- "@nmagnezi"
- "@oourfali"
Copy link
Contributor

Choose a reason for hiding this comment

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

the template asks that you make it clear why you're including each reviewer(i.e. what expertise/area of concern they cover), so they can focus their attention on the appropriate part of the EP.

this can also help the approver ensure we have the right set of reviewers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added @danielerez, @nmagnezi, @avishayt and @oourfali because they work on the OpenShift appliance project, which is our first use case for this feature: we would like to use it for upgrades of appliances.

I added @mrunalp because he participated in the development of the image pinning support in CRI-O, and he advised us during the discussions prior to bringing up this EP and its predecessor #1432 (now closed).

I didn't find a place in the template. Should I add it as YAML comments in the header?

Copy link
Contributor

Choose a reason for hiding this comment

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

The template gives an example of the suggested format:

reviewers: # Include a comment about what domain expertise a reviewer is expected to bring and what area of the enhancement you expect them to focus on. For example: - "@networkguru, for networking aspects, please look at IP bootstrapping aspect"
  - TBD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the example, I missed that part because I was looking at the rendered template :-( .

approvers:
- "@sdodson"
- "@zaneb"
- "@LalatenduMohanty"
Copy link
Contributor

Choose a reason for hiding this comment

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

we should aim to have a single approver for EPs, otherwise it becomes unclear who's responsible for deciding consensus is reached/discussion is complete and giving the final sign off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No objection. Who should be the approver for this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Scott, Zane, and Lala should decide or if they don't think it should/can be one of them, they can help find another name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sdodson @zaneb @LalatenduMohanty what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I hesitate to volunteer anyone, but given that this is in the machine-config directory and appears to be deliberately tightly scoped to the MCO (based on feedback that the previous PR could be split up), I'd have expected it to be either Sinny or Mrunal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrunalp @sinnykumari would you volunteer?

Copy link
Member

Choose a reason for hiding this comment

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

As this enhancement expected changes to MCO, someone like @mrunalp or @sinnykumari should be the approver.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add both me and Mrunal as it involves changes in MCO including ContainerRuntimeConfig.
/cc @haircommander for awareness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @sinnykumari, done.

api-approvers:
- "@sdodson"
- "@zaneb"
- "@deads2k"
- "@JoelSpeed"
creation-date: 2023-09-21
last-updated: 2023-09-21
tracking-link:
- https://issues.redhat.com/browse/RFE-4482
see-also:
- https://github.com/openshift/enhancements/pull/1432
- https://github.com/openshift/machine-config-operator/pull/3839
replaces: []
superseded-by: []
---

# Pin and pre-load images
Copy link
Member

Choose a reason for hiding this comment

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

BTW containers/bootc#128 is related to this, and would have the advantage it'd work in an ergonomic way outside of OCP too.


## Summary

Provide an mechanism to pin and pre-load container images.
Copy link
Member

Choose a reason for hiding this comment

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

what is the motivation for pinning? I understand pre-loading, but disqualifying the images from garbage collection in the kubelet means unused images will clutter the disk and potentially cause the disk to be overused. Is it to prevent cri-o from removing them on upgrade as it does today?

Choose a reason for hiding this comment

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

The motivation is to be able for the cluster to operate properly in the absence of an external registry. The storage should suffice in these use-cases. The idea is to pin only platform and partner images only, not all customer images running workloads on this cluster.

Copy link
Member

Choose a reason for hiding this comment

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

if the only object is to get the clusters to operate without a registry, then simply pre-loading should suffice IMO. pinning doesn't seem necessary to me

Copy link
Contributor

Choose a reason for hiding this comment

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

how would you ensure the pre-loaded images don't get removed by GC before they are needed?

Copy link
Member

Choose a reason for hiding this comment

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

if "The storage should suffice in these use-cases" then GC should never kick in as it's currently only triggered by disk usage hitting a certain threshold

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note also that users will be adding/modifying/removing their own workloads, which can bring new images, consume disk space and trigger the GC. We don't want the pinned images to be the ones evicted by that. Do we still agree that it is better to explicitly say that some images should never be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still agree that it is better to explicitly say that some images should never be removed?

I do, though we do need to think through what the user experience/recovery/alerting is if they do run out of storage because of pinned images and the system can't evict anything. I.e. how does the user get notified of the problem? And once they hit the problem is the cluster already broken or do they have time to take action before then? If they find out after the disk is full what's the recovery steps and what are the implications for the cluster+workloads until the recovery steps are performed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say that problems due to exhausted disk space aren't new: they are already possible, and I assume that your concerns are already somehow addressed. However there are some differences:

  1. If disk exhaustion happens while trying to pull the images then we will need a mechanism to report it via the API, because this won't be initiated by the kubelet and there is no pod status where to report it. The status of the PinnedImageSet should be enough for that. CRI-O will still report the issue in its log (if there is space for that), like for any other image.

  2. If disk exhaustion happens after the images have been pulled, then we need to make sure that the kubelet GC doesn't select these images. My understanding is that that is already solved by the pinning support in CRI-O: even if kubelet asks CRI-O to delete a pinned image CRI-O will not delete it. @mrunalp can you confirm/reject this?

  3. If the recovery steps (in documentation) include deleting images then we should amend them to ensure these images aren't removed.

I'l try to include these points in the document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to the risk ans mitgations section.


## Motivation

Slow and/or unreliable connections to the image registry servers interfere with
operations that require pulling images. For example, an upgrade may require
pulling more than one hundred images. Failures to pull those images cause
retries that interfere with the upgrade process and may eventually make it
fail. One way to improve that is to pull the images in advance, before they are
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have data indicating inability to pull an image (whether due to an issue in the registry or networking connectivity or whatever) is a common cause of upgrade failures?

i think the more compelling motivation for this EP is to speed up upgrades by pre-pulling images so that when the user actually clicks the "upgrade now" button, the cluster spends less time in an upgrading state (which users tend to perceive as an unstable/risky state to be in)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have numbers. I know that the upgrade failure rate due to failures to pull images is 100% when there is no registry. That is our main use case.

I am refraining from saying that this speeds up upgrades because in my experience it doesn't. A regular upgrade with a reasonable internet connection takes approx 40 minutes. The same upgrade after pre-pulling the images also takes approx those 40 minutes, at least that is my experience. I am assuming that is different when there is a severe bandwidth limitation, but I didn't test that scenario.

Choose a reason for hiding this comment

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

It's safe to say that downloading the images in advance provides a more consistent upgrade time in environments with limited connectivity. This is important when scheduling upgrades into maintenance windows, even if the upgrade might not otherwise fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @avishayt, added your comment.

Copy link
Member

Choose a reason for hiding this comment

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

@bparees I know that it's not uncommon for upgrade CI jobs to have some image pulls get throttled entering ImagePullBackOff, that's in small five node clusters, if the clusters were significantly larger the likelihood of throttling increases because we'd have more individual nodes pulling images.

actually needed, and ensure that they aren't removed.

### User Stories

#### Pre-load and pin upgrade images

As the administrator of a cluster that has a low bandwidth and/or unreliable
connection to an image registry server I want to pin and pre-load all the
images required for the upgrade in advance, so that when I decide to actually
Copy link
Member

Choose a reason for hiding this comment

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

How are we picking the image to pre-load? Pulling the full payload may be more than what's actually needed. Are we going to look at the currently running images and only pull newer references of those from the payload?

Copy link
Contributor Author

@jhernand jhernand Oct 11, 2023

Choose a reason for hiding this comment

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

Determining the set of image references for an upgrade is out of the scope for this enhancement. It will be part of a separate enhancement (based on the now obsolete #1432 ) describing the orchestration of the upgrade without a registry.

Anyhow, the initial idea was to have a single PinnedImageSet with all the payload images, for all the nodes. We added the spec.nodeSelector to be able to have different sets of images for control plane nodes and worker nodes. For example:

# For control plane nodes:
apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: PinnedImageSet
metadata:
  name: my-control-plane-pinned-images
spec:
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/control-plane: ""
  pinnedImages:
  ... 

---   
      
# For worker nodes:
apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: PinnedImageSet
metadata: 
  name: my-worker-pinned-images
spec:
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/worker: ""
  pinnedImages:
  ...

I expect that the calculation of those sets of images for the control plane nodes and the worker nodes will be done in advance by CVO or by the tool that creates the upgrade bundle. We don't intend to look at the currently running images for that.

Note that this same PinnedImageSet mechanism could be used by other tools or applications that calculate a set of images looking at the currently running ones; the mechanism doesn't forbid that.

Choose a reason for hiding this comment

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

I'd indeed keep that aside for now. the way those are calculated is in parallel to providing the relevant APIs to allow that to work.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with not addressing this now and just viewing it as roughly equivalent to mirroring the release image plus any additional images, however long term I'd like to see us move toward all upgrades pre-fetching and pinning images and before we get there I'd like to ensure that we try to limit the images as much as reasonable.

We should however try to make it clear what disk space will be consumed by this effort.

perform the upgrade there will be no need to contact that slow and/or
unreliable registry server and the upgrade will successfully complete in a
predictable time.

#### Pre-load and pin application images

As the administrator of a cluster that has a low bandwidth and/or unreliable
connection to an image registry server I want to pin and pre-load the images
required by my application in advance, so that when I decide to actually deploy
it there will be no need to contact that slow and/or unreliable registry server
and my application will successfully deploy in a predictable time.

### Goals

Provide a mechanism that cluster administrators can use to pin and pre-load
container images.

### Non-Goals

None.

## Proposal

### Workflow Description

1. The administrator of a cluster uses the `ContainerRuntimeConfig` object to
Copy link
Member

Choose a reason for hiding this comment

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

First, I think we should make it very ergonomic to automatically pin the images referenced by the release image. Not sure where that API object would live...maybe something in the CVO?

OK first actually, I think we should just default to having e.g. the MCO do a rolling "pre-upgrade" in addition to its actual upgrade. We could match it to the rollout of the MCD (the daemonset).

Today the MCO does a node-by-node drain+upgrade (according to maxUnavailable). But we can change it to pre-pull images before it starts an upgrade on a node. (Or, for the very first node, it'd be concurrent).

Then our API surface might have a tunable for "maximum number of nodes to preload/pin" like maxUnavailable.

Choose a reason for hiding this comment

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

@cgwalters i agree, but the higher level orchestration should happen on the CVO level and should be covered another enhancement - the idea was to split between those in order to deliver it as building blocks

request that a set of container images are pinned and pre-loaded:
Copy link
Contributor

Choose a reason for hiding this comment

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

The user experience of asking the administrator to provide a list of all of the images in one of our payloads is going to be a bit brittle. I understand that we need to be able to pin things that are not in the payload, too, but could we offer some shorthand for release payloads? For example, could we have a separate field for pinning a release image that uses the same pull spec that we use in the ClusterVersion API?

Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively i'd expect this to be wrapped by tooling that is part of the upgrade flow which generates/manages this resource for the user.

that said, i haven't read #1483 yet which is the "bootstrapping + upgrading" portion of this overall effort.

if the intent of this EP is just "make it possible to pull some images to nodes and pin them on those nodes" then i think this is fine as is, and we'd cover "how does this api get configured with the images to be pulled + pinned for an upgrade" as part of the upgrade process/EP.

and i'm strongly supportive of that separation of concerns, we just need to ensure the api being defined here is one that is well aligned to how the upgrade flow will need to use it. (e.g. it probably implies multiple instances of this resource so that the upgrade tooling can create/manage its own "pin these images" list w/o conflicting with any list of images the user is providing for their workloads)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #1432 (now closed) we suggested to have the logic to generate the list of images for an upgrade inside CVO. I am trying to keep that out of this EP to keep this simple and independent of upgrades. That part of #1432 will eventually be in a yet to be created EP. Note that it will not be part of #1483, as that is only about not requiring a registry during the upgrade.

I think a reasonable way to avoid conflicts could be to have something like this:

pinnedImageSets:
- name: upgrade
  images:
  - quay.io/openshift-release-dev/ocp-release@sha256:...
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
  ...
- name: my-workload
  images:
  - quay.io/my-org/my-image-1@sha256:...
  - quay.io/my-org/my-image-2@sha256:...
  ...

That would also help to include additional information regarding each set of images. For example, to have different sets of images for different types of nodes we could add a node selector:

pinnedImageSets:
- name: upgrade-control-plane
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/control-plane: ""
  images:
  - quay.io/openshift-release-dev/ocp-release@sha256:...
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
  ...
- name: upgrade-workers
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/worker: ""
  images:
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
  ...

Would that address your concerns?

Note that all this would be part of the ContainerRuntimeConfig object. This "pinned image set" concept could also be a separate CRD, but I am trying to avoid that in order to simplify things.

Copy link
Member

Choose a reason for hiding this comment

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

That part of #1432 will eventually be in a yet to be created EP. Note that it will not be part of #1483, as that is only about not requiring a registry during the upgrade.

Sounds like you have some material after all for:

### Non-Goals

None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @zaneb, added a non-goal for that.


```yaml
apiVersion: machineconfiguration.openshift.io/v1
kind: ContainerRuntimeConfig
metadata:
name: ...
spec:
containerRuntimeConfig:
pinnedImages:
- quay.io/openshift-release-dev/ocp-release@sha256:...
- quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
- quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
...
```

1. The machine config operators ensures that all the images are pinned and
Copy link
Member

Choose a reason for hiding this comment

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

this is one final thing I am not clear on: who is pulling the images?

Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should determine that before we move forward, though I could be convinced otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suggestion is to have a new PinnedImageSetController sub-controller in the machine-config-controller. I will try to clarify that in this sentence. It is also explained in the implementation details section:

The machine-config-controller will grow a new PinnedImageSetController sub
controller that will watch the PinnedImageSet custom resources. When one of
those is created, updated or deleted the controller will start a daemon set that
will do the following in each node of the cluster:

  1. Verify that there is enough disk space available in the machine to pull the
    images.

  2. Create, modify or delete the CRI-O pinning configuration file.

  3. Reload the CRI-O configuration, with the equivalent of systemctl reload crio.

    Note that currently CRI-O doesn't recalculate the set of pinned images on
    reload, support for that will need to be added.

  4. Use the CRI-O gRPC API to run the equivalent of crictl pull for each of the
    images.

This sub-controller can't pull the images directly because that requires talking to the gRPC API of CRI-O, and that is only available via localhost in each node. It is for that reason that I am suggesting to add a daemon set to do it. It could also be part of the machine-config-daemon instead of a new daemon set, but I am not familiar enough with that code to say what is better. I also tried to explain that in the implementation details section.

Ultimately the images will be pulled by the CRI-O running in each node, think of it as going to each node and running crictl pull ... for each image.

Copy link
Contributor

@sinnykumari sinnykumari Oct 31, 2023

Choose a reason for hiding this comment

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

I see this similar to how MCO applies kubeletconfig and containerruntime config on node. kubeletconfig and containerruntime controller processes respective CRD applied to the cluster which in results renders a new MachineConfig. Node sub-controller applies desired config annotation on node to signal machine-config-deamon (MCD running on each node ) to apply the change. In similar way, PinnedImageSetController would process the applied PinnedImageSetController CRD and a new rendered MachineConifg gets generated. MCD learns to apply the change by pulling those images using crictl pull and whatever needs to be done locally on the node. Alternatively, we can introduce a new daemon if needed due to any unknown limitation in MCD. Note that introducing a new daemon would still require some sort of co-ordination so that they don't go sideways like MCD started drain/reboot due a change and at the same time new daemon to pulling the images)

pulled in all the nodes of the cluster.

### API Extensions
Copy link
Contributor

@sinnykumari sinnykumari Oct 6, 2023

Choose a reason for hiding this comment

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

As per my understanding, we will also introduce a pinnedImageSet CRD in MCO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is what I mention below:

A new PinnedImageSet object will be added to the
machineconfiguration.openshift.io API group.

Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

By CRD for pinnedImageSet I meant similar to CRD we have today like machineconfig , kubeletconfig so that admin have friendly output to see by oc or equivalent utility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is what I mean when I say "object". I replaced "object" with "custom resource" to try to make it clear. So, we will have a new PinnedImageSet custom resource. It is defined in detail in openshift/api#1609.

Copy link
Member

@zaneb zaneb Oct 9, 2023

Choose a reason for hiding this comment

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

For clarity, a new type in the k8s API is a CRD (Custom Resource Definition) and an instance of that type is a CR (or object)</pedantry>


There are no new object kinds introduced by this enhancement, but new fields
will be added to existing `ContainerRuntimeConfig` objects.
Copy link
Member

Choose a reason for hiding this comment

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

I assume it's fine to modify a v1 API in OpenShift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we are considering introducing a new PinnedImageSet CR instead of modifying ContainerRuntimeConfig. See here, for example. Would you be in favour of that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it's fine to modify a v1 API in OpenShift.

as long as it's just an addition of a field and it has a sane default for upgrading clusters that won't have the value specified it's ok.

@deads2k i know we've run into cases of clients that were updating resources instead of patching them and as a result they were stomping/dropping fields they didn't know about, is that something we worry about when modifying(extending) APIs or that just falls into "bad client behavior"?


The new fields for the `ContainerRuntimeConfig` object are defined in detail in
https://github.com/openshift/machine-config-operator/pull/3839.
Copy link
Contributor

Choose a reason for hiding this comment

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

MCO api and crd has moved to openshift/api repo https://github.com/openshift/api/tree/master/machineconfiguration/v1 . Any api change will happen at new location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @sinnykumari, I moved the API pull request to that repository: openshift/api#1609 .


### Implementation Details/Notes/Constraints

Starting with version 4.14 of OpenShift CRI-O will have the capability to pin
certain images (see [this](https://github.com/cri-o/cri-o/pull/6862) pull
request for details). That capability will be used to pin all the images
required for the upgrade, so that they aren't garbage collected by kubelet and
CRI-O.
Copy link
Member

Choose a reason for hiding this comment

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

this is not quite true today. These images aren't currently disqualified for image removal on upgrade that CRI-O currently does

Copy link
Member

Choose a reason for hiding this comment

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

FWIW: we're working on dropping this behavior in CRI-O but it will take some time (4.17 time frame maybe??)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So images will be removed when CRI-O itself is upgraded regardless of being pinned? If so, is there any way around it? Would it help to disable the crio-wipe service.

Copy link
Member

Choose a reason for hiding this comment

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

yeah there's a setting to disable it. I also think it's reasonable to disqualify pinned images in the restart flow

Copy link
Member

Choose a reason for hiding this comment

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

crio-wipe.service no longer does the image wiping, it happens internally-- setting the version_file_persist config field to "" will disable it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we include that change to CRI-O (disqualify pinned images in the restart flow) in this enhancement?

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, added a paragraph explaining that.


The changes to pin the images will be done in a `/etc/crio/crio.conf.d/pin.conf`
Copy link
Member

Choose a reason for hiding this comment

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

Admins could override that file which would cause a race. Do we need something like "multiple config-dirs" in CRI-O or a reserved namespace in the MCO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this file could be named after the ContainerRuntimeConfig UUID (or the PinnedImagesSet UUID, if we agree to introduce that). Could be something like this:

pinned-images-be8545cb-dd0e-48cd-8e4f-687babc5b1b2.conf

Less likely to conflict with files created by admins, but not totally impossible.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think that's better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, added that in the latest patch.

file, something like this:

```toml
pinned_images=[
"quay.io/openshift-release-dev/ocp-release@sha256:...",
"quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...",
"quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...",
...
]
```

The images need to be pre-loaded and the CRI-O service needs to be reloaded
when this configuration changes. To support that a new field will be added to
the `ContainerRuntimeConfig` object:

```yaml
apiVersion: machineconfiguration.openshift.io/v1
kind: ContainerRuntimeConfig
metadata:
name: ...
spec:
containerRuntimeConfig:
pinnedImages:
- quay.io/openshift-release-dev/ocp-release@sha256:...
- quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
- quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
...
```

When the new `pinnedImages` field is added or changed the machine config
operator will need to pull those images (with the equivalent of `crictl pull`),
Copy link
Member

Choose a reason for hiding this comment

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

We cannot support private registries here, right? At least no pull secrets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this support private registries just fine: it uses the same mechanisms that CRI-O uses to pull any other images.

create or update the corresponding `/etc/crio/crio.conf.d/pin.conf` file and ask
CRI-O reload its configuration (with the equivalent of `systemctl reload
crio.service`).
Copy link
Contributor

Choose a reason for hiding this comment

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

is the expectation that these images will be pulled+pinned on every node in the cluster?

and that if a new node is added to the cluster, it will also then pull+pin these images when it comes up?

one thing i am wondering about is storage concerns....not all nodes run all images. do we need a nodeselector field to be included w/ a given list of images, in order to control which nodes actually pull+pin those images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that #1483 doesn't cover the overall upgrade approach. That was part of the now closed #1432 and will eventually be part of another EP. Our suggestion there is that CVO will be an user of this API: it extracts the list of images from the release image updates the ContainerRuntimeConfig object accordingly.

Yes, the intent of this EP is just to pull and pin images on nodes, regardless of what those images will be used for.

Yes, the images will be pulled and pinned in all the nodes of the cluster. Or else we can introduce the "pinned image set concept" to associate sets of images with node selectors as described in another comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a new node comes up it should honor the ContainerRuntimeConfig settings, and therefore pull and pin the images.

Copy link
Contributor

Choose a reason for hiding this comment

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

When a new node comes up it should honor the ContainerRuntimeConfig settings, and therefore pull and pin the images.

when a new node comes up you'll also have to unset the "all images pulled+pinned" status condition (whereever/however that is being reported) so that anything waiting on that (such as an upgrade coordinator) knows it now needs to go back to waiting.

i'm guessing you don't expect new nodes to be getting introduced during bootstrap+upgrade, but it feels like a potential for race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Our suggestion in the previous EP was to suspend autoscaling during the upgrade to avoid that potential race.


The machine config operator will then will use the gRPC API of CRI-O to run the
equivalent of `crictl pull` for each of the images. When that is completed the
Copy link
Member

Choose a reason for hiding this comment

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

Mean we mount crio.sock into the MCO daemons or do we already do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that requires access to crio.sock. I don't know if MCO already does that. @sinnykumari @mrunalp do you know? Should I include that in this document or should we leave it as an implementation detail?

Copy link
Contributor

@sinnykumari sinnykumari Oct 5, 2023

Choose a reason for hiding this comment

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

We don't mount crio.sock today in MCD.

machine config operator will update the new `status.pinnedImages` field of the
rendered machine config:

```yaml
status:
pinnedImages:
- quay.io/openshift-release-dev/ocp-release@sha256:...
- quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
- quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
Copy link
Contributor

Choose a reason for hiding this comment

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

In a multi-node cluster, there may be different status values for each node. How can we track those?

How are errors reported, so that if a node runs out of disk space or one of the images is inaccessible someone can discover that before trying the upgrade?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, great point about per node status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the user of this mechanism (be it the upgrade orchestrator or something else) what is important is if the images it needs have been pulled and pinned or not. That user can check that comparing the set of images it requested with the set of images contained in this status.pinnedImages field. Additional details of the failure can be reported as conditions, as events associated to the nodes, or as log messages.

In a previous incarnation of this EP I proposed to have these details in a new ClusterVersion.status field, but I was suggested to move it to the machine config because other MCO status-like configuration is there. @cgwalters I think you suggested that, but don't remember exactly.

If we wanted to make things easy for users of this API (the upgrade orchestrator, for example) what would be convenient is to have some place in the API where they can check if all the images have been pulled and pinned in all the relevant nodes, without having to explicitly compare the sets of images. Something like this inside ContainerRuntimeConfig:

status:
  pinnedImageSets:
  - name: upgrade-control-plane
    conditions:
    - type: Ready
      status: "True"

With that the user of the API would only have to check if that Ready condition is true.

Errors could then be reported with other conditions, for example:

status:
  pinnedImageSets:
  - name: upgrade-control-plane
    conditions:
    - type: Failed
      message: |
        Image 'quay.io/openshift-release-dev/...' can't be pulled because 'node0' doesn't
        have enough disk space

Should I move the EP in that direction? Note again that I am not doing that because I have been asked to avoid adding a status field to the ContainerRuntimeConfig object. If you think we should move in that direction then it will probably make sense to add a new PinnedImageSet CRD.

Copy link
Contributor

Choose a reason for hiding this comment

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

it does seem like the pinnedimageset deserves its own status. I wasn't part of the conversation that lead to the decision to use ContainerRuntimeConfig or why we can't add more status to it, but it is starting to feel like having a specific CRD to drive+report on image pulling+pinning would be appropriate as the api seems like it is going to grow in complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the document to use a new PinnedImageSet object instead of modifying the ContainerRuntimeConfig object.

...
```

### Risks and Mitigations

None.

### Drawbacks

This approach requires non trivial changes to the machine config operator.
Copy link
Contributor

Choose a reason for hiding this comment

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

at least as currently defined another drawback is the implication that every pinned image is pulled/pinned on every node, even though most of the nodes will never run pods using a given image. (e.g. master nodes will never run a user workload image. and worker nodes will never run control plane images. but there are more subtle cases too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there are subtleties, like compact three node clusters, where control plane nodes do run workloads. I think we can address this concern with the "pinned image set" concept that includes a node selector.

Copy link
Member

Choose a reason for hiding this comment

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

currently ctrcfgs are run per MCP, so a different set could be configured per control plane and worker pools, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can support different sets of images for control plane and workers adding a nodeSelector to then pinned image set, something like this:

pinnedImageSets:
- name: upgrade-control-plane
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/control-plane: ""
  images:
  - quay.io/openshift-release-dev/ocp-release@sha256:...
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
  ...
- name: upgrade-workers
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/worker: ""
  images:
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...
  - quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:...


## Design Details

### Open Questions

None.

### Test Plan

We add a CI test that verifies that images are correctly pinned and pre-loaded.

### Graduation Criteria

The feature will ideally be introduced as `Dev Preview` in OpenShift 4.X,
moved to `Tech Preview` in 4.X+1 and declared `GA` in 4.X+2.

#### Dev Preview -> Tech Preview

- Availability of the CI test.

- Obtain positive feedback from at least one customer.

#### Tech Preview -> GA

- User facing documentation created in
[https://github.com/openshift/openshift-docs](openshift-docs).

#### Removing a deprecated feature

Not applicable, no feature will be removed.

### Upgrade / Downgrade Strategy

Not applicable.

### Version Skew Strategy

Not applicable.

### Operational Aspects of API Extensions

Not applicable, there are no API extensions.

#### Failure Modes

#### Support Procedures

## Implementation History

There is an initial prototype exploring some of the implementation details
described here in this [https://github.com/jhernand/upgrade-tool](repository).

## Alternatives

The alternative to this is to manually pull the images in all the nodes of the
cluster, manually create the `/etc/crio/crio.conf.d/pin.conf` file and manually
reload the CRI-O service.

## Infrastructure Needed

Infrastructure will be needed to run the CI test described in the test plan
above.