Dynamically set Imagestream importMode based on cluster payload type #1605
Conversation
Prashanth684
force-pushed
the
dynamic-importmode
branch
from
4589ebd to
3ceeba0
Compare
Prashanth684
requested review from
deads2k,
dmage,
soltysh and
wking
and removed request for
dgoodwin and
mandre
enhancements/multi-arch/dynamic-imagestream-importmode-setting.md
Outdated
Show resolved
Hide resolved
| `release.openshift.io/architecture`) | ||
| 3. openshift-apiserver operator looks to see if | ||
| the image config CRD has the | ||
| `ImagestreamImportMode` set. If it does, get that |
There was a problem hiding this comment.
could we use the ClusterVersion only if we give up
Allow users to control the import mode setting
manually through theimage.config.openshift.io
image config CRD.
I'd be ok with that tradeoff to reduce surface area and footguns.
There was a problem hiding this comment.
Closing the loop. This is potentially useful in the case where customers are afraid of size.
Let's keep the API field, but not write any documentation for it and add a clusterfleetevaluation measuring using. If it's not used after four releases, let's remove.
There was a problem hiding this comment.
ok. I will add a note regarding this.
There was a problem hiding this comment.
included a note in the "Operational Aspects of API Extensions section"
| 3. openshift-apiserver operator looks to see if | ||
| the image config CRD has the | ||
| `ImagestreamImportMode` set. If it does, get that | ||
| value, if not look at the `ClusterVersion` status |
There was a problem hiding this comment.
No fallback. only use the images.config.openshift.io status field.
There was a problem hiding this comment.
images.config.openshift.io would not have a status field related to this. Only the spec would have a field enabling the importmode to be set through it. The default path is for openshift-apiserver-operator to look at the ClusterVersion status and set the importmode appropriately.
There was a problem hiding this comment.
if the imagestreamImportMode field is set in the images.config.openshift.io spec (as described below), that is considered first, if not fallback based on the ClusterVersion's new status field which exposes the type of the payload.
There was a problem hiding this comment.
made the appropriate changes to include this in the flow.
| `ImagestreamImportMode` config flag to CRD spec. | ||
|
|
||
| ``` | ||
| type ImageSpec struct { |
There was a problem hiding this comment.
need a status field as well. The imageregistry operator will set the status based on .spec+CVO status. The openshift-apiserver-operator will only consume the status and will require the status prior to starting the first time.
There was a problem hiding this comment.
Is there an issue if the openshift-apiserver-operator directly consumes the ClusterVersion status?
There was a problem hiding this comment.
I guess this makes sense since the status field of images.config.openshift.io is the only field the operator would need to look at to make the decision and that field would be updated by the imageregistry operator based on the ClusterVersion status/ the imagestreamImportMode field in the spec
There was a problem hiding this comment.
made the appropriate changes to based on adding a field in the status.
Prashanth684
force-pushed
the
dynamic-importmode
branch
3 times, most recently
from
c335185 to
fc6d4ef
Compare
| mode would always be set to `Legacy` because it | ||
| is sufficient if the single image manifest is | ||
| imported and for a cluster installed with the | ||
| multi payload it would be `PreserveOriginal` |
There was a problem hiding this comment.
Pinning down current behavior, multi-arch release controller -> recent 4.16 nightly -> blocker run -> gathered artifacts -> ... -> must-gather artifacts:
$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-multiarch-master-nightly-4.16-ocp-e2e-aws-ovn-heterogeneous/1782426110521249792/artifacts/ocp-e2e-aws-ovn-heterogeneous/gather-must-gather/artifacts/must-gather.tar | tar -tvz | grep 'namespaces/openshift/.*imagestreams/installer.yaml'
-rw------- 1017300000/root 2306 2024-04-22 09:47 quay-io-openshift-release-dev-ocp-v4-0-art-dev-sha256-1e4b8e008b76da76e5b9f893667d39d627f442cc2dfa4139e0084dc679786828/namespaces/openshift/image.openshift.io/imagestreams/installer.yaml
$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-multiarch-master-nightly-4.16-ocp-e2e-aws-ovn-heterogeneous/1782426110521249792/artifacts/ocp-e2e-aws-ovn-heterogeneous/gather-must-gather/artifacts/must-gather.tar | tar -xOz quay-io-openshift-release-dev-ocp-v4-0-art-dev-sha256-1e4b8e008b76da76e5b9f893667d39d627f442cc2dfa4139e0084dc679786828/namespaces/openshift/image.openshift.io/imagestreams/installer.yaml | yaml2json | jq -c '.spec.tags[].importPolicy'
{"importMode":"PreserveOriginal","scheduled":true}And amd64 release controller -> 4.16.0-ec.5 -> blocker run -> gathered artifacts -> ... -> must-gather artifacts:
$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-ovn-serial/1777335045871112192/artifacts/e2e-aws-ovn-serial/gather-must-gather/artifacts/must-gather.tar | tar -tvz | grep 'namespaces/openshift/.*imagestreams/installer.yaml'
-rw------- 1010890000/root 2306 2024-04-08 09:46 quay-io-openshift-release-dev-ocp-v4-0-art-dev-sha256-6496aec41107e9730791d82ebc65bb873aa228c0fd873444932b477679feb1ff/namespaces/openshift/image.openshift.io/imagestreams/installer.yaml
$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-ovn-serial/1777335045871112192/artifacts/e2e-aws-ovn-serial/gather-must-gather/artifacts/must-gather.tar | tar -xOz quay-io-openshift-release-dev-ocp-v4-0-art-dev-sha256-6496aec41107e9730791d82ebc65bb873aa228c0fd873444932b477679feb1ff/namespaces/openshift/image.openshift.io/imagestreams/installer.yaml | yaml2json | jq -c '.spec.tags[].importPolicy'
{"importMode":"PreserveOriginal","scheduled":true}What's the downside to that approach? I could see mirror-size concerns, but you list sparse-manifests as a non-goal, and while I could see not needing a particular image in all architectures, I'm having trouble seeing cluster-scoped decisions in that space. And... by the time you're launching clusters off of a multi-arch mirror, you've already completed the mirroring, right? Without sparse-manifests, how do you even install a multi-arch payload based on partial mirroring?
There was a problem hiding this comment.
Looks like those OpenShift ImageStreams moved to explicitly PreserveOriginal in openshift/cluster-samples-operator#482, which seems to have gone out in 4.13.
There was a problem hiding this comment.
This proposal is meant to set importMode default based on the type of payload. Today, the default is Legacy which is problematic when running on a cluster with multiarch compute nodes as the imported image is not manifestlisted. This is not a problem for CVO managed imagestreams as we default them to preserveOriginal, but for user managed imagestreams, the default is set through apiserver. Also, sparse manifest in this context refers to sparse mirroring where a user wants to mirror only certain arches, but keep the payload digest intact. this is not supported by any registry today, but it would help users who are concerned with mirroring all 4 arches in any of their images.
There was a problem hiding this comment.
Yup, RFE-3333 and MULTIARCH-424 seem like reasonable public references for sparse-manifest mirroring, and while that seems extremely useful, I understand that it's a larger lift than you're aiming for in this enhancement.
If the target of this enhancement is to create an even-more-clever default that works well for more ImageStream creators without requiring an explicit importMode, what's keeping us from adding additional steps to the Legacy logic like "don't flatten to a single arch if the cluster has nodes from multiple arches"?
There was a problem hiding this comment.
i don't think we need to build this level of intelligence into apiserver/image registry operator. eventually, when sparse manifests are supported, importing manifestlists as default will become a non issue, so we want to keep it simple.
| - Dynamically alter import mode type on upgrade | ||
| from a single arch to a multi paylaod and vice | ||
| versa. |
There was a problem hiding this comment.
Is this implying that a cluster could move from multi to single arch? I thought that was explicitly forbidden given the complexity of ensuring that we had evacuated all critical components from nodes of the to-be-removed arch?
There was a problem hiding this comment.
it is not officially supported - but that doesn't prevent anyone from explicitly upgrading/downgrading. For this particular scenario though it wouldn't matter either way as the apiserver pivots importmode based on the release payload arch.
| - As an Openshift user, I do not want to change my | ||
| existing scripts and oc commands to add extra | ||
| flags to enable manifestlist imports on | ||
| imagestreams. |
There was a problem hiding this comment.
One thing to note here is that while users may (eventually) not need to change their oc commands and scripts to enable automagic manifest list imports they will likely have to update any scripts that deal with Image objects (our own CI went through this). So moving to manifest lists will not be entirely transparent at, least not in every case, because Image objects for manifest lists do not contain layers, but sub manifests (sorry I don't remember the fields exactly, but can dig them up if needed.).
There was a problem hiding this comment.
yes, this was brought up and I believe this should not be as big of a concern because not a whole lot of users outside of CI and internal tooling consume the image APIs directly
enhancements/multi-arch/dynamic-imagestream-importmode-setting.md
Outdated
Show resolved
Hide resolved
enhancements/multi-arch/dynamic-imagestream-importmode-setting.md
Outdated
Show resolved
Hide resolved
enhancements/multi-arch/dynamic-imagestream-importmode-setting.md
Outdated
Show resolved
Hide resolved
|
|
||
| N/A | ||
|
|
||
| ### Drawbacks |
There was a problem hiding this comment.
One concern I have with enabling PreserveOriginal import mode for all future image stream imports cluster-wide is the potential growth of storage space consumption in etcd.
For each sub manifest present in a manifest list, there will be an equivalent Image object with a list of its layers + other basic info.
While Image objects aren't extremely big, a single manifest list usually has a few sub manifests in it, so instead of having just 1 Image object (like what the Legacy mode does), we will potentially have the manifest list + the number of architectures it supports. So if a manifest list supports 4 architectures, there would be a total of 5 Image objects created in etcd... for just one manifest listed tag imported.
There was a problem hiding this comment.
Does this depend on whether the reference policy is local or from source? in other words even if it is from source, do we incur the cost of these entries? I'll make a note of this.
There was a problem hiding this comment.
Yes, the reference policy is about whether the image registry itself stores copies of the images or not. Image objects are always stored [in etcd] regardless of which reference policy is used.
There was a problem hiding this comment.
added this to the risks section
Prashanth684
force-pushed
the
dynamic-importmode
branch
from
f8d366d to
e3004f5
Compare
| ... | ||
| history: | ||
| ... | ||
| payloadArchitecture: amd64 # could be arm64/ppc64le/s390x/multi |
There was a problem hiding this comment.
The current spec.desiredUpdate property is architecture, and the valid values are Multi or empty. We'd initially floated multi to match the Cincinnati query parameter, but were told to use CamelCase for consistency with other Kube enums.
It would be theoretically possible to also pass along the single-arch payload architecture, but I don't see why that would be necessary (components in a single-arch cluster that need to know can use GOARCH or similar), and the release.openshift.io/architecture metadata is multi or unset. So I expect we want:
desired:
architecture: Multi # optional, unset/empty-string if it's single arch
here just for consistency. And that we want it under desired to mirror the spec.desiredUpdate nesting.
There was a problem hiding this comment.
makes sense. I will update the doc accordingly
|
|
||
| #### Hypershift / Hosted Control Planes | ||
|
|
||
| N/A |
There was a problem hiding this comment.
On HyperShift, having the hosted-API-side ClusterVersion driving control-plane side functionality (like OpenShift API server behavior) exposes you to some hosted-admin interference (see this comment, explaining why we jump through hoops to avoid touching hosted-API stuff when setting the upstream update service for HyperShift clusters). Default importMode doesn't seem particularly sensitive, and the registry lives on the hosted compute nodes anyway, so maybe routing through ClusterVersion is fine in this case. But probably worth explaining that issue, and why we're ok going through ClusterVersion here.
enhancements/multi-arch/dynamic-imagestream-importmode-setting.md
Outdated
Show resolved
Hide resolved
enhancements/multi-arch/dynamic-imagestream-importmode-setting.md
Outdated
Show resolved
Hide resolved
| - The `imagestreamImportMode` API setting in the | ||
| image config CRD's spec will not be documented. A | ||
| cluster-fleet-evaluation would be done which accesses | ||
| telemtry data to determines the usage. If the field |
There was a problem hiding this comment.
And you possibly mean Insights? Because it doesn't seem like it's worth a new Telemetry metric for this. Possibly you want to reference the cluster fleet evaluation enhancement, although while that enhancement calls for EvaluationConditionDetected, it actually ended up landing as the plural EvaluationConditionsDetected in openshift/api#1250.
There was a problem hiding this comment.
i meant telemetry from the perspective of querying for the imageStreamImportMode API field in the image config object..but is that sort of thing typically done through insights?
Prashanth684
force-pushed
the
dynamic-importmode
branch
from
e3004f5 to
7ef4774
Compare
Prashanth684
force-pushed
the
dynamic-importmode
branch
from
7ef4774 to
377f368
Compare
|
|
||
| ## Support Procedures | ||
|
|
||
| ### Cluster-fleet evaluation considerations |
There was a problem hiding this comment.
cc @flavianmissi to take a look too as it involves the cluster-image-registry-operator reporting this condition.
There was a problem hiding this comment.
@flavianmissi it would be great if you could review this change to see if it looks ok to you.
There was a problem hiding this comment.
apologies for the delayed review - it lgtm!
Foloowing up on conversations around not altering scripts and oc commands behaviour for importing imagestreams as a single or manifestlist, this enhancement proposes a way to dynamically set the import mode at install/upgrade time or even toggle it manually. https://issues.redhat.com/browse/MULTIARCH-4552
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
This is the api implementation for openshift/enhancements#1605 - Introduces ImageStreamImportMode field in the image config spec and status - the status will be consumed by apiserver to set importMode for all imagestreams - Introduce an `ImageStreamImportMode` tech preview feature gate - Add feature gate test
As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR sets the status field in the image config based on the spec setting so it can be synced by the apiserver's observedconfig to set the import mode for imagestreams.
…ed config changes in the image config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the observed config, checks if the importmode string is present and uses it as the default when creating imagestreams.
…erved config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the image config status, checks if the importmode string is present and syncs that value in the imagepolicyconfig of the observed config to be used by the apiserver.
As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR sets the status field in the image config based on the spec setting so it can be synced by the apiserver's observedconfig to set the import mode for imagestreams.
…ed config changes in the image config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the observed config, checks if the importmode string is present and uses it as the default when creating imagestreams.
…erved config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the image config status, checks if the importmode string is present and syncs that value in the imagepolicyconfig of the observed config to be used by the apiserver.
…erved config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the image config status, checks if the importmode string is present and syncs that value in the imagepolicyconfig of the observed config to be used by the apiserver.
…ed config changes in the image config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the observed config, checks if the importmode string is present and uses it as the default when creating imagestreams.
…erved config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the image config status, checks if the importmode string is present and syncs that value in the imagepolicyconfig of the observed config to be used by the apiserver.
…ed config changes in the image config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the observed config, checks if the importmode string is present and uses it as the default when creating imagestreams.
…erved config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the image config status, checks if the importmode string is present and syncs that value in the imagepolicyconfig of the observed config to be used by the apiserver.
…erved config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the image config status, checks if the importmode string is present and syncs that value in the imagepolicyconfig of the observed config to be used by the apiserver.
…erved config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the image config status, checks if the importmode string is present and syncs that value in the imagepolicyconfig of the observed config to be used by the apiserver.
…ed config changes in the image config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the observed config, checks if the importmode string is present and uses it as the default when creating imagestreams.
…ed config changes in the image config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the observed config, checks if the importmode string is present and uses it as the default when creating imagestreams.
…ed config changes in the image config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the observed config, checks if the importmode string is present and uses it as the default when creating imagestreams.
6 participants
Following up on conversations around not altering scripts and oc commands behaviour for importing imagestreams as a single or manifestlist, this enhancement proposes a way to dynamically set the import mode at install/upgrade time or even toggle it manually.
https://issues.redhat.com/browse/MULTIARCH-4552