OCPNODE-2205: Lazy image pull support

[harche]

xref : https://issues.redhat.com/browse/OCPNODE-2205

[openshift-ci-robot]

@harche: This pull request references OCPNODE-2205 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

xref : https://issues.redhat.com/browse/OCPNODE-2205

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 openshift-eng/jira-lifecycle-plugin repository.

[harche]

/cc @sairameshv

[giuseppe]

our tools do not support eStargz. We are not able to produce this format.

We can use the zstd:chunked format.

Same comment applies in other places in this patch

[harche]

Updated. Thanks.

[ktock]

Stargz store performs lazy pulling if the image is eStargz or zstd:chunked and CRI-O performs lazy pulling for them when stargz store enabled. So I think the formats don't need to be limited to zstd:chunked and eStargz can still be listed as one of the lazy-pullable formats.

[harche]

@ktock to begin with only zstd:chunked format be supported by OCP support. i.e. we will not technically prevent anyone from using any other format, but the official support will only be extending, at least in the beginning, to zstd:chunked format.

[rphillips]

I think we should have the type in addition to the pool name. Just in case we want to change the type of the lazy image pull later.

[kannon92]

We would need this for KEP-4191 also. Since we are going to modify imagestore.

[kannon92]

It would be worth exploring what types of images would not work with stargz. And how we can deduce that?

ie Can we understand why stargz is not working for this images and what kind of logs can we look at to determine that for support/operational?

[harche]

That snapshotter supports zstd:chunked and eStargz format. OCP will support zstd:chunked format only. You can enable debugging in /etc/container/storage.conf to get more verbose output of the image handling.

[kannon92]

That isn't really answering my question.

Potential Benefits for Large Images: If the container image is shared between various containers which > utilize only part of the image during their execution, Lazy image pulling speeds up such container startup by > downloading only the relavent bits from those images.

this makes it sound like the benefits of lazy pulling is not guaranteed. So i wanted to know how we could know if it wasn't working or working. From a support and a user perspective.

[kannon92]

Will it always work if someone uses zstd:chunked for their containers. And in all cases, that is all that is needed?

Are there cases where all the bits would need to be downloaded for a container to start?

[harche]

If the Stargz service plugin is not running on the host, then the images will not get lazy pulled. i.e. the container would start times will be like regular container images.

If that plugin is running and the users suspect any defect in the plugin they can look at the unit logs for details.

You can use image with zstd:chucked format, but without that plugin the lazy part of image pulling won't work, so you would have container start up times comparable to the regular container images.

[kannon92]

I'm mostly asking because the language you said makes it sound like this isn't guaranteed.

I think if stargz service plugin is enabled, and images will get lazy pulled.

I would maybe just drop the "Potentially".

But adding some logs on debugging this would be useful for docs/feature enablement.

[mtrmac]

Basically lazy pulling is trading throughput for latency — startup might be faster, but all operations are a bit slower due to the indirection required for the “is the file available already, or do I need to download it” checks.

So, the answer to “what types of images don’t benefit” from lazy pulling is “all well-designed ones” (when throughput matters): if all files in the image are actually necessary, and consumed over the lifetime of the application, then it is faster to pull and to use the image using an ordinary pull, rather than the FUSE slowly-download-files-on-demand lazy approach.

Lazy pulling should be beneficial only if many files in the container are not actually necessary for processing the typical request (and if the files that are necessary are so few that the per-file request latency is not overwhelming). I’m sure that happens frequently in practice, but it is quite likely an indication of something that could be fixed in the image.

---

(There are other benefits to zstd:chunked, like possibly sharing files across container images / layers, but those don’t depend on lazy pulling.)

[harche]

Basically lazy pulling is trading throughput for latency

stargz-snapshotter will continue to download rest of the image in the background.

[mtrmac]

The FUSE overhead is paid during every single filesystem operation during the container runtime, isn’t it? For a long-running container which is not running purely out of memory (e.g. a single statically linked binary) it will eventually overwhelm any possible savings.

[harche]

/test markdownlint

[cgwalters]

(why?)

[harche]

For the dev preview phase we thought it would be better to limit the usage to the worker nodes because we still have an important pending issue of downloaded chucks not being verified in backing c/storage library. @giuseppe, @ktock and @mtrmac are working on addressing that - containers/storage#795 (comment)

