Pin and pre-load images

[jhernand]

This patch adds an enhancement that describes a mechanism to pin and pre-load container images.

Related: https://issues.redhat.com/browse/RFE-4482
Related: https://issues.redhat.com/browse/OTA-1001
Related: https://issues.redhat.com/browse/OTA-997
Related: openshift/machine-config-operator#3839
Related: #1432

[dhellmann]

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?

[bparees]

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)

[jhernand]

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.

[zaneb]

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.

[jhernand]

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

[dhellmann]

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?

[bparees]

+1, great point about per node status

[jhernand]

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.

[bparees]

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.

[jhernand]

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

[bparees]

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.

[jhernand]

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

[bparees]

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

[jhernand]

@sdodson @zaneb @LalatenduMohanty what do you think?

[zaneb]

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.

[jhernand]

@mrunalp @sinnykumari would you volunteer?

[LalatenduMohanty]

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

[sinnykumari]

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

[jhernand]

Thanks @sinnykumari, done.

[bparees]

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

[jhernand]

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?

[bparees]

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

[jhernand]

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

[bparees]

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)

[jhernand]

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.

[avishayt]

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.

[jhernand]

Thanks @avishayt, added your comment.

[sdodson]

@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.

[bparees]

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)

[bparees]

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?

[jhernand]

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.

[jhernand]

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

[bparees]

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.

[jhernand]

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

[bparees]

+1, great point about per node status

[bparees]

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).

[jhernand]

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.

[haircommander]

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

[jhernand]

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:...

[cgwalters]

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

[cgwalters]

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.

[romfreiman]

@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

[openshift-ci]

@sinnykumari: GitHub didn't allow me to request PR reviews from the following users: for, awareness.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

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

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/test-infra repository.

[haircommander]

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

[haircommander]

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

[jhernand]

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.

[haircommander]

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

[haircommander]

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

[jhernand]

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

[haircommander]

yeah I think so

[jhernand]

OK, added a paragraph explaining that.

[haircommander]

cc @saschagrunert @sohankunkerkar @QiWang19 for CRI-O/ctrcfg reviews

[haircommander]

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?

[oourfali]

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.

[haircommander]

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

[bparees]

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

[haircommander]

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

[haircommander]

kubernetes/enhancements#4211 😃

[jhernand]

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?

[bparees]

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?

[jhernand]

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.

[jhernand]

Added this to the risk ans mitgations section.

[saschagrunert]

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

[jhernand]

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?

[bparees]

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"?

[saschagrunert]

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?

[jhernand]

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.

[saschagrunert]

Yeah I think that's better

[jhernand]

OK, added that in the latest patch.

[saschagrunert]

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

[jhernand]

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?

[sinnykumari]

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

[saschagrunert]

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

[jhernand]

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

[sinnykumari]

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.

[jhernand]

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

[sinnykumari]

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

[jhernand]

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?

[sinnykumari]

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.

[jhernand]

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.

[zaneb]

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>

[sinnykumari]

I maybe wrong, but all these extra constraint makes me think that we may need a new sub-controller in MCC to render the pinnedImageSet and apply the extra logic to generate a file with filename having amended uuid.

[jhernand]

I am not familiar enough with MCC to be certain, but what you say sounds reasonable to me. Added a sentence explaining that:

This logic to handle the pinned image sets and generate the configuration files
will be part of a new sub-controller in MCC.

Does that look good?

[hexfusion]

The authoritative config is based on a sorting of the files done by crio. So for example if [crio.image] contained pinned_images in the following files.

00-default.conf
my-pinned-images-550a1d88-2976-4447-9fc7-b65e457a7f42.conf
01-my files.conf

the later 01-my files.conf would take preference.

[sinnykumari]

Where are we talking about getting the behavior changed? In crio or additional logic in MCO?

[jhernand]

I believe this will need to be changed in CRI-O. Tried to clarify it.

[haircommander]

Those could be tags or digests, although in our use cases they will always be digests

