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

lxd/instance: Add support for armhf vm's on arm64 hosts #14077

Merged
merged 1 commit into from
Sep 12, 2024
Merged

lxd/instance: Add support for armhf vm's on arm64 hosts #14077

merged 1 commit into from
Sep 12, 2024

Conversation

kadinsayani
Copy link
Contributor

@kadinsayani kadinsayani commented Sep 9, 2024 •
edited
Loading

This improvement will resolve #14001 with the inclusion of an armhf 32-bit lxd-agent binary in the lxd snap.

simondeziel
simondeziel previously approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@simondeziel simondeziel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM but should we rework those else if to use or comparison and avoid dup'ing the if's body? The aim would be to reduce the code duplication.

@kadinsayani kadinsayani force-pushed the 14001-launch-armhf-vm-arm64-host branch from 386da99 to 0df5c94 Compare September 10, 2024 16:27
simondeziel
simondeziel reviewed Sep 10, 2024
View reviewed changes
lxd/instance/drivers/driver_qemu_templates.go Outdated
@@ -74,6 +74,10 @@ func qemuMachineType(architecture int) string {
switch architecture {
case osarch.ARCH_64BIT_INTEL_X86:
machineType = "q35"
case osarch.ARCH_32BIT_ARMV7_LITTLE_ENDIAN:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case osarch.ARCH_32BIT_ARMV7_LITTLE_ENDIAN:
case osarch.ARCH_32BIT_ARMV7_LITTLE_ENDIAN, osarch.ARCH_32BIT_ARMV8_LITTLE_ENDIAN, osarch.ARCH_64BIT_ARMV8_LITTLE_ENDIAN:

Copy link
Member

Choose a reason for hiding this comment

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

As always, my questions/suggestions on anything related with Go are to be taken with a large grain of salt. @tomponline is really the authoritative voice here. I'm still just a Go noob ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this one out! I didn't know you could handle cases with multiple options this way. I'm still learning something new about Go every day 😄

@kadinsayani
lxd/instance: Add support for armhf vm's on arm64 hosts
973c2ef 
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
@kadinsayani kadinsayani force-pushed the 14001-launch-armhf-vm-arm64-host branch from 0df5c94 to 973c2ef Compare September 10, 2024 16:34
@tomponline
Copy link
Member

@kadinsayani if this is ready for review please can you click the "Ready for review" button, ta

@kadinsayani kadinsayani marked this pull request as ready for review September 11, 2024 10:45
tomponline
tomponline approved these changes Sep 11, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Has this been tested working @kadinsayani ?

@kadinsayani
Copy link
Contributor Author

kadinsayani commented Sep 12, 2024 •
edited
Loading

Has this been tested working @kadinsayani ?

Launching armhf vm's on a 64-bit arm rpi host, yes ✅

Running lxc exec c1 -- sh with a snap that includes an armhf 32-bit lxd-agent binary, not yet ❌

I'll provide an update on this tomorrow 😄

@tomponline
Copy link
Member

Running lxc exec c1 -- sh with a snap that includes an armhf 32-bit lxd-agent binary, not yet ❌

Yeah doing multi arch agent is a different feature.

So this pr ready to review?

@tomponline tomponline merged commit 3fd5b76 into canonical:main Sep 12, 2024
29 checks passed
@kadinsayani kadinsayani deleted the 14001-launch-armhf-vm-arm64-host branch September 12, 2024 14:08
@simondeziel simondeziel mentioned this pull request Sep 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simondeziel simondeziel simondeziel approved these changes

@tomponline tomponline tomponline approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to launch armhf VMs on arm64 hosts
3 participants
@kadinsayani @tomponline @simondeziel