We will be blocking on that before graduating this to TechPreview

[mtrmac]

FWIW broken layer integrity enforcement is a security blocker.

I don’t know that it’s appropriate to ship to customers in any channel; at the very least it would require a well-advertised caveat that customers certainly see before actively opting in.

[mtrmac]

(Also, at the very least I am not actively working on that, I don’t know about the others listed.

As a blocker for this enhancement it probably should be tracked in more salient place than a closed upstream PR.)

[mrunalp]

This should be dropped.

[ktock]

integrity

As shown in #1600 (comment) , it's fixed.
I've just dropped this item from the list.

[cgwalters]

I am curious how "battle tested" this is. I guess we'll find out in testing, but the amount of new moving parts in code here has me left with an instinct that this will require a maintainer.

[harche]

I am curious how "battle tested" this is. I guess we'll find out in testing

Initial tests in OCP are promising. We are also proposing to add Openshift e2e job around lazy image pulling in this EP.

[mtrmac]

I agree, the plugin will certainly require a knowledgeable maintainer able to deal with customer support escalations. Even if there were no bugs in it (which I doubt), expertise would still be required to allow post-mortem analysis of broken systems.

That’s a non-trivial ongoing cost.

[cgwalters]

I know what you're trying to say here but it sounds like it's implying zstd:chunked is separate from OCI; as you know it's just an alternative compression scheme with metadata, but readers may not. How about

Replacement of the standard OCI image format; zstd:chunked is a variant of zstd compressed layers in OCI, which is supported by podman and moby

[harche]

Sounds great, thanks. Updated.

[cgwalters]

Since they can only specify one thing, why would this be mentioned in the config?

[harche]

Currently we have eStargz and zstd:chunked formats supported by the stargz-snapshotter. We are going to support only zstd:chucked format to begin with. But in future, we may support other snapshotters with their own image formats e.g. https://github.com/awslabs/soci-snapshotter

So we are just trying to make sure in future we should be able to support more snapshotters and formats (if required).

/cc @rphillips

[cgwalters]

Why? That's bizarre.

[harche]

Thanks, dropped it.

[cgwalters]

This means very little and I doubt it's necessary https://systemd.io/NETWORK_ONLINE/

[harche]

Thanks, dropped it.

[cgwalters]

For us actually, WantedBy=crio.service or kubelet-dependencies.target.

[harche]

Thanks, updated to crio.service

[cgwalters]

Right so here's where I think what we're doing in splitting binaries in CoreOS and configuration in MCO makes little sense.

I think if this is packaged as an RPM then this systemd unit should live in that RPM and not in the MCO.

And most importantly, it's ExecStart=/usr/bin/stargz-store.

(/usr/local is machine local state on CoreOS)

[harche]

Updated ExecStart=/usr/bin/stargz-store, thanks.

If we ship the systemd unit with the RPM, is there a way to enable/disable the service using MCO? We would want this service only if user explicitly wants to use lazy image pulling. Otherwise, there is no reason to have that service running by default.

[cgwalters]

See also https://www.phoronix.com/news/Linux-6.9-FUSE-Passthrough

[harche]

Thanks for that pointer, added a remark in that section about it at the end.

[cgwalters]

Yes. But...I think a notable callout here is going to need to be the docs work to describe how to do that.

In general it should be highlighted here more obviously that because zstd:chunked isn't the default, in reality customers will need to enable it in their image builds for their own images, and if they want the benefit for our images, they'll need to re-compress (which is probably not very viable).

[harche]

Good idea, I will add a documentation related goal so that we won't miss it.

[mtrmac]

and if they want the benefit for our images, they'll need to re-compress (which is probably not very viable).

In particular recompressing the OpenShift product images is, AFAIK, not viable in production workloads, because the release image payload, and the image integrity design, is built around the release image containing a full list of image digests of the OpenShift images.

[mtrmac]

Similarly any signed image. And I’d expect operators / Helm charts to fairly frequently refer to images by digest.

So… this will most likely primarily apply to customer-internal images only.

[rphillips]

format: zstd:chunked

[harche]