if that's the case, then the API should validate that IMO

[sdodson]

@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.

[sdodson]

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.

[sdodson]

When will that be added?

What specifically do we mean by "reset" here? If I land a new PinnedImageSet config and reload will cri-o pick that up on reload but if I were to modify or delete it then those changes wouldn't be picked up on reload?

As long as new configs are applied on reload I think this is likely manageable, infact some customers may wish to preserve their ability to preform a z-stream rollback not wishing to delete the previous PIS until subsequent update. That would however mean that they need storage capacity for up to 3 PIS, old, current, and to-be applied versions. When applying the update they could remove the old version and allow those images to be garbage collected after reboot.

[sdodson]

Do we then do anything with the PIS crio config? Do we just keep trying to pull pinned images hoping that garbage collection frees sufficient space to complete the operation?

If we delete it then that doesn't really free the space until cri-o gets restarted per earlier notes about resetting. This worries me that we could wedge nodes, should we ensure that before we pull images we compute the space which the garbage collection threshold would trigger at and abort if the content we'd pull exceeds the minimum free space available and not pull if GC is in progress?

32GiB to pull, 320GiB disk with 10% GC threshold wouldn't allow us to pull, but any threshold higher than that would because we can be sure if GC is not running we've got enough space to complete our operation.

[jhernand]

We intend to add the upgrade orchestration in a separate EP, based on #1432. I would like to do after this and #1483 are approved.

My understanding is that "reset" in this context means that when CRI-O receives the reload signal it needs to forget the previous set of pinned images and rebuild it from the configuration files. According to @haircommander it doesn't do that today.

Note that our suggestion is that when some PinnedImageSet changes first CRI-O is reconfigured and reloaded, and only then images start to be pulled. Imagine, for example, that you have images A and B pinned and then you remove B and add C. What would happen first is that CRI-O is reconfigured so that only A and C are pinned. That is unlikely to fail, as it only involves generating configuration files and sending signals. Only after that would we start to pull the images. That is more likely to fail, for example because C cannot be pulled. But even if that fails Kubelet can still garbage collect B.

I'd say that if disk is exhausted while pulling the images the behavior should be similar to what happens when a regular pod fails to pull an image. We retry that, so we should retry this as well.

Computing the disk space needed in advance would be helpful indeed. But implementing it will be challenging. It means contacting the registry to figure out the size of the layers, but also figuring out how much space those layers will take when extracted, and that requires intimate knowledge of the underlying workings of the container library. Would it be acceptable to use an heuristic? For example, can we contact the registry to find the sizes of the layers and then assume that the space consumed will be twice that? I say twice because that is roughly the case of the current OCP payloads. @sdodson let me know of you think that is acceptable.

[sdodson]

Yeah, 2x the size of the layers seems like a good start to me.

[jhernand]

OK, added the disk space check details to the implementation details and risk mitigation sections.

[sdodson]

@jhernand @sinnykumari Any thoughts about what specifically is doing the pulling here? Is that MCD on the node growing a controller to iterate over all PIS .conf files? the MCO itself or MCC calling CRI-O remotely over gRPC? I assume this interface isn't already network accessible?

[jhernand]

I am not very familiar with the machine config code, but I'd say that this should be a new machine-config-controller sub-controller that watches the PinnedImageSet custom resources. When it comes to actually pulling the images it should either create a new daemon set or else let the machine-config-daemon do it, because that task requires access to the gRPC API of CRI-O which is only available via localhost. In the prototype we used a daemon set.

[sdodson]

Do you mind updating it to be clear that either the MCD or another new DaemonSet will be responsible for monitoring the PIS configs and retrieving images?

[jhernand]

I updated the implementation details section to try to clarify that there will be a new machine-config-controller sub controller and a new daemon set. Please check again: 25fc032

[sinnykumari]

