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 arm64 support #4870

Merged
merged 4 commits into from Jun 21, 2021
Merged

Add arm64 support #4870

merged 4 commits into from Jun 21, 2021

Conversation

yselkowitz
Copy link
Contributor

  • Add ARM architecture types
  • Fix libvirt cluster destroy on ARM
  • Add ARM support to AWS installation
  • cmd/openshift-install: Add architecture as an option for install-config
  • Make order of install-config questions more consistent
  • aws: treat a1 as backup to m6g
  • Add Makefile
  • Add rhcos-stream data for aarch64

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign smarterclayton after the PR has been reviewed.
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found 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

@Prashanth684
Copy link
Contributor

/retest

@staebler
Copy link
Contributor

This PR is covering a lot of changes, some of which are independent from each other. Please separate into discrete PRs that can be discussed on their own. Also, please add references to enhancements or Jira cards that provide more context for these changes.

@yselkowitz yselkowitz mentioned this pull request Apr 30, 2021
Merged
@yselkowitz
Copy link
Contributor Author

I moved the Makefile to #4891 where it belongs, and asked if the rhcos stream data could be added to #4861. I think that's about as much as we could sensibly split out.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2021
staebler
staebler reviewed May 11, 2021
View reviewed changes
pkg/destroy/libvirt/libvirt.go Outdated Show resolved Hide resolved
pkg/types/aws/defaults/platform.go Outdated Show resolved Hide resolved
pkg/asset/installconfig/installconfig.go Outdated Show resolved Hide resolved
data/data/rhcos-stream.json Outdated Show resolved Hide resolved
pkg/asset/installconfig/platform.go Outdated Show resolved Hide resolved
pkg/asset/machines/aws/instance_types.go Outdated Show resolved Hide resolved
pkg/types/machinepools.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2021
@cgwalters
Copy link
Member

Seems sane to me to split out the rhcos stream data commit FWIW.

@yselkowitz
Copy link
Contributor Author

Seems sane to me to split out the rhcos stream data commit FWIW.

Only if that would be expedited, because the rest of this won't work without it.

@staebler
Copy link
Contributor

Seems sane to me to split out the rhcos stream data commit FWIW.

Only if that would be expedited, because the rest of this won't work without it.

I don't care about separating that out. It is non-controversial. I just want to separate out the stuff that is controversial so that we can have discussions about those separately.

And note that nothing can be merged for another 4 weeks anyway.

@yselkowitz
Copy link
Contributor Author

Added an initial attempt to drop the survey question in favour of deriving based on payload architecture (assumed from the build host).

@yselkowitz yselkowitz force-pushed the arm64 branch 2 times, most recently from d037c87 to 7c17d00 Compare May 13, 2021 20:43
Prashanth684
Prashanth684 reviewed May 14, 2021
View reviewed changes
hack/build.sh Outdated
@@ -16,8 +16,9 @@ fi
MODE="${MODE:-release}"
GIT_COMMIT="${SOURCE_GIT_COMMIT:-$(git rev-parse --verify 'HEAD^{commit}')}"
GIT_TAG="${BUILD_VERSION:-$(git describe --always --abbrev=40 --dirty)}"
HOSTARCH="$(go env GOHOSTARCH)"
Copy link
Contributor

Choose a reason for hiding this comment

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

ok so iiuc - the architecture is now set to the host architecture if built manually and set to the architecture of the payload if it is extracted from the payload? so if i actually custom build the installer on an ARM laptop it would set the architecture in the install-config to arm64 - but which might not match the payload architecture necessarily.

Also note that someone could still override the payload using OPENSHIFT_RELEASE_IMAGE_OVERRIDE. so if these are called out it should make things clear for upstream as well as downstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this would be the correct architecture to select. Why should the architecture of the host where the installer is built effect the architecture of the cluster? I would expect the cluster architecture to default to amd64 to retain current behavior.

@yselkowitz
Copy link
Contributor Author

/retest

1 similar comment
@yselkowitz
Copy link
Contributor Author

/retest

@yselkowitz
Copy link
Contributor Author

Any idea why the openstack tests are failing so quickly?

@staebler
Copy link
Contributor

Any idea why the openstack tests are failing so quickly?

They were broken by #4999.

@yselkowitz
Copy link
Contributor Author

Any remaining issues to address here?

staebler
staebler reviewed Jun 18, 2021
View reviewed changes
pkg/asset/machines/aws/instance_types.go Outdated Show resolved Hide resolved
pkg/asset/machines/aws/instance_types.go Outdated Show resolved Hide resolved
pkg/types/aws/defaults/platform.go Outdated Show resolved Hide resolved
pkg/types/aws/defaults/platform.go Outdated Show resolved Hide resolved
pkg/version/version.go Outdated Show resolved Hide resolved
pkg/types/aws/validation/machinepool.go Outdated Show resolved Hide resolved
@yselkowitz
Copy link
Contributor Author

/retest

staebler
staebler reviewed Jun 18, 2021
View reviewed changes
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

Looks good save for a couple nits. Let's squash this down into one or a few commits.

pkg/asset/machines/aws/instance_types.go Outdated Show resolved Hide resolved
pkg/types/aws/validation/machinepool.go Outdated Show resolved Hide resolved
@Prashanth684
Copy link
Contributor

Looks good save for a couple nits. Let's squash this down into one or a few commits.

squashed the later commits relating to fixes in the comments.

staebler
staebler reviewed Jun 19, 2021
View reviewed changes
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

Please put the changes in the last commit into their appropriate earlier commits. I do not want a commit in the history that is "miscellaneous fixes".

pkg/types/aws/validation/machinepool.go Outdated Show resolved Hide resolved
Prashanth684 and others added 4 commits June 20, 2021 00:58
@Prashanth684 @yselkowitz
Add ARM architecture types
ec42fc1
@Prashanth684 @yselkowitz
Fix libvirt cluster destroy on ARM
1194d86
@Prashanth684 @yselkowitz
Add ARM support to AWS installation
0f8dcc3
@yselkowitz
Set default cluster architecture based on build host
729882a 
All installer binaries extracted from a payload, regardless of their
runtime OS or architecture, are built on the payload architecture.
Therefore, GOHOSTARCH can be used to assume the cluster architecture for
which its payload was built.  This is set through the Dockerfiles so that
manual builds of installer will continue to default to amd64.
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 20, 2021

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

Test name Commit Details Rerun command
ci/prow/e2e-openstack 729882a link /test e2e-openstack
ci/prow/e2e-openstack-upi 729882a link /test e2e-openstack-upi
ci/prow/e2e-metal-single-node-live-iso 729882a link /test e2e-metal-single-node-live-iso

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

staebler
staebler approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 21, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2021
@openshift-merge-robot openshift-merge-robot merged commit e1e4b2a into openshift:master Jun 21, 2021
@SaravanaStorageNetwork SaravanaStorageNetwork mentioned this pull request Jun 26, 2021
Closed
@yselkowitz yselkowitz mentioned this pull request Apr 7, 2022
Merged
@yselkowitz yselkowitz deleted the arm64 branch May 29, 2022 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Prashanth684 Prashanth684 Prashanth684 left review comments

@ausil ausil ausil left review comments

@staebler staebler staebler approved these changes

@cfergeau cfergeau Awaiting requested review from cfergeau

@jstuever jstuever Awaiting requested review from jstuever

Assignees

@staebler staebler

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@yselkowitz @openshift-ci-robot @Prashanth684 @staebler @cgwalters @LorbusChris @rhuss @ausil @openshift-merge-robot