Repository: openshift/assisted-service (language: python)

Issue:
MGMT-16813: Prioritize finding release image with exact CPU architecture
<!--
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

You can refer to [Kubernetes community documentation] on writing good commit messages, which provides good tips and ideas.

Some PRs address specific issues. Please, refer to the [CONTRIBUTING] documentation for more
information on how to link a PR to an existing issue.

It's recommended to take a few extra minutes to provide more information about
how this code was tested. Here are some questions that may be worth answering:

- Should this PR be tested by the reviewer?
- Is this PR relying on CI for an e2e test run?
- Should this PR be tested in a specific environment?
- Any logs, screenshots, etc that can help with the review process?

-->
https://issues.redhat.com/browse/MGMT-16813
Previously, the `getReleaseImageFromCache` would get all release
images that included the CPU architecture and return the first
instance it found that matched the desired Openshift version.

Since multi-arch images include the CPU architecture, it led
to undesireable selection of the multi-arch image over the
exact matching image with the specified CPU architecture.

This fix ensures that the release image will be found with
the following priority:
1. Exact CPU Architecture & OpenShift x.y.z version match
2. Exact CPU Architecture & OpenShift Major Minor (x.y) version match
3. Multi-arch CPU with specified CPU Architecture & OpenShift x.y.z version match
4. Multi-arch CPU with specified CPU Architecture & OpenShift Major Minor (x.y) version match

## List all the issues related to this PR

- [ ] New Feature <!-- new functionality -->
- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
- [x] Bug fix
- [ ] Tests
- [ ] Documentation
- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->

## What environments does this code impact?

- [ ] Automation (CI, tools, etc)
- [ ] Cloud
- [x] Operator Managed Deployments
- [ ] None

## How was this code tested?

<!-- Please, select one or more if needed: -->

- [ ] assisted-test-infra environment
- [ ] dev-scripts environment
- [ ] Reviewer's test appreciated
- [x] Waiting for CI to do a full test run
- [ ] Manual (Elaborate on how it was tested)
- [ ] No tests needed

## Checklist

- [x] Title and description added to both, commit and PR.
- [x] Relevant issues have been associated (see [CONTRIBUTING] guide)
- [x] This change does not require a documentation update (docstring, `docs`, README, etc)
- [ ] Does this change include unit-tests (note that code changes require unit-tests)

## Reviewers Checklist

- Are the title and description (in both PR and commit) meaningful and clear?
- Is there a bug required (and linked) for this change?
- Should this PR be backported?