I am not sure if we want to create a new deamonset to fetch images. MCO team didn't have any implementation detail discussion for it. But per my understanding, how I see this is MCC gains a new sub-controller to process PIS Custom Resource Object supplied to the cluster and renders it into rendered MacineConifg. MCD is where we do usually node level processing. So, if MCD can handle writing PIS related config on node as well as pulling listed images then we should use MCD. Otherwise, we can look for adding a new daemonset.

[jhernand]

Sounds good to me. I think this is an implementation detail that we don't need to close in this document.

[dhellmann]

If there is only 1 status value here, does the MCO have a way to track progress across nodes? Can the user see that progress somehow?

What happens if MCO/MCC is restarted in the middle of pinning a set of images, will it have the information it needs to recover?

[jhernand]

In the currently proposed API there is no mechanism to track progress across nodes. We did have that in the prototype: a set of per-node list of images successfully pulled. But I decided to left that out here because it is not really necessary for the user of the API: all what the user of the API needs to know is if the images have been successfully pulled (in all the nodes) or not. I acknowledge that per-node progress information may be needed to actually implement this, but I'd consider it an implementation detail that doesn't need to be here. Let me know if you disagree and I will add those details.

When MCO/MCC come back after a restart or crash they will know if the images have been pulled and pinned already (that is part of the PinnedImageSet status). If they haven't then the process can start again: a typical reconciliation. Some image may already have been pulled. CRI-O will be asked to pull it again, it will contact the registry, verify that it is already pulled, and do nothing (layers are already downloaded, and won't be downloaded again).

[sdodson]

It occurred to me that node objects contain the list of images on the node because I believe the scheduler prefers to schedule pods toward nodes where the image is already present. So this seems like it could be computed centrally but the cost of doing so may be high to watch or list all nodes and confirm that all currently pinned images are present on all of them.

I'm fine with nodes working autonomously toward pulling all of the images but I think there needs to be a central point to answer the question "Right now, is a given PinnedImageSet pulled on all nodes." And ideally in the future PinnedImageSet could be extended to indicate which release image it contains and then the CVO can inspect all PinnedImageSets to know if there's a pending operation for the release payload that's being proposed for an upgrade.

[jhernand]

