OCPBUGS-48641: config/v1/types_cluster_version: Explain image and version both set #2158
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,7 @@ type ClusterVersionSpec struct { | |
| // | ||
| // Some of the fields are inter-related with restrictions and meanings described here. | ||
| // 1. image is specified, version is specified, architecture is specified. API validation error. | ||
| // 2. image is specified, version is specified, architecture is not specified. The version extracted from the referenced image must match the specified version. | ||
| // 3. image is specified, version is not specified, architecture is specified. API validation error. | ||
| // 4. image is specified, version is not specified, architecture is not specified. image is used. | ||
| // 5. image is not specified, version is specified, architecture is specified. version and desired architecture are used to select an image. | ||
|
|
@@ -702,16 +702,16 @@ type Update struct { | |
| Architecture ClusterVersionArchitecture `json:"architecture"` | ||
|
|
||
| // version is a semantic version identifying the update version. | ||
| // version is required if architecture is specified. | ||
|
Contributor
There was a problem hiding this comment. You could add a CEL rule to validate this. We can test that it ratchets so that existing broken resources do not suddenly become broken
Member
Author
There was a problem hiding this comment. "version is ... required if architecture is specified" dates back to #1339, and seems orthogonal to the change I'm suggesting here. And actually,
Contributor
There was a problem hiding this comment. I agree it's orthogonal, but it's good to make compatible incremental change as we are touching areas of APIs. What happens if version is missing when architecture is specified today? Ignoring CLI tooling that would set it, since folks can manipulate these resources themselves, would it cause CVO to return errors when it processes the object? If so, adding a CEL rule as below would give more immediate feedback to a user, and is relatively free to us to implement. As of 4.18 this should ratchet itself, but we would need to test it. A self ratcheting version
Member
Author
There was a problem hiding this comment.
Looks like that's already guarded here. With a $ oc patch clusterversion version --type json -p '[{"op": "add", "path": "/spec/desiredUpdate", "value": {"architecture": "Multi"}}]'
The ClusterVersion "version" is invalid: spec.desiredUpdate: Invalid value: "object": no such key: version evaluating rule: Version must be set if Architecture is setSo I can leave the |
||
| // If both version and image are set, the version extracted from the referenced image must match the specified version. | ||
| // | ||
| // +optional | ||
| Version string `json:"version"` | ||
|
|
||
| // image is a container image location that contains the update. | ||
| // image should be used when the desired version does not exist in availableUpdates or history. | ||
| // When image is set, architecture cannot be specified. | ||
| // If both version and image are set, the version extracted from the referenced image must match the specified version. | ||
| // | ||
| // +optional | ||
| Image string `json:"image"` | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poking a bit more deeply into where the nominal "ignored" came from, on 2022-11-08 I claimed it was ignored. I'm not sure what 2022-me was thinking there; possibly I was just focused on how the CVO looks up which image to use (and that logic doesn't run when
imageis explicitly set inspec), and I overlooked the sync-worker validation as it judges the requesteddesiredUpdateforReleaseAccepted?