Skip to content

OTA-1531: [3/x] cvo: read version from release metadata on startup #1188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 12, 2025
Merged

OTA-1531: [3/x] cvo: read version from release metadata on startup #1188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca30fcb
cvo: read version from release metadata on startup
petr-muller May 7, 2025
a6cf93c
refactor: check release metadata arch earlier
petr-muller May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions pkg/payload/payload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,19 +308,20 @@ func loadPayloadMetadata(releaseDir, releaseImage string) (*Update, error) {
if err != nil {
return nil, err
}
release.Image = releaseImage

imageRef, err := loadImageReferences(releaseDir)
if err != nil {
return nil, err
}

arch := string(release.Architecture)
if arch == "" {
arch = runtime.GOARCH
klog.V(2).Infof("Architecture from %s (%s) retrieved from runtime: %q", cincinnatiJSONFile, release.Version, arch)
}

release.Image = releaseImage

imageRef, err := loadImageReferences(releaseDir)
if err != nil {
return nil, err
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This move only addresses a review comment from an earlier PR: #1185 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is interesting that github chose to interpret the change as stanza from 311 (the release part) is moved down to 318, instead of stanza 318 (the arch part) is moved up.

Github did not read our conversation carefully enough. 😄

if imageRef.Name != release.Version {
return nil, fmt.Errorf("Version from %s (%s) differs from %s (%s)", imageReferencesFile, imageRef.Name, cincinnatiJSONFile, release.Version)
}
Expand Down Expand Up @@ -358,6 +359,17 @@ func loadPayloadTasks(releaseDir, cvoDir, releaseImage, clusterProfile string) [
}}
}

// RootPath represents a path to the directory containing the payload
type RootPath string

const DefaultRootPath = RootPath(DefaultPayloadDir)

// LoadReleaseMetadata loads the release metadata from the appropriate location inside the payload directory
func (p RootPath) LoadReleaseMetadata() (configv1.Release, error) {
releaseDir := filepath.Join(string(p), ReleaseManifestDir)
return loadReleaseMetadata(releaseDir)
}

func loadReleaseMetadata(releaseDir string) (configv1.Release, error) {
var release configv1.Release
path := filepath.Join(releaseDir, cincinnatiJSONFile)
Expand Down
25 changes: 25 additions & 0 deletions pkg/start/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,31 @@ func (o *Options) Run(ctx context.Context) error {
return err
}

payloadRoot := payload.DefaultRootPath
if o.PayloadOverride != "" {
payloadRoot = payload.RootPath(o.PayloadOverride)
}

cvoOcpVersion := "0.0.1-snapshot"
// Peek at the local release metadata to determine the version of OCP this CVO belongs to. This assumes the CVO is
// executing in a container from the payload image. Full payload content is only read later once leader lease is
// acquired, and here we should only read as little data as possible to determine the version so we can establish
// enabled feature gate checker for all following code.
//
// We cannot refuse to start CVO if for some reason we cannot determine the OCP version on startup from the local
// release metadata. The only consequence is we fail to determine enabled/disabled feature gates and will have to use
// some defaults.
releaseMetadata, err := payloadRoot.LoadReleaseMetadata()
switch {
case err != nil:
klog.Warningf("Failed to read release metadata to determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err)
case releaseMetadata.Version == "":
klog.Warningf("Version missing from release metadata, cannot determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would err have to be nil when it reaches this case?

Suggested change
klog.Warningf("Version missing from release metadata, cannot determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err)
klog.Warningf("Version missing from release metadata, cannot determine OCP version for this CVO (will use placeholder version %q)", cvoOcpVersion)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will address

default:
cvoOcpVersion = releaseMetadata.Version
klog.Infof("Determined OCP version for this CVO: %q", cvoOcpVersion)
Comment on lines +213 to +214
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT:

Suggested change
cvoOcpVersion = releaseMetadata.Version
klog.Infof("Determined OCP version for this CVO: %q", cvoOcpVersion)
klog.Infof("Determined OCP version for this CVO: %q", releaseMetadata.Version)

(Definitely just a matter of choice) I would use "0.0.1-snapshot" in the other 2 places as well (and we could remove cvoOcpVersion). For example,

klog.Warningf("Failed to read release metadata to determine OCP version for this CVO (will use placeholder '0.0.1-snapshot'): %v", err)

I feel it would be easier a bit to read the code that way and without having to think if cvoOcpVersion is used later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was kind of expecting releaseMetadata.Version is used somewhere because it's value has been determined earlier. Will I see it in another PR soon?

Not the releaseMetadata.Version directly but cvoOcpVersion will be used later, it will either have the value read from the release metadata or the placeholder one. It makes sense to log the value that will be eventually used, and also we should keep the placeholder value in a single place, not inline it in logging strings.

Check out the followups:

The "determine OCP version" code will be made into a method:

cluster-version-operator/pkg/start/start.go

Lines 217 to 243 in 280e27f

// getOcpVersion peeks at the local release metadata to determine the version of OCP this CVO belongs to. This assumes
// the CVO is executing in a container from the payload image. This does not and should not fully load whole payload
// content, that is only loaded later once leader lease is acquired. Here we should only read as little data as possible
// to determine the version so we can establish enabled feature gate checker for all following code.
func (o *Options) getOcpVersion() string {
payloadRoot := payload.DefaultRootPath
if o.PayloadOverride != "" {
payloadRoot = payload.RootPath(o.PayloadOverride)
}
cvoOcpVersion := "0.0.1-snapshot"
// We cannot refuse to start CVO if for some reason we cannot determine the OCP version on startup from the local
// release metadata. The only consequence is we fail to determine enabled/disabled feature gates and will have to use
// some defaults.
releaseMetadata, err := payloadRoot.LoadReleaseMetadata()
switch {
case err != nil:
klog.Warningf("Failed to read release metadata to determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err)
case releaseMetadata.Version == "":
klog.Warningf("Version missing from release metadata, cannot determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err)
default:
cvoOcpVersion = releaseMetadata.Version
klog.Infof("Determined OCP version for this CVO: %q", cvoOcpVersion)
}
return cvoOcpVersion
}

And the determined version will be used to determine enable CVO feature gates:

cluster-version-operator/pkg/start/start.go

Lines 253 to 254 in 280e27f

cvoOcpVersion := o.getOcpVersion()
cvoGates := featuregates.DefaultCvoGates(cvoOcpVersion)

}

// initialize the controllers and attempt to load the payload information
controllerCtx, err := o.NewControllerContext(cb)
if err != nil {
Expand Down