In the currently proposed API (see openshift/api#1609) there is a Ready condition to indicate that all the images of a PinnedImageSet have been pinned and pulled in all the nodes of the cluster. For example:

apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: PinnedImageSet
metadata:
  name: my-worker-pinned-images
spec:
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/worker: ""
  pinnedImages: [ ... ]
status:
  conditions:
  - type: Ready
    status: "True"   # <-- This indicates that all the images have been pulled in all the nodes.
  - type: Failed
    status: "False"

That is what users of the API need. For example, CVO would need just to check this, it won't be interested in knowing what is the progress for specific nodes.

The magic that is missing and I didn't describe in the document is how that is calculated. There are multiple mechanisms that can be used for that. For example, we could add an annotation (or label) to nodes to indicate that all the images are pulled in that particular node, and then the machine-config-controller sub-controller could watch nodes and calculate this. Or we could put that in the status of the PinnedImageSet:

...
status:
  conditions:
  - type: Ready
    status: "False"
  - type: Failed
    status: "False"
  nodeConditions:  # <-- This is a dictionary, where keys are node names.
    node0:
    - type: Ready
      status: "True"   # <-- This indicates 'node0' pulled all the images.
    - type: Failed
      status: "False"
    node1:
    - type: Ready
      status: "False"   # <-- This indicates 'node1' pulled all the images.
    - type: Failed
      status: "False"
    ...

I'd say that this latter is better, because all the information is in one single place: less objects to watch.

There are other alternatives, like having a list of node names that have all the images pulled (smaller, so better for large clusters), having it only in memory of the machine-config-controller sub-controller (assuming it uses the leadership mechanism), etc. I am refraining from writing any of that in this document because I believe it is an implementation detail that is of no interest to the user of the API.

Regarding how CVO will potentially use this, I think it should create a PinnedImageSet with a name (or label, or annotatoin) that contains the version number. For example, to upgrade to 4.13.150 it would create something like this:

apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: PinnedImageSet
metadata:
  name: upgrade-worker-4.13.150
spec:
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/worker: ""
  pinnedImages: [ ... ]

CVO will then be watching these objects, and process only the ones that it created for that particular upgrade. When all of them have the Ready condition set to True (the one that says "for all nodes") it will then proceed to the next steps of the upgrade.

[sinnykumari]

We maybe able to utilize and extend MCO state reporting API to indicate status of PinnedImageSet on a node openshift/api#1596.
Also, I beleive in the end MCO will manage, apply and process PinnedImageSet nodes at pool level? So, MCP status should throw error if it failed to apply PinnedImageSet on a node in the pool.

[jhernand]

I'd say that the machine-config-daemon (or a new daemon set, whatever we decide to implement this) will need to enumerate the PinnedImageSet custom resources, and for each one check if the spec.nodeSelector matches the node where it is running. If it matches then it will need to pull the images. I guess I am missing something because I don't see the difficulty. @sinnykumari @haircommander @yuqi-zhang can you elaborate on what is your concern so that I can try to address it?

[yuqi-zhang]

Sorry, just got to this thread today. So as I understand the PinnedImageSet object, the final goal is just to make sure each node is able to call cri-o with a list of images it needs to pull, correct?

In a sense if we wish to process this logic separately, we don't need to tie them to machineconfig pools. In fact we probably don't even need a new controller. The "smaller" implementation is just a daemon with RBAC access to PinnedImageSet object + its own node object, and is able to watch for changes. Once any PIS changes, the daemon can cross check with its node object to see if images need to be pulled, and just run a crio call. So I guess there isn't really a need to leverage MCO directives.

This is just one way to interpret this. I could be missing some critical big picture here though

[jhernand]

The goal is to have the images available in the nodes, calling CRI-O to do it is just how we suggest to implement it.

Note that CRI-O doesn't have a call to pull multiple images at once, instead each image requires a separate call. See the PullImage RPC.

Your implementation suggestion looks reasonable to me @yuqi-zhang. I tried to not be too specific about the fine details because I assume we will decide about that during the implementation. The document already mentions that this can be part of the machine-config-daemon or of an independent daemon set.

[yuqi-zhang]

Ack, thanks, then the general idea seems fine to me. I think we can follow up on implementation details on the daemonset later.

@sinnykumari was I able to cover your concern?

[sinnykumari]

thanks Jerry, sounds good. I have been thinking it as a sub-controller and use MCD to make sure that these node operations are happening in co-ordinated way in MCO which I captured in #1481 (comment) . But yes, implementing them would make things more clearer on the best path.

[jhernand]

Those could be tags or digests, although in our use cases they will always be digests

if that's the case, then the API should validate that IMO

@haircommander added a validation to the API pull request:

openshift/api@c58fe27

Does that look good?

[haircommander]

Those could be tags or digests, although in our use cases they will always be digests

if that's the case, then the API should validate that IMO

@haircommander added a validation to the API pull request:

openshift/api@c58fe27

Does that look good?

this looks like it'll deny an image whose registry is specified. is that intentional?

[jhernand]

this looks like it'll deny an image whose registry is specified. is that intentional?

@haircommander I used a regular expression with no begin anchor and an end anchor: @sha256:[a-fA-F0-9]{64}$. Note that it doesn't start with ^ and ends with $. I assume that means "anything that ends in @sha256:[a-fA-F-0-9]{64}. I didn't want to write the full regular expression for an image reference.

[haircommander]

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

[haircommander]

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

[jhernand]

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.

[sinnykumari]

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)

[oourfali]

@sinnykumari @haircommander do you believe we can merge this enhancement?

[haircommander]

LGTM

[sinnykumari]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sinnykumari

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sinnykumari]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[haircommander]

/lgtm

[openshift-ci]

@jhernand: all tests passed!

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/test-infra repository. I understand the commands that are listed here.