: MULTIARCH-4568: Inject ReleaseArch into openshift-install #1792

jeffdyoung · 2024-05-31T13:31:24Z

In parallel with: openshift/installer#8515

This is necessary so the installer knows if it is using a single arch payload vs a multi arch payload.
ATM customers can't recognize the difference without having to query the container registry.
This is also the first step to enable deploying clusters with ARM control planes and x86 compute nodes (and vice versa)
https://issues.redhat.com/browse/MULTIARCH-4567

pkg/cli/admin/release/extract_tools.go

ardaguclu · 2024-06-03T06:15:41Z

pkg/cli/admin/release/extract_tools.go

+			Readme:                    readmeInstallUnix,
+			InjectReleaseImage:        true,
+			InjectReleaseVersion:      true,
+			InjectReleaseArchitecture: true,


There are various usage patterns of this command, have you had a chance to check all of them;

oc adm release info --tools

oc adm release --command=installer --command=darwing/arm64
...

yes, they will print a warning if the openshift-install doesn't have the right marker, but it will exit 0.

pkg/cli/admin/release/extract_tools.go

ardaguclu · 2024-06-03T06:20:43Z

pkg/cli/admin/release/extract_tools.go

@@ -1041,6 +1056,8 @@ const (
 	releaseImageMarker = "!_RELEASE_IMAGE_LOCATION_\x00XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\x00"
 	// releaseVersionMarker is the placeholder within a binary for the release image version name string.
 	releaseVersionMarker = "!_RELEASE_VERSION_LOCATION_\x00XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\x00"
+	// releaseArchitectureMarker is the placeholder within a binary for the list of architectures the release image supports.
+	releaseArchitectureMarker = "!_RELEASE_ARCHITECTURE_LOCATION_\x00XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\x00"


We are modifying binaries that are already built and that inevitably causes checksum value changes which is not good with respect to sanity checks. I'm against this change as it additionally modifies installer binary with more data and I believe that this is not the correct way to pass information.

But as far as I understand you can't find any alternative solution and I won't block this PR. I can only suggest that please ensure that the checksum values of the modified binary must be same with the data in the sha256sum file.

ardaguclu · 2024-06-03T06:24:55Z

I'd like to note that please be aware that with this change, all the new minor version extraction mechanisms for old major version will be affected. For instance, now new 4.14.z installer binary will be embedded with this information and if this funtionality relies on something in newer release payloads, that causes issues.

jeffdyoung · 2024-06-03T13:28:44Z

I'd like to note that please be aware that with this change, all the new minor version extraction mechanisms for old major version will be affected. For instance, now new 4.14.z installer binary will be embedded with this information and if this funtionality relies on something in newer release payloads, that causes issues.

@ardaguclu For binaries that don't have the _RELEASE_ARCHITECTURE_LOCATION_ marker, oc will throw a warning and not inject this information:

$ ./oc adm release extract --command openshift-install quay.io/openshift-release-dev/ocp-release-nightly@sha256:13b7b2a6b1b93ae93e1dbcbe4a319c31ad2b13cfb59618e4a7ec057cd1dae777 --filter-by-os linux/amd64 --command-os=linux/amd64  
warning: Unable to make all expected replacements in openshift-install.  Remaining: release architecture

Maybe there is a way to inspect the binary for this marker before we try to inject the change tho? 🤔

ardaguclu · 2024-06-03T13:36:23Z

I'd like to note that please be aware that with this change, all the new minor version extraction mechanisms for old major version will be affected. For instance, now new 4.14.z installer binary will be embedded with this information and if this funtionality relies on something in newer release payloads, that causes issues.

@ardaguclu For binaries that don't have the _RELEASE_ARCHITECTURE_LOCATION_ marker, oc will throw a warning and not inject this information:
$ ./oc adm release extract --command openshift-install quay.io/openshift-release-dev/ocp-release-nightly@sha256:13b7b2a6b1b93ae93e1dbcbe4a319c31ad2b13cfb59618e4a7ec057cd1dae777 --filter-by-os linux/amd64 --command-os=linux/amd64  
warning: Unable to make all expected replacements in openshift-install.  Remaining: release architecture

That means current status of this PR is causing failures for extractions from older release payloads.

Edited: Assuming that there might be places that returns exit code non-zero, when this warning happens.

Maybe there is a way to inspect the binary for this marker before we try to inject the change tho? 🤔

That looks to me a risky operation comparing to the benefit we are trying to get.

jeffdyoung · 2024-06-03T13:40:07Z

I'd like to note that please be aware that with this change, all the new minor version extraction mechanisms for old major version will be affected. For instance, now new 4.14.z installer binary will be embedded with this information and if this funtionality relies on something in newer release payloads, that causes issues.

@ardaguclu For binaries that don't have the _RELEASE_ARCHITECTURE_LOCATION_ marker, oc will throw a warning and not inject this information:
$ ./oc adm release extract --command openshift-install quay.io/openshift-release-dev/ocp-release-nightly@sha256:13b7b2a6b1b93ae93e1dbcbe4a319c31ad2b13cfb59618e4a7ec057cd1dae777 --filter-by-os linux/amd64 --command-os=linux/amd64  
warning: Unable to make all expected replacements in openshift-install.  Remaining: release architecture
That means current status of this PR is causing failures for extractions from older release payloads.

Maybe there is a way to inspect the binary for this marker before we try to inject the change tho? 🤔

That looks to me a risky operation comparing to the benefit we are trying to get.

It throws a warning, but the binary is intact and operational.
Open to other alternatives to solve this.

ardaguclu · 2024-06-03T14:50:10Z

I'd like to note that please be aware that with this change, all the new minor version extraction mechanisms for old major version will be affected. For instance, now new 4.14.z installer binary will be embedded with this information and if this funtionality relies on something in newer release payloads, that causes issues.

@ardaguclu For binaries that don't have the _RELEASE_ARCHITECTURE_LOCATION_ marker, oc will throw a warning and not inject this information:
$ ./oc adm release extract --command openshift-install quay.io/openshift-release-dev/ocp-release-nightly@sha256:13b7b2a6b1b93ae93e1dbcbe4a319c31ad2b13cfb59618e4a7ec057cd1dae777 --filter-by-os linux/amd64 --command-os=linux/amd64  
warning: Unable to make all expected replacements in openshift-install.  Remaining: release architecture
That means current status of this PR is causing failures for extractions from older release payloads.

Maybe there is a way to inspect the binary for this marker before we try to inject the change tho? 🤔

That looks to me a risky operation comparing to the benefit we are trying to get.
It throws a warning, but the binary is intact and operational. Open to other alternatives to solve this.

Have you had a chance to check that when this warning is printed, exit code is still zero?. If that is the case, we can move forward.

jeffdyoung · 2024-06-03T15:56:28Z

Details

Might be hard to see in this output, but the return code is 0:

$ ./oc adm release extract --tools registry.ci.openshift.org/ocp-arm64/release-arm64:4.16.0-0.nightly-arm64-2024-06-03-071524 --command-os darwin/amd64 && echo $?
warning: Unable to make all expected replacements in openshift-install.  Remaining: release architecture0

The tar sha matches as well

$ cat sha256sum.txt 
88f249abc813c522d9fd27d0c2d299facf2e387ad32b267df99de7e3f36ec636  openshift-client-mac-4.16.0-0.nightly-arm64-2024-06-03-071524.tar.gz
52d08375bc062f976e22ffea8c082717e8d01d771abd9d8d4a1d5d8830f81d17  openshift-install-mac-4.16.0-0.nightly-arm64-2024-06-03-071524.tar.gz
dee6027130d79d3cb41aa5d52a3796ebdf33404a78ac47682c7486b740b8a0da  release.txt

$ sha256sum openshift-install-mac-4.16.0-0.nightly-arm64-2024-06-03-071524.tar.gz 
52d08375bc062f976e22ffea8c082717e8d01d771abd9d8d4a1d5d8830f81d17  openshift-install-mac-4.16.0-0.nightly-arm64-2024-06-03-071524.tar.gz

jeffdyoung · 2024-06-20T11:32:16Z

/retest

jeffdyoung · 2024-06-20T15:41:23Z

/retest

jeffdyoung · 2024-06-20T21:56:01Z

/retest

ardaguclu · 2024-06-21T07:06:20Z

I'd like to get some opinions about this patch from the installer as this modifies the installer binary. WDYT? @zaneb
/retest

zaneb · 2024-06-24T02:41:26Z

The installer team will be able to weigh in on openshift/installer#8515. Assuming that is accepted then we'd want this change as well.

ardaguclu · 2024-06-24T05:34:39Z

/lgtm
/retest

openshift-ci · 2024-06-24T05:36:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, jeffdyoung

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/cli/admin/release/OWNERS~~ [ardaguclu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

r4f4 · 2024-07-08T14:21:14Z

/retitle: MULTIARCH-4568: Inject ReleaseArch into openshift-install

openshift-ci-robot · 2024-07-08T14:21:32Z

@jeffdyoung: This pull request references MULTIARCH-4568 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 task to target the "4.17.0" version, but no target version was set.

In response to this:

In parallel with: openshift/installer#8515

This is necessary so the installer knows if it is using a single arch payload vs a multi arch payload.
ATM customers can't recognize the difference without having to query the container registry.
This is also the first step to enable deploying clusters with ARM control planes and x86 compute nodes (and vice versa)
https://issues.redhat.com/browse/MULTIARCH-4567

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-07-08T18:31:30Z

/retest-required

Remaining retests: 0 against base HEAD 639f714 and 2 for PR HEAD e77658e in total

openshift-ci · 2024-07-08T21:32:47Z

@jeffdyoung: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-bot · 2024-07-09T02:07:18Z

[ART PR BUILD NOTIFIER]

This PR has been included in build openshift-enterprise-cli-container-v4.17.0-202407090112.p0.ga2e5b09.assembly.stream.el9 for distgit openshift-enterprise-cli.
All builds following this will include this PR.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2024

openshift-ci bot requested review from deads2k and soltysh May 31, 2024 13:36

jeffdyoung mentioned this pull request May 31, 2024

MULTIARCH-4568: Print release image arch and default arch when the payload is multi openshift/installer#8515

Merged

ardaguclu reviewed Jun 3, 2024

View reviewed changes

jeffdyoung force-pushed the MULTIARCH-4568 branch from a872877 to 77c691c Compare June 18, 2024 20:09

jeffdyoung changed the title ~~[WIP] MULTIARCH-4568 Inject ReleaseArch into openshift-install~~ MULTIARCH-4568 Inject ReleaseArch into openshift-install Jun 18, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2024

Inject ReleaseArch into openshift-install

e77658e

jeffdyoung force-pushed the MULTIARCH-4568 branch from 86a2ee1 to e77658e Compare June 20, 2024 11:35

openshift-ci bot assigned ardaguclu Jun 24, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2024

openshift-ci bot changed the title ~~MULTIARCH-4568 Inject ReleaseArch into openshift-install~~ : MULTIARCH-4568: Inject ReleaseArch into openshift-install Jul 8, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 8, 2024

openshift-merge-bot bot merged commit a2e5b09 into openshift:master Jul 8, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

: MULTIARCH-4568: Inject ReleaseArch into openshift-install #1792

: MULTIARCH-4568: Inject ReleaseArch into openshift-install #1792

jeffdyoung commented May 31, 2024 •

edited

Loading

ardaguclu Jun 3, 2024

jeffdyoung Jun 18, 2024

ardaguclu Jun 3, 2024

ardaguclu commented Jun 3, 2024 •

edited

Loading

jeffdyoung commented Jun 3, 2024

ardaguclu commented Jun 3, 2024 •

edited

Loading

jeffdyoung commented Jun 3, 2024

ardaguclu commented Jun 3, 2024

jeffdyoung commented Jun 3, 2024

jeffdyoung commented Jun 20, 2024

jeffdyoung commented Jun 20, 2024

jeffdyoung commented Jun 20, 2024

ardaguclu commented Jun 21, 2024

zaneb commented Jun 24, 2024

ardaguclu commented Jun 24, 2024

openshift-ci bot commented Jun 24, 2024

r4f4 commented Jul 8, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jul 8, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jul 8, 2024

openshift-ci bot commented Jul 8, 2024

openshift-bot commented Jul 9, 2024

: MULTIARCH-4568: Inject ReleaseArch into openshift-install #1792

: MULTIARCH-4568: Inject ReleaseArch into openshift-install #1792

Conversation

jeffdyoung commented May 31, 2024 • edited Loading

ardaguclu Jun 3, 2024

Choose a reason for hiding this comment

jeffdyoung Jun 18, 2024

Choose a reason for hiding this comment

ardaguclu Jun 3, 2024

Choose a reason for hiding this comment

ardaguclu commented Jun 3, 2024 • edited Loading

jeffdyoung commented Jun 3, 2024

ardaguclu commented Jun 3, 2024 • edited Loading

jeffdyoung commented Jun 3, 2024

ardaguclu commented Jun 3, 2024

jeffdyoung commented Jun 3, 2024

jeffdyoung commented Jun 20, 2024

jeffdyoung commented Jun 20, 2024

jeffdyoung commented Jun 20, 2024

ardaguclu commented Jun 21, 2024

zaneb commented Jun 24, 2024

ardaguclu commented Jun 24, 2024

openshift-ci bot commented Jun 24, 2024

r4f4 commented Jul 8, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Jul 8, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Jul 8, 2024

openshift-ci bot commented Jul 8, 2024

openshift-bot commented Jul 9, 2024

jeffdyoung commented May 31, 2024 •

edited

Loading

ardaguclu commented Jun 3, 2024 •

edited

Loading

ardaguclu commented Jun 3, 2024 •

edited

Loading

r4f4 commented Jul 8, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jul 8, 2024 •

edited by openshift-ci bot

Loading