Skip to content
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.

Sign up for GitHub

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

Jump to bottom

*: add groundwork for muiti-arch bootimages #2885

Merged
merged 3 commits into from Jan 16, 2020
Merged

*: add groundwork for muiti-arch bootimages #2885

merged 3 commits into from Jan 16, 2020

Conversation

crawford
Copy link
Contributor

@crawford crawford commented Jan 7, 2020

This doesn't actually include support for s390x, since we don't have any s390x builds yet. The intent is that this will be backported (where we do have builds) and the master branch will be updated once builds are available.

/cc @cgwalters @jaypoulz @dgoodwin
/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 7, 2020
dgoodwin
dgoodwin reviewed Jan 7, 2020
View reviewed changes
Copy link
Contributor

@dgoodwin dgoodwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we'll need a few small changes in Hive to support. I'll add a Jira.

data/data/rhcos.json Outdated Show resolved Hide resolved
@dgoodwin
Copy link
Contributor

dgoodwin commented Jan 7, 2020

Will there be separate release images for each architecture, or one multiarch image?

cgwalters
cgwalters approved these changes Jan 7, 2020
View reviewed changes
hack/update-rhcos-bootimage.py Show resolved Hide resolved
@cgwalters
Copy link
Member

This is fine as is; just an optional comment; we have separate meta.json files generated by coreos-assembler for each build+arch, so I think it'd be cleaner to use separate files like rhcos-x86_64.json and rhcos-s390x.json etc. that would map more clearly to how the "coreos-assembler schema" works. But eh, again also fine as is!

ashcrow
ashcrow approved these changes Jan 7, 2020
View reviewed changes
Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this. Thank you @crawford.

@Prashanth684
Copy link
Contributor

Will there be separate release images for each architecture, or one multiarch image?

Will there be separate release images for each architecture, or one multiarch image?

separate release images for each arch.

@ashcrow
Copy link
Member

ashcrow commented Jan 7, 2020

https://github.com/openshift/installer/pull/2885/files#diff-9f95494109d2a540a4a321c151e20c63R733 may not produce the error as expected in the test ... possibly the strings of Unsupported versus Invalid and verbosity of error:

 --- FAIL: TestValidateInstallConfig (0.07s)
 --- FAIL: TestValidateInstallConfig/invalid_machine_pool_architecture (0.00s)
    installconfig_test.go:753: 
        	Error Trace:	installconfig_test.go:753
        	Error:      	Expect 
               "[controlPlane.architecture: Unsupported value: "foo": supported values: "amd64", compute[0].architecture: Invalid value: "bar": heteregeneous multi-arch is not supported; compute pool architecture must match control plane, compute[0].architecture: Unsupported value: "bar": supported values: "amd64"]" 
        to match
               "[controlPlane.architecture: Unsupported value: "foo": supported values: "amd64", compute[0].architecture: Unsupported value: "bar": supported values: "amd64"]"
        	Test:       	TestValidateInstallConfig/invalid_machine_pool_architecture

@crawford
Copy link
Contributor Author

@ashcrow Thanks, that extra test sneaked its way in from my branch that enabled s390x. I'll reintroduce that later :)

wking
wking reviewed Jan 14, 2020
View reviewed changes
pkg/types/machinepools.go Outdated Show resolved Hide resolved
wking
wking reviewed Jan 14, 2020
View reviewed changes
pkg/types/machinepools.go Outdated Show resolved Hide resolved
@wking
Copy link
Member

wking commented Jan 14, 2020

nit: needs a bump to the docs to describe the new property.

I'm also personally in favor of Colin's file per arch approach, as long as the RHCOS pipeline is building separate metadata files for each architecture. Or bump hack/update-rhcos-bootimage.py to take ARCH ... so you can say:

hack/update-rhcos-bootimage.py 44.81.201912131839.0 amd64 s390x ...

or whatever and have it automatically pull in all the metadata for each arch and assemble the single file.

sdodson
sdodson approved these changes Jan 14, 2020
View reviewed changes
Copy link
Member

@sdodson sdodson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I know there are people referencing rhcos.json directly my inclination would be to preserve compatibility by making amd64 the default and only defining new architectures. However this was never a documented API as far as I know so I'm fine with ripping the bandaid off as long as we make sure to message this change to internal developers. This PR is sufficient for messaging externally.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2020
@crawford
Copy link
Contributor Author

/hold

I'm going to rework this so we don't break these consumers. As much as I like ripping off band-aids, there is a bit too much flying around right now.

@crawford
hack: improve portability of bootimage script
8428471 
Not everyone has python3 installed into /usr/bin/.
@crawford
Copy link
Contributor Author

/hold cancel

Okay, this now preserves the original rhcos.json. I'll follow up with the direct consumers and tell them to stop. After that, we can remove the file.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 14, 2020
@crawford
hack: update format of rhcos build metadata
b1726d2 
This splits the RHCOS build metadata into architecture-specific files.
This will allow the metadata to contain information about bootimages of
multiple architectures. In order to preserve backward compatibility
(there are a few users, including certain CI jobs, that pull rhcos.json
from GitHub directly), I've opted to use separate files for each
architecture. Normally, we could have just symlinked the legacy metadata
file, but when hosted on raw.githubcontent.com, the symlinks aren't
followed.

When updating the RHCOS bootimages, this script will need to be run once
for each architecture that is being updated.
@cgwalters
Copy link
Member

/approve
lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, cgwalters, sdodson

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sdodson
Copy link
Member

sdodson commented Jan 15, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020
@openshift-bot
Copy link
Contributor

/retest

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

@crawford
*: add support for multi-arch rhcos image lookup
2583ba4 
This adds an architecture parameter to the RHCOS image lookup process
and a corresponding field to MachinePool. This is a backward-compatible
change, defaulting the architecture to AMD64 if none has been specified.
This also enforces that the control plane and compute nodes share an
architecture, since we don't support heterogeneous clusters today.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020
@wking
Copy link
Member

wking commented Jan 15, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020
@openshift-bot
Copy link
Contributor

/retest

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

9 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@crawford
Copy link
Contributor Author

/test shellcheck
/test yaml-lint

@openshift-merge-robot openshift-merge-robot merged commit 218972a into openshift:master Jan 16, 2020
@crawford crawford deleted the multiarch-bootimages branch January 16, 2020 01:26
@openshift-ci-robot
Copy link
Contributor

@crawford: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-libvirt 2583ba4 link /test e2e-libvirt
ci/prow/e2e-aws-fips 2583ba4 link /test e2e-aws-fips
ci/prow/e2e-openstack 2583ba4 link /test e2e-openstack
ci/prow/e2e-ovirt 2583ba4 link /test e2e-ovirt
ci/prow/e2e-aws-scaleup-rhel7 2583ba4 link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@crawford crawford mentioned this pull request Jan 16, 2020
Merged
@Prashanth684 Prashanth684 mentioned this pull request Mar 4, 2020
Merged
@wking wking mentioned this pull request May 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgoodwin dgoodwin dgoodwin left review comments

@wking wking wking left review comments

@ashcrow ashcrow ashcrow approved these changes

@sdodson sdodson sdodson approved these changes

@cgwalters cgwalters cgwalters approved these changes

@jaypoulz jaypoulz Awaiting requested review from jaypoulz

Assignees

@sdodson sdodson

@wking wking

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@crawford @dgoodwin @cgwalters @Prashanth684 @ashcrow @wking @openshift-ci-robot @sdodson @openshift-bot @openshift-merge-robot