Enable multi-arch support in downloads #343

yselkowitz · 2019-11-08T02:25:47Z

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.

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).

openshift-ci-robot · 2019-11-08T02:26:02Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

yselkowitz · 2019-11-08T02:26:17Z

/hold
until the cli-artifacts change is merged:
openshift/oc#153

benjaminapetersen · 2019-11-08T16:51:44Z

manifests/07-downloads-deployment.yaml

              os.mkdir(arch)
              for operating_system in ['linux', 'mac', 'windows']:
                  os.mkdir(os.path.join(arch, operating_system))
+          for arch in ['arm64', 'ppc64le', 's390x']:


To make this usable you would also need to update the the generated downloads links:
https://github.com/openshift/console-operator/blob/master/pkg/console/controllers/clidownloads/controller.go#L155
This will update the links in the console itself.

This is when it would be ideal to have 1 place for this data.

I'm tempted to suggest we would add this annotation to the deployment:

annotations: release.openshift.io/create-only: "true"

And then cut the spec and add from within the operator itself. This would let the operator generate both the cliDownloadsLinks CR & this script from the same source struct.

@spadgett @wking not sure if there are any objections to further work in this deployment. Personally I'm not super jazzed about adding logic for string manipulation, since this script is just a bit embedded string, but I don't really love the manual sync of multiple resources either.

If the script is static, I'd keep it as a manifest. I'd only generate the downloads deployment in the operator if we need some logic, for instance, if we decide to make the node selector customizable.

cc @bparees

Its probably time to add an e2e test to this and validate that an http request to each of these endpoints is valid.

For docs purposes, the old link to the mirror is gone:
https://github.com/openshift/console/pull/2951/files#diff-f2e8b14c70b3c8bf99b5eed3b019db60L46
We no longer hard-code this:
https://github.com/openshift/console/pull/2951/files#diff-20ab917f3e7dc4814ce688ea3e52044eL67
Since we now do what I referenced above with the CLIDownloadsController.

Mac and Windows still only cover one architecture, but that treats them as if they might cover all of them with an error if it (not surprisingly) does not. That seems like a lot of overhead for a result for which we already know the answer.

Mac and Windows still only cover one architecture, but that treats them as if they might cover all of them with an error if it (not surprisingly) does not.

A warning, not an error.

That seems like a lot of overhead for a result for which we already know the answer.

We don't know the answer, it depends on which cli-artifacts gets used for this deployment. Maybe later they do grow additional architectures, or maybe we drop an arch, or grow a new OS, or whatever. I don't think this deployment should be opinionated. It should just look in the places that might be there, serve what it has, and 404 requests for content it doesn't have.

Is it really possible to have a cli-artifacts out of sync with console-operator? if not, then we do know which architectures are available, do we not?

Is it really possible to have a cli-artifacts out of sync with console-operator?

