Skip to content

OTA-1531: [2/x] cvo: refactor LoadUpdate #1185

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 3 commits into from
May 7, 2025
Merged

OTA-1531: [2/x] cvo: refactor LoadUpdate #1185

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76e78a0
refactor(payload): separate tasks and metadata loading
petr-muller May 2, 2025
e8648c7
refactor(payload): separate arch load from metadata and runtime
petr-muller May 7, 2025
4fe21f6
refactor(payload): rename `loadReleaseFromMetadata`->`loadReleaseMeta…
petr-muller May 7, 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
100 changes: 51 additions & 49 deletions pkg/payload/payload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,22 @@ type metadata struct {

func LoadUpdate(dir, releaseImage, excludeIdentifier string, requiredFeatureSet string, profile string,
knownCapabilities []configv1.ClusterVersionCapability) (*Update, error) {
klog.V(2).Infof("Loading updatepayload from %q", dir)
if err := ValidateDirectory(dir); err != nil {
return nil, err
}
var (
cvoDir = filepath.Join(dir, CVOManifestDir)
releaseDir = filepath.Join(dir, ReleaseManifestDir)
)
Comment on lines +143 to +150
Copy link
Member Author

Choose a reason for hiding this comment

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

Just moved here from loadUpdatePayloadMetadata


payload, tasks, err := loadUpdatePayloadMetadata(dir, releaseImage, profile)
payload, err := loadPayloadMetadata(releaseDir, releaseImage)
if err != nil {
return nil, err
}

tasks := loadPayloadTasks(releaseDir, cvoDir, releaseImage, profile)
Copy link
Member Author

Choose a reason for hiding this comment

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

Just moved here from loadUpdatePayloadMetadata


var onlyKnownCaps *configv1.ClusterVersionCapabilitiesStatus

if knownCapabilities != nil {
Expand Down Expand Up @@ -293,47 +303,42 @@ func ValidateDirectory(dir string) error {
return nil
}

type payloadTasks struct {
idir string
preprocess func([]byte) ([]byte, error)
skipFiles sets.Set[string]
}

func loadUpdatePayloadMetadata(dir, releaseImage, clusterProfile string) (*Update, []payloadTasks, error) {
klog.V(2).Infof("Loading updatepayload from %q", dir)
if err := ValidateDirectory(dir); err != nil {
return nil, nil, err
}
var (
cvoDir = filepath.Join(dir, CVOManifestDir)
releaseDir = filepath.Join(dir, ReleaseManifestDir)
)

release, arch, err := loadReleaseFromMetadata(releaseDir)
func loadPayloadMetadata(releaseDir, releaseImage string) (*Update, error) {
release, err := loadReleaseMetadata(releaseDir)
if err != nil {
return nil, nil, err
return nil, err
}
release.Image = releaseImage

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

if imageRef.Name != release.Version {
return nil, nil, fmt.Errorf("Version from %s (%s) differs from %s (%s)", imageReferencesFile, imageRef.Name, cincinnatiJSONFile, release.Version)
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)
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved here from loadReleaseFromMetadata

Copy link
Member

Choose a reason for hiding this comment

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

nit: I would move this part right after line 307.
Then I would not need to check if arch is used between 307 and 318.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a good point, I'll move it right after the error check in a followup PR #1188

}

tasks := getPayloadTasks(releaseDir, cvoDir, releaseImage, clusterProfile)
if imageRef.Name != release.Version {
return nil, fmt.Errorf("Version from %s (%s) differs from %s (%s)", imageReferencesFile, imageRef.Name, cincinnatiJSONFile, release.Version)
}

return &Update{
Release: release,
ImageRef: imageRef,
Architecture: arch,
}, tasks, nil
}, nil
}

type payloadTasks struct {
Copy link
Member

Choose a reason for hiding this comment

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

super nit: Without moving this struct (feels like the code did not change) it would be easier to comparing the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I just wanted this struct closer to loadPayloadTasks that creates this data

idir string
preprocess func([]byte) ([]byte, error)
skipFiles sets.Set[string]
}

func getPayloadTasks(releaseDir, cvoDir, releaseImage, clusterProfile string) []payloadTasks {
func loadPayloadTasks(releaseDir, cvoDir, releaseImage, clusterProfile string) []payloadTasks {
cjf := filepath.Join(releaseDir, cincinnatiJSONFile)
irf := filepath.Join(releaseDir, imageReferencesFile)

Expand All @@ -353,52 +358,49 @@ func getPayloadTasks(releaseDir, cvoDir, releaseImage, clusterProfile string) []
}}
}

func loadReleaseFromMetadata(releaseDir string) (configv1.Release, string, error) {
func loadReleaseMetadata(releaseDir string) (configv1.Release, error) {
var release configv1.Release
path := filepath.Join(releaseDir, cincinnatiJSONFile)
data, err := os.ReadFile(path)
if err != nil {
return release, "", err
return release, err
}

var metadata metadata
if err := json.Unmarshal(data, &metadata); err != nil {
return release, "", fmt.Errorf("unmarshal Cincinnati metadata: %w", err)
return release, fmt.Errorf("unmarshal Cincinnati metadata: %w", err)
}

if metadata.Kind != "cincinnati-metadata-v0" {
return release, "", fmt.Errorf("unrecognized Cincinnati metadata kind %q", metadata.Kind)
return release, fmt.Errorf("unrecognized Cincinnati metadata kind %q", metadata.Kind)
}

if metadata.Version == "" {
return release, "", errors.New("missing required Cincinnati metadata version")
return release, errors.New("missing required Cincinnati metadata version")
}

if _, err := semver.Parse(metadata.Version); err != nil {
return release, "", fmt.Errorf("Cincinnati metadata version %q is not a valid semantic version: %v", metadata.Version, err)
return release, fmt.Errorf("Cincinnati metadata version %q is not a valid semantic version: %v", metadata.Version, err)
}

release.Version = metadata.Version

var arch string
if archInterface, ok := metadata.Metadata["release.openshift.io/architecture"]; ok {
if archString, ok := archInterface.(string); ok {
if archString == releaseMultiArchID {
release.Architecture = configv1.ClusterVersionArchitectureMulti
arch = string(release.Architecture)
} else {
return release, "", fmt.Errorf("Architecture from %s (%s) contains invalid value: %q. Valid value is %q.",
cincinnatiJSONFile, release.Version, archString, releaseMultiArchID)
}
klog.V(2).Infof("Architecture from %s (%s) is multi: %q", cincinnatiJSONFile, release.Version, archString)
} else {
return release, "", fmt.Errorf("Architecture from %s (%s) is not a string: %v",
cincinnatiJSONFile, release.Version, archInterface)
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)
}
} else {
arch = runtime.GOARCH
klog.V(2).Infof("Architecture from %s (%s) retrieved from runtime: %q", cincinnatiJSONFile, release.Version, arch)

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)
Copy link
Member Author

Choose a reason for hiding this comment

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

The "metadata does not have architecture key, lets read runtime" part moved one layer above to loadPayloadMetadata

Otherwise the logic is identical, just was refactored into guard clause sequence.

}

if urlInterface, ok := metadata.Metadata["url"]; ok {
if urlString, ok := urlInterface.(string); ok {
release.URL = configv1.URL(urlString)
Expand All @@ -415,7 +417,7 @@ func loadReleaseFromMetadata(releaseDir string) (configv1.Release, string, error
}
}

return release, arch, nil
return release, nil
}

func loadImageReferences(releaseDir string) (*imagev1.ImageStream, error) {
Expand Down