-
Notifications
You must be signed in to change notification settings - Fork 376
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.
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
Enable all Linux arches in cli-artifacts #153
Conversation
Hi @yselkowitz. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
In order for console-operator deployment to succeed, cli-artifacts needs to be available on all arches for downloads-openshift-console. However, in that case, /usr/bin/oc (inherited from cli) is a native binary, and we want to provide all primary Linux architectures to match those on mirror.openshift.com, regardless of cluster architecture. This depends on openshift/oc#153 to provide the new binaries in cli-artifacts.
/ok-to-test |
images/cli-artifacts/Dockerfile.rhel
Outdated
|
||
FROM registry.svc.ci.openshift.org/ocp/4.2:cli | ||
COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/darwin_amd64/oc /usr/share/openshift/mac/oc | ||
COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/windows_amd64/oc.exe /usr/share/openshift/windows/oc.exe | ||
COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/linux_amd64/oc /usr/share/openshift/linux-x86_64/oc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rename here? Can't we just stick with the GOARCH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure how to layout these directories so I went by https://mirror.openshift.com/pub/openshift-v4/clients/oc/4.2/ (where uname-style arches are used instead of GOARCH-style names). As they are new, they can be named as desired, I just need to keep the console-operator patch in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see us porting oc
from Go to another language in the near future, so I'd rather just:
COPY /go/src/github.com/openshift/oc/_output/bin/ /usr/share/openshift/
Folks who want a different naming scheme can translate; we don't have to translate for them here.
images/cli-artifacts/Dockerfile.rhel
Outdated
@@ -4,11 +4,15 @@ FROM registry.svc.ci.openshift.org/ocp/builder:golang-1.12 AS builder | |||
WORKDIR /go/src/github.com/openshift/oc | |||
COPY . . | |||
RUN yum install -y --setopt=skip_missing_names_on_install=False gpgme-devel libassuan-devel | |||
RUN make cross-build-darwin-amd64 cross-build-windows-amd64 --warn-undefined-variables | |||
RUN make cross-build --warn-undefined-variables | |||
|
|||
FROM registry.svc.ci.openshift.org/ocp/4.2:cli | |||
COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/darwin_amd64/oc /usr/share/openshift/mac/oc | |||
COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/windows_amd64/oc.exe /usr/share/openshift/windows/oc.exe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we want to include the arch, can we add hardlinks or symlinks or some such to these with explicit arches? We'd need to keep the old paths around (at least as links) until oc adm release extract
and the console downloads deployment had been ported to use the new paths. And personally, I'd be happier if we just stuck to GOOS and used darwin
instead of mac
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change any of the existing paths, as I wasn't sure where else they may be used. Can we leave that for a separate commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change any of the existing paths, as I wasn't sure where else they may be used. Can we leave that for a separate commit?
I think we should leave dropping the existing paths to future work. But openshift/console-operator#343 can be simplified if we add the consistent paths in this PR. Something like:
COPY /go/src/github.com/openshift/oc/_output/bin/ /usr/share/openshift/
RUN mkdir /usr/share/openshift/mac /usr/share/openshift/windows && ln /usr/share/openshift/darwin_amd64/oc /usr/share/openshift/mac/oc && ln /usr/share/openshift/windows_amd64/oc.exe /usr/share/openshift/windows/oc.exe
/test e2e-aws |
In order for console-operator deployment to succeed, cli-artifacts needs to be available on all arches for downloads-openshift-console. However, in that case, /usr/bin/oc (inherited from cli) is a native binary, and we want to provide all primary Linux architectures to match those on mirror.openshift.com, regardless of cluster architecture. This depends on openshift/oc#153 to provide the new binaries in cli-artifacts.
In order for console-operator deployment to succeed, cli-artifacts needs to be available on all arches for downloads-openshift-console. However, in that case, /usr/bin/oc (inherited from cli) is a native binary, and we want to provide all primary Linux architectures to match those on mirror.openshift.com. In order to do so, this provides cross-compiled Linux binaries for multiple architectures. Cross-compiling oc fails with gssapi enabled, therefore it is disabled in the cross builds.
In order for console-operator deployment to succeed, cli-artifacts needs to be available on all arches for downloads-openshift-console. However, in that case, /usr/bin/oc (inherited from cli) is a native binary, and we want to provide all primary Linux architectures to match those on mirror.openshift.com, regardless of cluster architecture. This depends on openshift/oc#153 to provide the new binaries in cli-artifacts.
/test e2e-cmd |
1 similar comment
/test e2e-cmd |
In order for console-operator deployment to succeed, cli-artifacts needs to be available on all arches for downloads-openshift-console. However, in that case, /usr/bin/oc (inherited from cli) is a native binary, and we want to provide all primary Linux architectures to match those on mirror.openshift.com, regardless of cluster architecture. This depends on openshift/oc#153 to provide the new binaries in cli-artifacts.
@wking reworked the patch per your comments, anything else needed here? |
/approve I'll let @wking reply with the LGTM once he's happy with the changes. |
/lgtm (but I'm not an approver). Dunno how robust our CI coverage is here, but we can always adjust later if we're somehow breaking backwards compat here (although I'd expect the console downloads pod to crashloop if we did). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: crawford, JAORMX, soltysh, wking, yselkowitz 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:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
In order for console-operator deployment to succeed, cli-artifacts needs to be available on all arches for downloads-openshift-console. However, in that case, /usr/bin/oc (inherited from cli) is a native binary, and we want to provide all primary Linux architectures to match those on mirror.openshift.com, regardless of cluster architecture. This depends on openshift/oc#153 to provide the new binaries in cli-artifacts.
In order for console-operator deployment to succeed, cli-artifacts needs to be available on all arches for downloads-openshift-console. However, in that case, /usr/bin/oc (inherited from cli) is a native binary, and we want to provide all primary Linux architectures to match those on mirror.openshift.com, regardless of cluster architecture. This depends on openshift/oc#153 to provide the new binaries in cli-artifacts. (cherry picked from commit 95d5e4b)
openshift#153 introduced a regression because `oc adm release extract` does not handle the hard links well.
openshift#153 introduced a regression because `oc adm release extract` does not handle the hard links well.
Avoid [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc by avoiding the hardlink paths and teaching extract about the new canonical paths from e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153). [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642
Avoid [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc by avoiding the hardlink paths and teaching extract about the new canonical paths from e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153). [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642
Also teach oc to extract from standard locations e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153) to avoid choking on hardlinks [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc Teaching extractTarget about architectures avoids conflicts with multiple architectures trying to use the no-longer-specific-enough oc-linux in targetsByName [2]: $ ./oc adm release extract --command=oc --command-os='*' registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: unable to iterate over layer sha256:13caf755813923e69e25ff1a28cc65766d6dcaa12676e5e274db9ec828e55d71 from registry.svc.ci.openshift.org/ocp/4.3-2019-11-20-121416@sha256:6497d5cb7102903baf98bbc0e07144f04827a21dcd800ff61360d25228a2d2ce: unable to find target with mapping name openshift-client-linux-4.3.0-0.ci-2019-11-20-121416.tar.gz Adding --command-arch on top of that allows us to set currentArch to avoid extracting 'oc' for multiple architectures all into the same file (making it unlikely that you get the architecture you want ;). [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642 [2]: openshift#172 (comment)
Also teach oc to extract from standard locations e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153) to avoid choking on hardlinks [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc Teaching extractTarget about architectures avoids conflicts with multiple architectures trying to use the no-longer-specific-enough oc-linux in targetsByName [2]: $ ./oc adm release extract --command=oc --command-os='*' registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: unable to iterate over layer sha256:13caf755813923e69e25ff1a28cc65766d6dcaa12676e5e274db9ec828e55d71 from registry.svc.ci.openshift.org/ocp/4.3-2019-11-20-121416@sha256:6497d5cb7102903baf98bbc0e07144f04827a21dcd800ff61360d25228a2d2ce: unable to find target with mapping name openshift-client-linux-4.3.0-0.ci-2019-11-20-121416.tar.gz Adding --command-arch on top of that allows us to set currentArch to avoid extracting 'oc' for multiple architectures all into the same file (making it unlikely that you get the architecture you want ;). [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642 [2]: openshift#172 (comment)
Also teach oc to extract from standard locations e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153) to avoid choking on hardlinks [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc Teaching extractTarget about architectures avoids conflicts with multiple architectures trying to use the no-longer-specific-enough oc-linux in targetsByName [2]: $ ./oc adm release extract --command=oc --command-os='*' registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: unable to iterate over layer sha256:13caf755813923e69e25ff1a28cc65766d6dcaa12676e5e274db9ec828e55d71 from registry.svc.ci.openshift.org/ocp/4.3-2019-11-20-121416@sha256:6497d5cb7102903baf98bbc0e07144f04827a21dcd800ff61360d25228a2d2ce: unable to find target with mapping name openshift-client-linux-4.3.0-0.ci-2019-11-20-121416.tar.gz Adding --command-arch on top of that allows us to set currentArch to avoid extracting 'oc' for multiple architectures all into the same file (making it unlikely that you get the architecture you want ;). Updated the completions with: $ make build $ hack/update-generated-completions.sh [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642 [2]: openshift#172 (comment)
Also teach oc to extract from standard locations e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153) to avoid choking on hardlinks [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc Teaching extractTarget about architectures avoids conflicts with multiple architectures trying to use the no-longer-specific-enough oc-linux in targetsByName [2]: $ ./oc adm release extract --command=oc --command-os='*' registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: unable to iterate over layer sha256:13caf755813923e69e25ff1a28cc65766d6dcaa12676e5e274db9ec828e55d71 from registry.svc.ci.openshift.org/ocp/4.3-2019-11-20-121416@sha256:6497d5cb7102903baf98bbc0e07144f04827a21dcd800ff61360d25228a2d2ce: unable to find target with mapping name openshift-client-linux-4.3.0-0.ci-2019-11-20-121416.tar.gz Adding --command-arch on top of that allows us to set currentArch to avoid extracting 'oc' for multiple architectures all into the same file (making it unlikely that you get the architecture you want ;). Updated the completions with: $ make build $ hack/update-generated-completions.sh [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642 [2]: openshift#172 (comment)
Lean on the path consistency from openshift/oc@e949088d (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift/oc#153) to make it easier to scale as we add/remove arch/OS support. With this commit, the downloads deployment is less opinionated about what is in the cli-artifacts bundle. It looks for all possible arch/OS combinations, serves what it finds, and 404s requests for binaries it could not find. I've also renamed mac -> darwin in the canonical URIs, so there's no mental overhead for translating from the conventional GOOS/GOARCH [1]. I've added symlinks in the pod to support external folks who had hard-coded the old URIs, but we should be able to drop those symlinks after a reasonable deprecation period. I have not overhauled the server to 301 requests to the deprecated URI, and we probably want that before dropping the symlinks. [1]: https://golang.org/doc/install/source#environment
Deprecating the previous --command-os to get consistency with other image handling. --filter-by-os originally landed in openshift/origin@e04b16527b (cli: Mirror images across registries or to S3, 2017-06-04, openshift/origin#14471). The wrapping in: o.FilterOptions.FilterByOS = fmt.Sprintf("^%s/", o.CommandOperatingSystem) guards against the unlikely case that a given --command-os value is a valid prefix for a longer OS, or matches an arch or varient or some such. Also teach oc to extract from standard locations e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153) to avoid choking on hardlinks [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc Teaching extractTarget about architectures avoids conflicts with multiple architectures trying to use the no-longer-specific-enough oc-linux in targetsByName [2]: $ ./oc adm release extract --command=oc --command-os='*' registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: unable to iterate over layer sha256:13caf755813923e69e25ff1a28cc65766d6dcaa12676e5e274db9ec828e55d71 from registry.svc.ci.openshift.org/ocp/4.3-2019-11-20-121416@sha256:6497d5cb7102903baf98bbc0e07144f04827a21dcd800ff61360d25228a2d2ce: unable to find target with mapping name openshift-client-linux-4.3.0-0.ci-2019-11-20-121416.tar.gz Adding --command-arch on top of that allows us to set currentArch to avoid extracting 'oc' for multiple architectures all into the same file (making it unlikely that you get the architecture you want ;). Updated the completions with: $ make build $ hack/update-generated-completions.sh [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642 [2]: openshift#172 (comment)
Deprecating the previous --command-os to get consistency with other image handling. --filter-by-os originally landed in openshift/origin@e04b16527b (cli: Mirror images across registries or to S3, 2017-06-04, openshift/origin#14471). The wrapping in: o.FilterOptions.FilterByOS = fmt.Sprintf("^%s/", o.CommandOperatingSystem) guards against the unlikely case that a given --command-os value is a valid prefix for a longer OS, or matches an arch or varient or some such. Also teach oc to extract from standard locations e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153) to avoid choking on hardlinks [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc Teaching extractTarget about architectures avoids conflicts with multiple architectures trying to use the no-longer-specific-enough oc-linux in targetsByName [2]: $ ./oc adm release extract --command=oc --command-os='*' registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: unable to iterate over layer sha256:13caf755813923e69e25ff1a28cc65766d6dcaa12676e5e274db9ec828e55d71 from registry.svc.ci.openshift.org/ocp/4.3-2019-11-20-121416@sha256:6497d5cb7102903baf98bbc0e07144f04827a21dcd800ff61360d25228a2d2ce: unable to find target with mapping name openshift-client-linux-4.3.0-0.ci-2019-11-20-121416.tar.gz Adding --command-arch on top of that allows us to set currentArch to avoid extracting 'oc' for multiple architectures all into the same file (making it unlikely that you get the architecture you want ;). Updated the completions with: $ make build $ hack/update-generated-completions.sh [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642 [2]: openshift#172 (comment)
These are where the binaries lived before e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153), and since the extraction chokes on hardlinks [1], we need to be looking at those locations to extract from 4.2.z and older releases. Also shuffles the Dockerfile around to move the darwin and Windows builds to their old locations and symlink them from the standard locations. That way older 4.2.z releases will find regular files at the locations they expect, but that we'll be able to serve files from the standardized pattern in the downloads deployment [2]. I'm also removing the cli-artifacts-built linux/amd64 binary in favor of the build we inherited from the parent cli image. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642 [2]: openshift/console-operator#354
In order for console-operator deployment to succeed, cli-artifacts needs to be available on all arches for downloads-openshift-console. However, in that case, /usr/bin/oc (inherited from cli) is a native binary, and we want to provide all primary Linux architectures to match those on mirror.openshift.com.
In order to do so, this provides cross-compiled Linux binaries for multiple architectures. Cross-compiling oc fails with gssapi enabled, therefore it is disabled in the cross builds.
Because this affects cluster deployment, we are looking to get this into 4.2 and later branches (we can provide separate PRs for stable branches as needed).