That will support only one, but in future if you want to support one or more then the list would be helpful.

format:
    - "zstd:chucked"
    - "ystd:hunked"
    - "xstd:junked" 

[harche]

https://github.com/openshift/enhancements/pull/1600/files#r1550258088

[ktock]

Stargz store also supports eStargz and CRI-O performs lazy pulling for both of zstd:chunked and eStargz so can we list that format here as well?

[harche]

@ktock Thanks for your reply. This enhancement proposal is aimed at introducing and supporting the new feature in Openshift. While it is true that the stargz snapshotter supports both zstd:chunked and eStargz formats, from the Openshift's supportability point of view we have to evaluate overall ecosystem around any feature and how best we can support our customers. At this point in time, Redhat's main container image building tools like podman and buildah focus on supporting zstd:chunked image format.

But having said that, we would definitely officially support more formats if overall container tools from Redhat support those formats as well. In fact, this is the reason we have added format field in the API with this enhancement proposal.

[ktock]

Thanks for the clarification!

This enhancement proposal is aimed at introducing and supporting the new feature in Openshift.

👍

While it is true that the stargz snapshotter supports both zstd:chunked and eStargz formats, from the Openshift's supportability point of view we have to evaluate overall ecosystem around any feature and how best we can support our customers. At this point in time, Redhat's main container image building tools like podman and buildah focus on supporting zstd:chunked image format.
But having said that, we would definitely officially support more formats if overall container tools from Redhat support those formats as well. In fact, this is the reason we have added format field in the API with this enhancement proposal.

So the runtime will have a logic to decide whether it enables lazy pulling on the pulling image, right? I have a question in the proposal, related to the implementation of that logic: PTAL #1600 (comment)

[harche]

We have a operator in Openshift called Machine Config Operator (MCO). We use that operator to lay down node specific configurations (e.g. config files in /etc). Our plan is to use that operator to lay down required configuration for the c/storage library used by crio to enable this feature. In fact, that config file for c/storage is already placed by MCO already, we just need to update it with the lazy image related configuration (if user decides to use this feature).

[ktock]

How is this value consumed by the runtime? i.e. How can the runtime use this value for deciding whether it enable lazy pulling on the pulling image? Maybe this should be a mediatype + manifest annotation?

[mtrmac]

Right now, none of this exists at a generic level: the on-demand layer plugin is provided with an image reference and a layer digest, and that’s it. So if this had to be implemented today, that plugin would need to re-parse the image manifest, find the layer, and impose its own conditions on the layer’s metadata.

If we need to add a generic feature, that will need to be co-designed along with this enhancement.

[harche]

How is this value consumed by the runtime? i.e. How can the runtime use this value for deciding whether it enable lazy pulling on the pulling image? Maybe this should be a mediatype + manifest annotation?

@ktock As of now, this values isn't directly used by the runtime. We are using this value to in MCO to prepare the configuration file for the c/storage library.

[mtrmac]

c/storage AFAIK has no option to restrict ALS lookups by compression format right now.

[ktock]

I think we can introduce a field additionallayerstore_target_mediatype in storage.conf. This is layer mediatypes to enable to lookup of Additional Layer Store.

containerRuntimeConfig.lazyImagePull.format should also be a list of mediatype. Or, MCO can define aliases for them; e.g. "zstd:chunked" means application/vnd.oci.image.layer.v1.tar+zstd.

[mtrmac]

My first instinct, from the runtime and c/storage perspective is to ask “why do we need that option at all?”.

I could, maybe, imagine that users might want to enable/disable lazy pulls based on the application (= on the registry namespace, on the K8S namespace, or as a Pod option). (To be clear, I don’t think we need that for a first version at all.)

But why would users want to make a choice based on the format? Isn’t that ~transparent to the running systems, other than performance?

---

It might be necessary to have an option that governs “what format backends (snapshotters?) should be installed on nodes” — maybe not for the first version, but it does make sense to ensure that we can add that option later.

But that option should govern the behavior of MCO, c/storage doesn’t need to know. storage.conf only contains an additionallayerstore option with a list of mount points; how those mount points get set up and what are the backing FUSE servers is not a storage.conf concern AFAICS.

[harche]

I agree, we can drop this option.

