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.

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

actions: enable arm64 images for operator #474

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

huoqifeng
Copy link
Contributor

@huoqifeng huoqifeng commented Dec 12, 2024

Added arm64 images build for operator and it's pre-reqs payload

@huoqifeng huoqifeng requested a review from a team as a code owner December 12, 2024 09:57
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

Hey @huoqifeng - just to confirm the scope of this PR. This is adding arm64 archs to the multi-arch container images for the operator and pre-reqs image, but I don't think that the kata-deploy payload for arm64 builds the confidential features (notably the confidential image), so I'm not expecting it to work yet?

@huoqifeng
Copy link
Contributor Author

Hey @huoqifeng - just to confirm the scope of this PR. This is adding arm64 archs to the multi-arch container images for the operator and pre-reqs image, but I don't think that the kata-deploy payload for arm64 builds the confidential features (notably the confidential image), so I'm not expecting it to work yet?

@stevenhorsman yes, I understand and will use separate PR for the remains.

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @huoqifeng!

@seungukshin
Copy link

Some e2e test may fail because arcitecture is not set properly for arm64. The following change would be needed.

diff --git a/tests/e2e/ansible/group_vars/all b/tests/e2e/ansible/group_vars/all
index 3a78652..b9a6eb6 100644
--- a/tests/e2e/ansible/group_vars/all
+++ b/tests/e2e/ansible/group_vars/all
@@ -34,4 +34,7 @@ test_pkgs:
     - jq
   centos:
     - jq
-target_arch: "{{ 'amd64' if ansible_architecture == 'x86_64' else ansible_architecture }}"
+binaries_architecture:
+  x86_64: amd64
+  aarch64: arm64
+target_arch: "{{ binaries_architecture[ansible_architecture] | default(ansible_architecture) }}"

Co-authored-by: Huo Qifeng <[email protected]>
Co-authored-by: Seunguk Shin <[email protected]>

Signed-off-by: Huo Qifeng <[email protected]>
@huoqifeng
Copy link
Contributor Author

Some e2e test may fail because arcitecture is not set properly for arm64. The following change would be needed.

diff --git a/tests/e2e/ansible/group_vars/all b/tests/e2e/ansible/group_vars/all
index 3a78652..b9a6eb6 100644
--- a/tests/e2e/ansible/group_vars/all
+++ b/tests/e2e/ansible/group_vars/all
@@ -34,4 +34,7 @@ test_pkgs:
     - jq
   centos:
     - jq
-target_arch: "{{ 'amd64' if ansible_architecture == 'x86_64' else ansible_architecture }}"
+binaries_architecture:
+  x86_64: amd64
+  aarch64: arm64
+target_arch: "{{ binaries_architecture[ansible_architecture] | default(ansible_architecture) }}"

Thank you @seungukshin for the nice finding, I lost it as the PR CI seems did not pick the test run. Fixed it by applied your patch!

Copy link

@seungukshin seungukshin left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @huoqifeng!

@huoqifeng
Copy link
Contributor Author

@confidential-containers/operator-maintainers may you please help kick the PR test job?

@huoqifeng
Copy link
Contributor Author

@wainersm @BbolroC @bpradipt may you help review?

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Thanks, builds&installs well although I noticed our e2e testing always builds all archs, which should be IMO changed to save some time. Anyway it's not related to this PR so I'd say it's good to go.

@ldoktor
Copy link
Contributor

ldoktor commented Dec 16, 2024

And it runs e2e on aarch64 well as well. Unfortunately IIRC we don't have the enterprise GH account so we can't enable it on arm just yet, right?

As for the functional testing, I do not have arm machine with KVM support handy so I just tried install/reinstall/uninstall only.

Overall let's get this in so people can start experimenting and let's discuss the potential e2e tests optimization elsewhere...

@ldoktor ldoktor merged commit dcefda3 into confidential-containers:main Dec 16, 2024
19 checks passed
@huoqifeng huoqifeng deleted the arm64 branch December 17, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants