Enable arm64 builds for Linux #4162

praveenkumar · 2024-05-14T07:27:12Z

No description provided.

openshift-ci · 2024-05-14T07:27:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from praveenkumar. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

praveenkumar · 2024-05-14T07:27:29Z

/hold

packaging/rpm/crc.spec.in

cfergeau · 2024-05-27T09:34:01Z

packaging/rpm/crc.spec.in

 install -m 0755 -vp %{gobuilddir}/src/%{goipath}/out/linux-amd64/crc %{buildroot}%{_bindir}/
+%endif
+%ifarch aarch64
+install -m 0755 -vp %{gobuilddir}/src/%{goipath}/out/linux-arm64/crc %{buildroot}%{_bindir}/


Could this use go env GOARCH instead of hardcoding these ifarch?

didn't find how to run that in the install section? https://community.nethserver.org/t/cross-compiling-multi-arch-rpms/18493 is also suggest same what we are using.

Do you think https://github.com/kovyrin/rpmbuild/blob/master/golang/SPECS/golang.spec#L27-L38 is better what we are having now?

Do you think https://github.com/kovyrin/rpmbuild/blob/master/golang/SPECS/golang.spec#L27-L38 is better what we are having now?

Yes I'd prefer something like this, it's a lot easier to see what is arch-specific, and what is common.

gbraad · 2024-05-29T05:40:21Z

Were other related issues addressed, like arm versions of the helper and driver?

praveenkumar · 2024-05-29T08:35:48Z

Were other related issues addressed, like arm versions of the helper and driver?

not yet, I will look into this again today, was destructed by 4.15 bundle creation and other issues.

cfergeau · 2024-05-29T09:33:47Z

was destructed by 4.15 bundle creation

Hopefully you were only distracted ;) ( https://en.wiktionary.org/wiki/destruct )

gbraad · 2024-06-21T03:50:16Z

What is the status of this overall? Seems this took a lot more effort than was anticipated.

gbraad · 2024-06-21T04:12:47Z

Makefile

 $(BUILD_DIR)/windows-amd64/crc.exe: $(SOURCES)
 	GOARCH=amd64 GOOS=windows go build -tags "$(BUILDTAGS)" -ldflags="$(LDFLAGS)" -o $@ $(GO_EXTRA_BUILDFLAGS) ./cmd/crc

 $(HOST_BUILD_DIR)/crc-embedder: $(SOURCES)
 	go build --tags="build" -ldflags="$(LDFLAGS)" -o $(HOST_BUILD_DIR)/crc-embedder $(GO_EXTRA_BUILDFLAGS) ./cmd/crc-embedder

 .PHONY: cross ## Cross compiles all binaries
-cross: $(BUILD_DIR)/macos-arm64/crc $(BUILD_DIR)/macos-amd64/crc $(BUILD_DIR)/linux-amd64/crc $(BUILD_DIR)/windows-amd64/crc.exe
+cross: $(BUILD_DIR)/macos-arm64/crc $(BUILD_DIR)/macos-amd64/crc $(BUILD_DIR)/linux-amd64/crc $(BUILD_DIR)/linux-arm64/crc $(BUILD_DIR)/windows-amd64/crc.exe


Not ready for this. Please leave this as a specific target.

Done, just have specific target not as part of cross.

This pr adds linux arm64 target to makefile and also make change to spec file to generate right package for arm64.

This is the target which is used as part of `release` and currently hardcoded to `amd64` which means even this target is run on `arm64` machine it will still create the `amd64` binary. In spec file also `release` target is used and for `arm64` builds `amd64` binary is used without this fix.

ppc64le architecture not supported by OpenShift Local as of now and it also create confusion because ppc64le package contains the amd64 binary.

admin-helper now have arm64 and amd64 binary for linux platfrom. This PR will make sure correct binary is downloaded and used for specific platform.

machine-driver-libvirt now have arm64 and amd64 binary for linux platfrom. This PR will make sure correct binary is downloaded and used for specific platform.

openshift-ci · 2024-06-21T10:50:21Z

@praveenkumar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/security	`52b9be6`	link	false	`/test security`
ci/prow/e2e-microshift-crc	`52b9be6`	link	true	`/test e2e-microshift-crc`
ci/prow/e2e-crc	`52b9be6`	link	true	`/test e2e-crc`
ci/prow/integration-crc	`52b9be6`	link	true	`/test integration-crc`

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

cfergeau · 2024-06-21T09:51:15Z

pkg/crc/preflight/preflight_checks_unix.go

-	// Only supported arches are amd64, and arm64 on macOS
-	if runtime.GOARCH == "amd64" || (runtime.GOARCH == "arm64" && runtime.GOOS == "darwin") {
+	// Only supported arches are amd64, and arm64 on macOS & Linux
+	if runtime.GOARCH == "amd64" || (runtime.GOARCH == "arm64" && runtime.GOOS != "windows") {


Might be easier to read as a switch case

switch runtime.GOARCH { case "amd64": return nil case "arm64": if runtime.GOOS != "windows" { return nil } }

I'd keep explicit linux/darwin checks instead of runtime.GOOS != "windows" to avoid returning nil on freebsd if someone tries that. This also alllow to add a warning for linux/arm64 that it's not widely tested.

cfergeau · 2024-06-21T09:51:26Z

pkg/crc/preflight/preflight_checks_unix.go

-	// Only supported arches are amd64, and arm64 on macOS
-	if runtime.GOARCH == "amd64" || (runtime.GOARCH == "arm64" && runtime.GOOS == "darwin") {
+	// Only supported arches are amd64, and arm64 on macOS & Linux
+	if runtime.GOARCH == "amd64" || (runtime.GOARCH == "arm64" && runtime.GOOS != "windows") {
 		return nil
 	}
 	return fmt.Errorf("CRC can only run on AMD64/Intel64 CPUs and Apple silicon")


This needs to be updated.

cfergeau · 2024-06-21T09:58:27Z

pkg/crc/constants/constants.go

@@ -55,7 +55,7 @@ const (

 var adminHelperExecutableForOs = map[string]string{
 	"darwin":  "crc-admin-helper-darwin",
-	"linux":   "crc-admin-helper-linux",
+	"linux":   fmt.Sprintf("crc-admin-helper-linux-%s", runtime.GOARCH),


There's a "platfrom" typo in the commit log.
I don't think the old binary name is cleaned up on upgrades?

cfergeau · 2024-06-21T10:01:03Z

packaging/rpm/crc.spec.in

@@ -30,7 +30,7 @@ Name:           %{goname}
 Release:        1%{?dist}
 Summary:        CRC's main executable
 License:        APL 2.0
-ExcludeArch:    armv7hl i686
+ExcludeArch:    armv7hl i686 ppc64le


it also create confusion because ppc64le package contains the amd64 binary.

I'd prefer to fix this rather than add artificial hurdles to ppc64le enablement if someone ever wants to try this.

If you add this, you should also remove

# crc uses `go test -race`, which triggers gvisor issues on ppc64le: # vendor/gvisor.dev/gvisor/pkg/sync/race_unsafe.go:47:6: missing function body # and a ppc64le implementation is indeed missing on ppc64le: # https://github.com/google/gvisor/blob/master/pkg/sync/race_unsafe.go # https://github.com/google/gvisor/tree/master/pkg/sync %ifnarch ppc64le make test %endif

cfergeau · 2024-06-21T11:01:15Z

packaging/rpm/crc.spec.in

@@ -30,7 +30,7 @@ Name:           %{goname}
 Release:        1%{?dist}
 Summary:        CRC's main executable
 License:        APL 2.0
-ExcludeArch:    armv7hl i686
+ExcludeArch:    armv7hl i686 ppc64le
 URL:            %{gourl}
 ExcludeArch:    s390x


Why is this separate?

cfergeau · 2024-06-21T11:04:23Z

Makefile

 else
-	$(HOST_BUILD_DIR)/crc-embedder embed --log-level debug --cache-dir=$(EMBED_DOWNLOAD_DIR) --no-download --goos=linux $(BUILD_DIR)/linux-amd64/crc
+	$(HOST_BUILD_DIR)/crc-embedder embed --log-level debug --cache-dir=$(EMBED_DOWNLOAD_DIR) --no-download --goos=linux $(BUILD_DIR)/linux-${GOARCH}/crc


Have you checked that after the naming changes for crc-admin-helper and crc-driver-libvirt, the embedding process still works fine, and that crc setup is still able to find/use the embedded files (and not download them) if they are not cached locally?
It looks like this should work, but I may have missed something.

openshift-ci bot requested review from anjannath and gbraad May 14, 2024 07:27

openshift-ci bot added the do-not-merge/hold label May 14, 2024

cfergeau reviewed May 14, 2024

View reviewed changes

packaging/rpm/crc.spec.in Outdated Show resolved Hide resolved

praveenkumar force-pushed the enable_linux_arm64 branch 2 times, most recently from 933fd76 to d997f4c Compare May 21, 2024 10:51

cfergeau reviewed May 27, 2024

View reviewed changes

praveenkumar force-pushed the enable_linux_arm64 branch from d997f4c to 757f0c2 Compare May 29, 2024 09:23

praveenkumar force-pushed the enable_linux_arm64 branch from 757f0c2 to db12a15 Compare June 20, 2024 07:57

gbraad reviewed Jun 21, 2024

View reviewed changes

praveenkumar added 5 commits June 21, 2024 14:44

Add support to generate aarch64 bits for Linux

eb06d74

This pr adds linux arm64 target to makefile and also make change to spec file to generate right package for arm64.

packaging: Don't build rpm for ppc64le arch

2574b7f

ppc64le architecture not supported by OpenShift Local as of now and it also create confusion because ppc64le package contains the amd64 binary.

constants/constants.go: Add arch info to admin-helper linux binary

cadee95

admin-helper now have arm64 and amd64 binary for linux platfrom. This PR will make sure correct binary is downloaded and used for specific platform.

libvirt/constants.go: Add arch info to machine-driver-libvirt binary

52b9be6

machine-driver-libvirt now have arm64 and amd64 binary for linux platfrom. This PR will make sure correct binary is downloaded and used for specific platform.

praveenkumar force-pushed the enable_linux_arm64 branch from db12a15 to 52b9be6 Compare June 21, 2024 09:45

cfergeau reviewed Jun 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable arm64 builds for Linux #4162

Enable arm64 builds for Linux #4162

praveenkumar commented May 14, 2024

openshift-ci bot commented May 14, 2024

praveenkumar commented May 14, 2024

cfergeau May 27, 2024

praveenkumar May 28, 2024

praveenkumar May 28, 2024

cfergeau May 28, 2024

gbraad commented May 29, 2024

praveenkumar commented May 29, 2024

cfergeau commented May 29, 2024 •

edited

gbraad commented Jun 21, 2024

gbraad Jun 21, 2024

praveenkumar Jun 21, 2024

openshift-ci bot commented Jun 21, 2024

cfergeau Jun 21, 2024

cfergeau Jun 21, 2024

cfergeau Jun 21, 2024

cfergeau Jun 21, 2024

cfergeau Jun 21, 2024

cfergeau Jun 21, 2024

Enable arm64 builds for Linux #4162

Are you sure you want to change the base?

Enable arm64 builds for Linux #4162

Conversation

praveenkumar commented May 14, 2024

openshift-ci bot commented May 14, 2024

praveenkumar commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad commented May 29, 2024

praveenkumar commented May 29, 2024

cfergeau commented May 29, 2024 • edited

gbraad commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau commented May 29, 2024 •

edited