[ktock]

Thanks for the review. Removed this option from the proposal.

[mtrmac]

I don’t see any mechanism for containers to be downloaded again.

(Cc: @giuseppe to keep me honest about the c/storage aspects.)

Once an Image object exists in c/storage, it doesn’t go automatically away when a backend layer disappears; the image is just broken. Once a layer record exists in c/storage metadata, it doesn’t go automatically away when the layer store changes and the layer no longer exists; the layer is just broken (and might even “infect” future pulls that share this layer, because the metadata lookup will indicate that the layer already exists locally).

Unless the downgrade triggers a complete wipe? I don’t know whether it does, and the text above is not worded in a way consistent with that.

(Also, zstd in a non-chunked variant has been supported for a long time; and layers are locally stored uncompressed, so the compression format doesn’t matter all that much. But that’s all mostly irrelevant.)

Even in the absence of stargz-snapshotter, those container will start but without any performance gains.

Absolutely not; the snapshotted layers are stored in different paths from the point of view of the graph driver (and I very much hope that the backend plugins are not trying to store anything in the graph driver-managed locations).

See above about “once a layer record exists”…

This downgrade either needs to explicitly trigger a wipe (assuming that’s possible at all), or must be prevented.

---

And it’s not just downgrades: disabling the snapshotters on an already-running node would break the node for the same reasons.

[harche]

Yes, you are correct, if the images need to be cleaned up if the stargz store service is not enabled (or fails to start). I will update the proposal to explicitly call out that scenario. Thanks.

[mtrmac]

See the “downgrade” section; just disabling the option will break the node, AFAICS.

[mtrmac]

• Just to make sure OCPNODE-2205: Lazy image pull support #1600 (comment) is considered
• I don’t see any discussion of registry credentials. The lazy-pull layer store does not currently get any credentials from the actual pull operation in c/image, they must be provided to the plugin via other means
  • I see no discussion of that happening here at all
  • Assuming the capabilities as of today, that would either mean that per-image pull secrets linked from the Pod object would not be used at all, at best this would work for the cluster global pull secret; that’s possible but a fairly significant downside for any production workloads; or maybe this particular snapshotter seems to support somehow capturing credentials by interposing a CRI proxy (ingenious but eww, and I don’t know how that could work in multi-tenant situations).
  • Alternatively, some new feature to provide the credentials directly from the pull operation would have to be built. And because providing credentials via file paths passed to FUSE is out of the question, that would be a major rearchitecture of the API, AFAICS

[openshift-bot]

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

[ktock]

/remove-lifecycle stale

[mtrmac]

This LGTM as written, but I’m not an approver in this repo; and I can’t speak to support / production deployment implications.

[haircommander]

FYI openshift api doesn't anymore allow booleans because they're not extendable, you'll need to add an enum

[ktock]

Thanks for the review. Fixed to avoid using booleans.

[haircommander]

/lgtm

[ktock]

Thanks for your approval. Can we move this forward and merge this?

[harche]

Thanks for your approval. Can we move this forward and merge this?

cc @mrunalp for approval required to merge it.

[ktock]

@mrunalp Could you please take a look at this PR? 🙏

[harche]

@mrunalp Could you please take a look at this PR? 🙏

@ktock I think @mrunalp should be able to take a look at this when he is back next week.

[ktock]

/cc @mrunalp

[mrunalp]

Yeah, reviewing..

[mrunalp]

It should be possible to enable this on HyperShift node pools via MCO config above.

[ktock]

Added description to this section.

[mrunalp]

Microshift should need documentation on how to enable it as it doesn't have MCO.

[ktock]

To enable this without MCO, manual setup will be needed. Added description to this section.

[openshift-ci]

New changes are detected. LGTM label has been removed.

[ktock]

@mrunalp Thanks for your review! Fixed the document. PTAL 🙏

[ktock]

/cc @mrunalp

[openshift-ci]

@harche: 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-sigs/prow repository. I understand the commands that are listed here.

[cgwalters]

Also note containers/storage#2156

[ktock]

Hi @mrunalp, it was nice to meet you at the last KubeCon! Please take a look at this PR when you have time 🙏

[openshift-bot]

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

[ktock]

/remove-lifecycle stale