From a07710001c187890b01b079303a72b7e84ceaf39 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Tue, 16 Sep 2025 17:43:51 -0700 Subject: [PATCH 1/2] pkg/cincinnati: Centralize release metadata parsing Instead of carring similar code in the payload and Cincinnati packages, centralize in one place to make maintenance easier. And with the logic centralized, I've also put some time into hardening the channel parsing, to grumble about some possible issues. For payload loading, errors get a logged warning, but are not fatal. Having the CVO come up with logged warnings still allows cluster admins to update into a fix. But a crash-looping CVO would not notice ClusterVersion spec.desiredUpdate changes or be able to roll out a requested update into a fix. So this branch processes as much of the metadata as it can (to not let, for example, an invalid URL block us from loading architecture information), logs the warning about what it failed to parse, and continues on. For Cincinnati processing, errors are fatal. Cluster admins can take the ClusterVersion RetrievedUpdates=False message and complain to their Update Service maintainers, who can fix the metadata. --- pkg/cincinnati/cincinnati.go | 82 ++++++++++++++++++++++--------- pkg/cincinnati/cincinnati_test.go | 4 +- pkg/payload/payload.go | 41 ++-------------- 3 files changed, 67 insertions(+), 60 deletions(-) diff --git a/pkg/cincinnati/cincinnati.go b/pkg/cincinnati/cincinnati.go index 86a8721b9..9a0a12e19 100644 --- a/pkg/cincinnati/cincinnati.go +++ b/pkg/cincinnati/cincinnati.go @@ -3,6 +3,7 @@ package cincinnati import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -289,9 +290,9 @@ type graph struct { } type node struct { - Version semver.Version `json:"version"` - Image string `json:"payload"` - Metadata map[string]string `json:"metadata,omitempty"` + Version semver.Version `json:"version"` + Image string `json:"payload"` + Metadata map[string]interface{} `json:"metadata,omitempty"` } type edge struct { @@ -329,28 +330,65 @@ func (e *edge) UnmarshalJSON(data []byte) error { } func convertRetrievedUpdateToRelease(update node) (configv1.Release, error) { - cvoUpdate := configv1.Release{ - Version: update.Version.String(), - Image: update.Image, - } - if urlString, ok := update.Metadata["url"]; ok { - _, err := url.Parse(urlString) - if err != nil { - return cvoUpdate, fmt.Errorf("invalid URL for %s: %s", cvoUpdate.Version, err) + release, err := ParseMetadata(update.Metadata) + release.Version = update.Version.String() + release.Image = update.Image + return release, err +} + +// ParseMetadata parses release metadata (URL, channels, etc.). It does not populate the version or image properties. +func ParseMetadata(metadata map[string]interface{}) (configv1.Release, error) { + release := configv1.Release{} + errs := []error{} + if urlInterface, hasURL := metadata["url"]; hasURL { + if urlString, isString := urlInterface.(string); isString { + if _, err := url.Parse(urlString); err == nil { + release.URL = configv1.URL(urlString) + } else { + errs = append(errs, fmt.Errorf("invalid release URL: %s", err)) + } + } else { + errs = append(errs, fmt.Errorf("URL is not a string: %v", urlInterface)) } - cvoUpdate.URL = configv1.URL(urlString) } - if arch, ok := update.Metadata["release.openshift.io/architecture"]; ok { - switch arch { - case "multi": - cvoUpdate.Architecture = configv1.ClusterVersionArchitectureMulti - default: - klog.Warningf("Unrecognized release.openshift.io/architecture value %q", arch) + if archInterface, hasArch := metadata["release.openshift.io/architecture"]; hasArch { + if arch, isString := archInterface.(string); isString { + if arch == "multi" { + release.Architecture = configv1.ClusterVersionArchitectureMulti + } else { + errs = append(errs, fmt.Errorf("unrecognized release.openshift.io/architecture value %q", arch)) + } + } else { + errs = append(errs, fmt.Errorf("release.openshift.io/architecture is not a string: %v", archInterface)) } } - if channels, ok := update.Metadata["io.openshift.upgrades.graph.release.channels"]; ok { - cvoUpdate.Channels = strings.Split(channels, ",") - sort.Strings(cvoUpdate.Channels) + if channelsInterface, hasChannels := metadata["io.openshift.upgrades.graph.release.channels"]; hasChannels { + if channelsString, isString := channelsInterface.(string); isString { + if len(channelsString) == 0 { + errs = append(errs, fmt.Errorf("io.openshift.upgrades.graph.release.channels is an empty string")) + } else { + channels := strings.Split(channelsString, ",") + if len(channels) == 0 { + errs = append(errs, fmt.Errorf("no comma-delimited channels in io.openshift.upgrades.graph.release.channels %q", channelsString)) + } else { + for i := len(channels) - 1; i >= 0; i-- { + if len(channels[i]) == 0 { + errs = append(errs, fmt.Errorf("io.openshift.upgrades.graph.release.channels entry %d is an empty string: %q", i, channelsString)) + channels = append(channels[:i], channels[i+1:]...) + } + } + if len(channels) == 0 { + errs = append(errs, fmt.Errorf("no non-empty channels in io.openshift.upgrades.graph.release.channels %q", channels)) + } else { + sort.Strings(channels) + release.Channels = channels + } + } + } + } else { + errs = append(errs, fmt.Errorf("io.openshift.upgrades.graph.release.channels is not a string: %v", channelsInterface)) + } } - return cvoUpdate, nil + klog.V(2).Infof("parsed metadata: URL %q, architecture %q, channels %v, errors %v", release.URL, release.Architecture, release.Channels, errs) + return release, errors.Join(errs...) } diff --git a/pkg/cincinnati/cincinnati_test.go b/pkg/cincinnati/cincinnati_test.go index b482fa0b1..15e1c02d0 100644 --- a/pkg/cincinnati/cincinnati_test.go +++ b/pkg/cincinnati/cincinnati_test.go @@ -816,7 +816,7 @@ func Test_nodeUnmarshalJSON(t *testing.T) { exp: node{ Version: semver.MustParse("4.0.0-5"), Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-5", - Metadata: map[string]string{}, + Metadata: map[string]interface{}{}, }, }, { raw: []byte(`{ @@ -829,7 +829,7 @@ func Test_nodeUnmarshalJSON(t *testing.T) { exp: node{ Version: semver.MustParse("4.0.0-0.1"), Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-0.1", - Metadata: map[string]string{ + Metadata: map[string]interface{}{ "description": "This is the beta1 image based on the 4.0.0-0.nightly-2019-01-15-010905 build", }, }, diff --git a/pkg/payload/payload.go b/pkg/payload/payload.go index 88e88365a..71c66f3bd 100644 --- a/pkg/payload/payload.go +++ b/pkg/payload/payload.go @@ -10,8 +10,6 @@ import ( "os" "path/filepath" "runtime" - "sort" - "strings" "time" "k8s.io/klog/v2" @@ -27,6 +25,7 @@ import ( "github.com/openshift/cluster-version-operator/lib/capability" localmanifest "github.com/openshift/cluster-version-operator/lib/manifest" "github.com/openshift/cluster-version-operator/lib/resourceread" + "github.com/openshift/cluster-version-operator/pkg/cincinnati" ) // State describes the state of the payload and alters @@ -68,9 +67,6 @@ const ( // provide better visibility during install and upgrade of // error conditions. PrecreatingPayload - - // releaseMultiArchID identifies a multi architecture release. - releaseMultiArchID = "multi" ) // Initializing is true if the state is InitializingPayload. @@ -368,39 +364,12 @@ func loadReleaseMetadata(releaseDir string) (configv1.Release, error) { return release, fmt.Errorf("Cincinnati metadata version %q is not a valid semantic version: %v", metadata.Version, err) } - release.Version = metadata.Version - - if archRaw, hasArch := metadata.Metadata["release.openshift.io/architecture"]; hasArch { - arch, isString := archRaw.(string) - if !isString { - return release, fmt.Errorf("Architecture from %s (%s) is not a string: %v", - cincinnatiJSONFile, release.Version, archRaw) - } - - if arch != releaseMultiArchID { - return release, fmt.Errorf("Architecture from %s (%s) contains invalid value: %q. Valid value is %q.", - cincinnatiJSONFile, release.Version, arch, releaseMultiArchID) - } - - release.Architecture = configv1.ClusterVersionArchitectureMulti - klog.V(2).Infof("Architecture from %s (%s) is multi: %q", cincinnatiJSONFile, release.Version, arch) + release, err = cincinnati.ParseMetadata(metadata.Metadata) + if err != nil { + klog.Warningf("errors while parsing metadata from %s (%s): %v", cincinnatiJSONFile, release.Version, err) } - if urlInterface, ok := metadata.Metadata["url"]; ok { - if urlString, ok := urlInterface.(string); ok { - release.URL = configv1.URL(urlString) - } else { - klog.Warningf("URL from %s (%s) is not a string: %v", cincinnatiJSONFile, release.Version, urlInterface) - } - } - if channelsInterface, ok := metadata.Metadata["io.openshift.upgrades.graph.release.channels"]; ok { - if channelsString, ok := channelsInterface.(string); ok { - release.Channels = strings.Split(channelsString, ",") - sort.Strings(release.Channels) - } else { - klog.Warningf("channel list from %s (%s) is not a string: %v", cincinnatiJSONFile, release.Version, channelsInterface) - } - } + release.Version = metadata.Version return release, nil } From 9186228714648412ff7a787075823ce4814ebd57 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 3 Oct 2025 12:08:37 -0700 Subject: [PATCH 2/2] pkg/cvo: Log failures to merge release metadata Make it easier to understand when the cluster-version operator does not update status.desired.channels and such [1]. [1]: https://issues.redhat.com/browse/OTA-1627 --- pkg/cvo/cvo.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index 6f186479f..59b1da6ce 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -918,7 +918,24 @@ func mergeReleaseMetadata(release configv1.Release, getAvailableUpdates func() * } } } - if update != nil { + if update == nil { + var versionMatch *configv1.Release + if availableUpdates.Current.Version == merged.Version { + versionMatch = &availableUpdates.Current + } else { + for i, u := range availableUpdates.Updates { + if u.Version == merged.Version { + versionMatch = &availableUpdates.Updates[i] + break + } + } + } + if versionMatch == nil { + klog.V(2).Infof("No available update found matching the digest of %q or the version %q", merged.Image, merged.Version) + } else { + klog.V(2).Infof("No available update found matching the digest of %q, although there was a match for version %q with a different tag or digest %q", merged.Image, merged.Version, versionMatch.Image) + } + } else { if merged.Version == "" { merged.Version = update.Version }