[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines
[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md

**note**: will push up code gen once it finishes


Discussion / hints:
@CrystalChun: This pull request references [MGMT-16813](https://issues.redhat.com//browse/MGMT-16813) which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

<details>

In response to [this](https://github.com/openshift/assisted-service/pull/6066):

><!--
>Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
>
>You can refer to [Kubernetes community documentation] on writing good commit messages, which provides good tips and ideas.
>
>Some PRs address specific issues. Please, refer to the [CONTRIBUTING] documentation for more
>information on how to link a PR to an existing issue.
>
>It's recommended to take a few extra minutes to provide more information about
>how this code was tested. Here are some questions that may be worth answering:
>
>- Should this PR be tested by the reviewer?
>- Is this PR relying on CI for an e2e test run?
>- Should this PR be tested in a specific environment?
>- Any logs, screenshots, etc that can help with the review process?
>
>-->
>**Problem**: There have been instances where the `AgentClusterInstall` will specify to use a specific `ClusterImageSet`, but assisted will use a completely different image than the one specified.
>
>This is from the cluster deployment initially finding the release image, but only passing the release version and architecture to the `RegisterClusterInternal` function. 
>
>In the `RegisterClusterInternal` function, the assisted-service calls get release image again, but this time will choose it based off of its cache of images, which is not ordered, so it might choose a release image with the same corresponding version, but incorrect architecture. 
>
>**Proposed solution**: Allow the ClusterDeployment controller to pass in a release image that it already found via the `ClusterImageSet` to the `RegisterClusterInternal` function so that the cluster can actually use the release image that was specified. 
>
>## List all the issues related to this PR
>
>- [ ] New Feature <!-- new functionality -->
>- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
>- [x] Bug fix
>- [ ] Tests
>- [ ] Documentation
>- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->
>
>## What environments does this code impact?
>
>- [ ] Automation (CI, tools, etc)
>- [ ] Cloud
>- [x] Operator Managed Deployments
>- [ ] None
>
>## How was this code tested?
>
><!-- Please, select one or more if needed: -->
>
>- [ ] assisted-test-infra environment
>- [ ] dev-scripts environment
>- [ ] Reviewer's test appreciated
>- [x] Waiting for CI to do a full test run
>- [ ] Manual (Elaborate on how it was tested)
>- [ ] No tests needed
>
>## Checklist
>
>- [x] Title and description added to both, commit and PR.
>- [x] Relevant issues have been associated (see [CONTRIBUTING] guide)
>- [x] This change does not require a documentation update (docstring, `docs`, README, etc)
>- [ ] Does this change include unit-tests (note that code changes require unit-tests)
>
>## Reviewers Checklist
>
>- Are the title and description (in both PR and commit) meaningful and clear?
>- Is there a bug required (and linked) for this change?
>- Should this PR be backported?
>
>[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines
>[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md
>
>**note**: will push up code gen once it finishes


Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service).  If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
</details>
Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with `/test all`
@CrystalChun: This pull request references [MGMT-16813](https://issues.redhat.com//browse/MGMT-16813) which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

<details>

In response to [this](https://github.com/openshift/assisted-service/pull/6066):

><!--
>Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
>
>You can refer to [Kubernetes community documentation] on writing good commit messages, which provides good tips and ideas.
>
>Some PRs address specific issues. Please, refer to the [CONTRIBUTING] documentation for more
>information on how to link a PR to an existing issue.
>
>It's recommended to take a few extra minutes to provide more information about
>how this code was tested. Here are some questions that may be worth answering:
>
>- Should this PR be tested by the reviewer?
>- Is this PR relying on CI for an e2e test run?
>- Should this PR be tested in a specific environment?
>- Any logs, screenshots, etc that can help with the review process?
>
>-->
>https://issues.redhat.com/browse/MGMT-16813
>Previously, the `getReleaseImageFromCache` would get all release
>images that included the CPU architecture and return the first
>instance it found that matched the desired Openshift version.
>
>Since multi-arch images include the CPU architecture, it led
>to undesireable selection of the multi-arch image over the
>exact matching image with the specified CPU architecture.
>
>This fix ensures that the release image will be found with
>the following priority:
>1. Exact CPU Architecture & OpenShift x.y.z version match
>2. Exact CPU Architecture & OpenShift Major Minor (x.y) version match
>3. Multi-arch CPU with specified CPU Architecture & OpenShift x.y.z version match
>4. Multi-arch CPU with specified CPU Architecture & OpenShift Major Minor (x.y) version match
>
>## List all the issues related to this PR
>
>- [ ] New Feature <!-- new functionality -->
>- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
>- [x] Bug fix
>- [ ] Tests
>- [ ] Documentation
>- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->
>
>## What environments does this code impact?
>
>- [ ] Automation (CI, tools, etc)
>- [ ] Cloud
>- [x] Operator Managed Deployments
>- [ ] None
>
>## How was this code tested?
>
><!-- Please, select one or more if needed: -->
>
>- [ ] assisted-test-infra environment
>- [ ] dev-scripts environment
>- [ ] Reviewer's test appreciated
>- [x] Waiting for CI to do a full test run
>- [ ] Manual (Elaborate on how it was tested)
>- [ ] No tests needed
>
>## Checklist
>
>- [x] Title and description added to both, commit and PR.
>- [x] Relevant issues have been associated (see [CONTRIBUTING] guide)
>- [x] This change does not require a documentation update (docstring, `docs`, README, etc)
>- [ ] Does this change include unit-tests (note that code changes require unit-tests)
>
>## Reviewers Checklist
>
>- Are the title and description (in both PR and commit) meaningful and clear?
>- Is there a bug required (and linked) for this change?
>- Should this PR be backported?
>
>[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines
>[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md
>
>**note**: will push up code gen once it finishes


Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service).  If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
</details>
/retitle [MGMT-16813](https://issues.redhat.com//browse/MGMT-16813): Prioritize finding release image with exact CPU architecture

## [Codecov](https://app.codecov.io/gh/openshift/assisted-service/pull/6066?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 68.40%. Comparing base [(`eaab373`)](https://app.codecov.io/gh/openshift/assisted-service/commit/eaab3736514e20a320733252f787a6f9b581179d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) to head [(`4a9e43a`)](https://app.codecov.io/gh/openshift/assisted-service/pull/6066?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift).
> Report is 9 commits behind head on master.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/openshift/assisted-service/pull/6066/graphs/tree.svg?width=650&height=150&src=pr&token=YOR4NSSOXQ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)](https://app.codecov.io/gh/openshift/assisted-service/pull/6066?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)

```diff
@@            Coverage Diff             @@
##           master    #6066      +/-   ##
==========================================
- Coverage   68.41%   68.40%   -0.01%     
==========================================
  Files         239      239              
  Lines       35457    35503      +46     
==========================================
+ Hits        24257    24285      +28     
- Misses       9097     9113      +16     
- Partials     2103     2105       +2     
```


| [Files](https://app.codecov.io/gh/openshift/assisted-service/pull/6066?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) | Coverage Δ | |
|---|---|---|
| [internal/versions/versions.go](https://app.codecov.io/gh/openshift/assisted-service/pull/6066?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift#diff-aW50ZXJuYWwvdmVyc2lvbnMvdmVyc2lvbnMuZ28=) | `81.06% <100.00%> (+1.38%)` | :arrow_up: |

... and [7 files with indirect coverage changes](https://app.codecov.io/gh/openshift/assisted-service/pull/6066/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)

</details>
/retest-required
[APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/openshift/assisted-service/pull/6066#pullrequestreview-1934658472" title="Approved">adriengentil</a>*, *<a href="https://github.com/openshift/assisted-service/pull/6066#" title="Author self-approved">CrystalChun</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=openshift%2Fassisted-service).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/openshift/assisted-service/blob/master/OWNERS)~~ [CrystalChun,adriengentil]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
/retest-required

Remaining retests: 0 against base HEAD 8c91757ccb30e5bd09f7d30fbaa5110c76db3163 and 2 for PR HEAD 4a9e43a15d5a25e42811117a52d3e4017d46c243 in total

@CrystalChun: all tests **passed!**


[Full PR test history](https://prow.ci.openshift.org/pr-history?org=openshift&repo=assisted-service&pr=6066). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:CrystalChun).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Fix patch:
diff --git a/internal/versions/versions.go b/internal/versions/versions.go
index 9f6d55fec89..b1dfa947333 100644
--- a/internal/versions/versions.go
+++ b/internal/versions/versions.go
@@ -166,50 +166,80 @@ func (h *handler) GetReleaseImageByURL(ctx context.Context, url, pullSecret stri
 	return h.addReleaseImage(url, pullSecret)
 }
 
+// Finds a release image from the cache that matches the specified version and architecture in the following
+// priority:
+// 1. Exact CPU Architecture & OpenShift x.y.z version match
+// 2. Exact CPU Architecture & OpenShift Major Minor (x.y) version match
+// 3. Multi-arch CPU with specified CPU Architecture & OpenShift x.y.z version match
+// 4. Multi-arch CPU with specified CPU Architecture & OpenShift Major Minor (x.y) version match
 func (h *handler) getReleaseImageFromCache(openshiftVersion, cpuArchitecture string) (*models.ReleaseImage, error) {
 	if cpuArchitecture == "" {
 		// Empty implies default CPU architecture
 		cpuArchitecture = common.DefaultCPUArchitecture
 	}
+
 	// Filter Release images by specified CPU architecture.
-	releaseImages := funk.Filter(h.releaseImages, func(releaseImage *models.ReleaseImage) bool {
-		for _, arch := range releaseImage.CPUArchitectures {
-			if arch == cpuArchitecture {
-				return true
+	exactCPUArchReleaseImages := funk.Filter(h.releaseImages, func(releaseImage *models.ReleaseImage) bool {
+		return swag.StringValue(releaseImage.CPUArchitecture) == cpuArchitecture
+	})
+
+	// Filter Release images by multi-arch images containing the specified CPU architecture
+	multiArchReleaseImages := funk.Filter(h.releaseImages, func(releaseImage *models.ReleaseImage) bool {
+		if *releaseImage.CPUArchitecture == common.MultiCPUArchitecture {
+			for _, arch := range releaseImage.CPUArchitectures {
+				if arch == cpuArchitecture {
+					return true
+				}
 			}
 		}
-		return swag.StringValue(releaseImage.CPUArchitecture) == cpuArchitecture
+		return false
 	})
-	if funk.IsEmpty(releaseImages) {
+
+	if funk.IsEmpty(exactCPUArchReleaseImages) && funk.IsEmpty(multiArchReleaseImages) {
 		return nil, errors.Errorf("The requested CPU architecture (%s) isn't specified in release images list", cpuArchitecture)
 	}
+
+	releaseImage := getReleaseImageByVersion(openshiftVersion, exactCPUArchReleaseImages.([]*models.ReleaseImage))
+	if releaseImage == nil {
+		// Find release image from multi-arch list if one doesn't exist in the exact CPU Arch list
+		releaseImage = getReleaseImageByVersion(openshiftVersion, multiArchReleaseImages.([]*models.ReleaseImage))
+	}
+
+	if releaseImage != nil {
+		return releaseImage, nil
+	}
+
+	return nil, errors.Errorf(
+		"The requested release image for version (%s) and CPU architecture (%s) isn't specified in release images list",
+		openshiftVersion, cpuArchitecture)
+}
+
+func getReleaseImageByVersion(desiredOpenshiftVersion string, releaseImages []*models.ReleaseImage) *models.ReleaseImage {
 	// Search for specified x.y.z openshift version
 	releaseImage := funk.Find(releaseImages, func(releaseImage *models.ReleaseImage) bool {
-		return *releaseImage.OpenshiftVersion == openshiftVersion
+		return *releaseImage.OpenshiftVersion == desiredOpenshiftVersion
 	})
 
 	if releaseImage == nil {
 		// Fallback to x.y version
-		versionKey, err := toMajorMinor(openshiftVersion)
+		majorMinorVersion, err := toMajorMinor(desiredOpenshiftVersion)
 		if err != nil {
-			return nil, err
+			return nil
 		}
+
 		releaseImage = funk.Find(releaseImages, func(releaseImage *models.ReleaseImage) bool {
 			// Starting from OCP 4.12 multi-arch release images do not have "-multi" suffix
 			// reported by the CVO, but we still offer the sufix internally to allow for explicit
 			// selection or single- or multi-arch payload.
 			version := strings.TrimSuffix(*releaseImage.OpenshiftVersion, "-multi")
-			return version == versionKey
+			return version == majorMinorVersion
 		})
 	}
 
 	if releaseImage != nil {
-		return releaseImage.(*models.ReleaseImage), nil
+		return releaseImage.(*models.ReleaseImage)
 	}
-
-	return nil, errors.Errorf(
-		"The requested release image for version (%s) and CPU architecture (%s) isn't specified in release images list",
-		openshiftVersion, cpuArchitecture)
+	return nil
 }
 
 // ValidateReleaseImageForRHCOS validates whether for a specified RHCOS version we have an OCP


Test patch:
diff --git a/internal/versions/versions_test.go b/internal/versions/versions_test.go
index 4532096fafb..0ee429f2dff 100644
--- a/internal/versions/versions_test.go
+++ b/internal/versions/versions_test.go
@@ -210,6 +210,13 @@ var _ = Describe("GetReleaseImage", func() {
 		Expect(*releaseImage.Version).Should(Equal("4.9-candidate"))
 	})
 
+	It("fetch release image by major.minor if given X.Y.Z version", func() {
+		releaseImage, err := h.GetReleaseImage(ctx, "4.9.2", common.DefaultCPUArchitecture, pullSecret)
+		Expect(err).ShouldNot(HaveOccurred())
+		Expect(*releaseImage.OpenshiftVersion).Should(Equal("4.9"))
+		Expect(*releaseImage.Version).Should(Equal("4.9-candidate"))
+	})
+
 	It("get from ReleaseImages", func() {
 		for _, key := range osImages.GetOpenshiftVersions() {
 			for _, architecture := range osImages.GetCPUArchitectures(key) {
@@ -270,6 +277,56 @@ var _ = Describe("GetReleaseImage", func() {
 		Expect(*releaseImage.Version).Should(Equal("4.12.999-rc.4"))
 	})
 
+	It("returns the matching CPU architecture over multi-arch if it is present", func() {
+		h.releaseImages = models.ReleaseImages{
+			&models.ReleaseImage{
+				CPUArchitecture:  swag.String(common.MultiCPUArchitecture),
+				CPUArchitectures: []string{common.X86CPUArchitecture, common.ARM64CPUArchitecture, common.PowerCPUArchitecture},
+				OpenshiftVersion: swag.String("4.12-multi"),
+				URL:              swag.String("release_4.12.999-multi"),
+				Version:          swag.String("4.12.999"),
+			},
+			&models.ReleaseImage{
+				CPUArchitecture:  swag.String(common.X86CPUArchitecture),
+				CPUArchitectures: []string{common.X86CPUArchitecture},
+				OpenshiftVersion: swag.String("4.12"),
+				URL:              swag.String("release_4.12.999-x86_64"),
+				Version:          swag.String("4.12.999"),
+			},
+		}
+
+		releaseImage, err := h.GetReleaseImage(ctx, "4.12.999", common.X86CPUArchitecture, pullSecret)
+		Expect(err).ShouldNot(HaveOccurred())
+		Expect(*releaseImage.OpenshiftVersion).Should(Equal("4.12"))
+		Expect(*releaseImage.Version).Should(Equal("4.12.999"))
+		Expect(*releaseImage.CPUArchitecture).Should(Equal(common.X86CPUArchitecture))
+	})
+
+	It("returns the multi-arch image if matching CPU architecture is not present", func() {
+		h.releaseImages = models.ReleaseImages{
+			&models.ReleaseImage{
+				CPUArchitecture:  swag.String(common.MultiCPUArchitecture),
+				CPUArchitectures: []string{common.X86CPUArchitecture, common.ARM64CPUArchitecture, common.PowerCPUArchitecture},
+				OpenshiftVersion: swag.String("4.12-multi"),
+				URL:              swag.String("release_4.12.999-multi"),
+				Version:          swag.String("4.12.999"),
+			},
+			&models.ReleaseImage{
+				CPUArchitecture:  swag.String(common.X86CPUArchitecture),
+				CPUArchitectures: []string{common.X86CPUArchitecture},
+				OpenshiftVersion: swag.String("4.13"),
+				URL:              swag.String("release_4.13.999-x86_64"),
+				Version:          swag.String("4.13.999"),
+			},
+		}
+
+		releaseImage, err := h.GetReleaseImage(ctx, "4.12.999", common.X86CPUArchitecture, pullSecret)
+		Expect(err).ShouldNot(HaveOccurred())
+		Expect(*releaseImage.OpenshiftVersion).Should(Equal("4.12-multi"))
+		Expect(*releaseImage.Version).Should(Equal("4.12.999"))
+		Expect(*releaseImage.CPUArchitecture).Should(Equal(common.MultiCPUArchitecture))
+	})
+
 	Context("with a kube client", func() {
 		var (
 			client client.Client