Sure. For example, we could decide we want to publish a windows/arm64 oc, add it to cli-artifacts, and my standard-path approach (which I just pushed up as #354), would immediately serve it (although you'd still need a console bump to get an entry in PlatformBasedOCConsoleCLIDownloads). And if we wanted to avoid a PlatformBasedOCConsoleCLIDownloads bump, we could serve the choices dynamically from the downloads pod. Hmm... :)

It would definitely be nice to have a single source of truth and not have to sync these two items. That said, most who use the console will probably want one of the "mainstream" versions of oc, so clusttering up the UI with all the things may not be that valuable.

wking · 2019-11-09T04:26:07Z

/ok-to-test

Pretty sure downloads will die without a full cli-artifacts to work from, but let's see...

benjaminapetersen · 2019-11-11T21:26:27Z

errors in test:

# github.com/openshift/console-operator/pkg/console/controllers/clidownloads [github.com/openshift/console-operator/pkg/console/controllers/clidownloads.test]
pkg/console/controllers/clidownloads/controller_test.go:107:60: too many arguments in call to PlatformBasedOCConsoleCLIDownloads
have (string, string, string)
want (string, string)

Means you will have to fix func TestPlatformBasedOCConsoleCLIDownloads(t *testing.T) { ....}.
which exists in pkg/console/controllers/clidownloads/controller_test.go.

benjaminapetersen · 2019-11-12T15:55:25Z

Opened #344 to add tests for our oc downloads. I'd like to see some test coverage merge first, then this PR can update.

benjaminapetersen · 2019-11-13T18:52:20Z

/retest

timed out waiting for the condition
lots of timeouts, likely a flake.

yselkowitz · 2019-11-13T19:29:31Z

Repushed with corresponding changes for the unit test.

yselkowitz · 2019-11-13T23:31:52Z

/retest

yselkowitz · 2019-11-14T18:06:26Z

/retest

benjaminapetersen · 2019-11-18T15:11:23Z

E2E merged:
#344
You can now update this and we should be good to move forward

pkg/console/controllers/clidownloads/controller.go

benjaminapetersen · 2019-11-18T15:23:23Z

/retest e2e-aws-operator

Expected e2e func to be broken.

yselkowitz · 2019-11-18T17:24:08Z

/test e2e-gcp

yselkowitz · 2019-11-19T04:24:41Z

/hold cancel
openshift/oc#153 has been merged

yselkowitz · 2019-11-19T04:24:52Z

/test e2e-gcp

wking · 2019-11-19T05:24:12Z

pkg/console/controllers/clidownloads/controller.go

+		{"Linux for Power LE", "ppc64le/linux", "oc.tar"},
+		{"Linux for Z", "s390x/linux", "oc.tar"},
+		{"Mac", "amd64/mac", "oc.zip"},
+		{"Windows", "amd64/windows", "oc.zip"},


I'd prefer if these were "for amd64" or "for x86-64" or whatever Red Hat's preferred name for the architecture is instead of leaving it implicit. Incidentally, Wikipedia uses x86-64, not X86-64, and the RHEL 8 download page has x86_64, although Linux says the underscore form is a later thing.

By "implicit" are you referring to "Mac" and "Windows"? My patch does not change the name of those, and doing so might be more appropriate for a separate PR.

Repushed with (hopefully) more "offical" names for the Linux architectures (not that there is ever any consistency with these names).

By "implicit" are you referring to "Mac" and "Windows"?

Yup.

My patch does not change the name of those, and doing so might be more appropriate for a separate PR.

I'm not a maintainer here, but "add qualifying arch names to download links for all existing downloads" seems in-scope for a PR that adds multi-arch support ;).

I changed "Windows" to "Windows 64-bit" in a new commit. I see no use to adding anything to "Mac", as only 64-bit Intel is currently supported.

I don't think "64-bit" is sufficiently qualified (seems like it would also include arm64, etc.). I also think we need to be explicit about Mac, because folks could be running macOS/Darwin on other arches even if we don't support running oc on those arches. I've filed #353 to address both points, PTAL.

yselkowitz · 2019-11-19T05:48:46Z

/test e2e-gcp

yselkowitz · 2019-11-19T12:10:53Z

/test e2e-gcp

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.

yselkowitz · 2019-11-19T21:18:05Z

/retest

benjaminapetersen · 2019-11-20T03:10:53Z

/lgtm

I think we are good to go with this now.

openshift-ci-robot · 2019-11-20T03:11:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, yselkowitz

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [benjaminapetersen]

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

openshift-bot · 2019-11-20T03:36:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-11-20T03:49:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-11-20T04:02:16Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-11-20T04:54:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

…Mac/Windows Folks could presumably be running these OSes on other architectures. For example, the GOOS/GOARCH docs [1] provide for darwin on 386, amd64, arm, and arm64 and windows on 386 and amd64 (although Windows runs on ARM64 [2] and possibly other architectures as well). I'm sticking with x86_64 for consistency with the Linux entry from 865d84d (Enable multi-arch Linux links in clidownloads, 2019-11-08, openshift#343), although Linus says the x86-64 form predates the underscore form [3]. [1]: https://golang.org/doc/install/source#environment [2]: https://docs.microsoft.com/en-us/windows/arm/ [3]: https://lkml.org/lkml/2004/2/21/110

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 8, 2019

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 8, 2019

openshift-ci-robot requested review from jhadvig and spadgett November 8, 2019 02:26

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2019

yselkowitz mentioned this pull request Nov 8, 2019

Build ose-cli-artifacts for non-x86 platforms as well openshift-eng/ocp-build-data#201

Closed

benjaminapetersen reviewed Nov 8, 2019

View reviewed changes

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 8, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 9, 2019

wking mentioned this pull request Nov 11, 2019

Enable all Linux arches in cli-artifacts openshift/oc#153

Merged

yselkowitz force-pushed the master branch from e83b24c to 163f037 Compare November 13, 2019 19:28

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 13, 2019

yselkowitz force-pushed the master branch from 163f037 to c1da6c0 Compare November 13, 2019 20:42

benjaminapetersen reviewed Nov 18, 2019

View reviewed changes

pkg/console/controllers/clidownloads/controller.go Show resolved Hide resolved

yselkowitz force-pushed the master branch from c1da6c0 to ca9841b Compare November 18, 2019 15:30

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2019

wking reviewed Nov 19, 2019

View reviewed changes

yselkowitz force-pushed the master branch from ca9841b to 922144c Compare November 19, 2019 17:43

yselkowitz added 2 commits November 19, 2019 21:17

Enable multi-arch Linux links in clidownloads

865d84d

Clarify Windows platform in clidownloads

ed021f4

yselkowitz force-pushed the master branch from 922144c to ed021f4 Compare November 20, 2019 02:20

openshift-ci-robot assigned benjaminapetersen Nov 20, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2019

openshift-merge-robot merged commit 04c4013 into openshift:master Nov 20, 2019

yselkowitz mentioned this pull request Nov 20, 2019

Bug 1773808: Enable multi-arch support in downloads [4.2] #352

Merged

wking mentioned this pull request Nov 20, 2019

pkg/console/controllers/clidownloads/controller: Arch qualifiers for Mac/Windows #353

Merged

wking mentioned this pull request Nov 20, 2019

manifests/07-downloads-deployment: Serve from standardized paths #354

Closed

Enable multi-arch support in downloads #343

Enable multi-arch support in downloads #343

Uh oh!

Conversation

yselkowitz commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 8, 2019

Uh oh!

yselkowitz commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Nov 9, 2019

Uh oh!

benjaminapetersen commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminapetersen commented Nov 12, 2019

Uh oh!

benjaminapetersen commented Nov 13, 2019

Uh oh!

yselkowitz commented Nov 13, 2019

Uh oh!

yselkowitz commented Nov 13, 2019

Uh oh!

yselkowitz commented Nov 14, 2019

Uh oh!

benjaminapetersen commented Nov 18, 2019

Uh oh!

Uh oh!

benjaminapetersen commented Nov 18, 2019

Uh oh!

yselkowitz commented Nov 18, 2019

Uh oh!

yselkowitz commented Nov 19, 2019

Uh oh!

yselkowitz commented Nov 19, 2019

Uh oh!

wking Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yselkowitz commented Nov 19, 2019

Uh oh!

yselkowitz commented Nov 19, 2019

Uh oh!

yselkowitz commented Nov 19, 2019

Uh oh!

benjaminapetersen commented Nov 20, 2019

Uh oh!

yselkowitz commented Nov 8, 2019 •

edited

Loading

yselkowitz commented Nov 8, 2019 •

edited

Loading

benjaminapetersen commented Nov 11, 2019 •

edited

Loading

wking Nov 19, 2019 •

edited

Loading