search

openshift / oc

The OpenShift Command Line, part of OKD
https://www.openshift.org
Apache License 2.0
185 stars 373 forks source link

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

Closed jeffdyoung closed 31 minutes ago

jeffdyoung commented 1 month ago

In parallel with: https://github.com/openshift/installer/pull/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

ardaguclu commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 2 weeks ago

/retest

jeffdyoung commented 2 weeks ago

/retest

jeffdyoung commented 2 weeks ago

/retest

ardaguclu commented 2 weeks ago

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

zaneb commented 2 weeks ago

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 commented 2 weeks ago

/lgtm /retest

openshift-ci[bot] commented 2 weeks ago

[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](https://github.com/openshift/oc/blob/master/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 commented 7 hours ago

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

openshift-ci-robot commented 7 hours ago

@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](https://github.com/openshift/oc/pull/1792): >In parallel with: https://github.com/openshift/installer/pull/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](https://prow.ci.openshift.org/command-help?repo=openshift%2Foc). 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.
openshift-ci-robot commented 3 hours ago

/retest-required

Remaining retests: 0 against base HEAD 639f714b589b047a175b37fa0d28886a17e17ec3 and 2 for PR HEAD e77658e7eaf7dfe310e779ad9b6a74e581a3715c in total

openshift-ci[bot] commented 33 minutes ago

@jeffdyoung: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).