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

Default machine #1014

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Default machine #1014

merged 2 commits into from
Mar 21, 2024

Conversation

e4t
Copy link
Contributor

@e4t e4t commented Apr 24, 2023

The default machine types picked by libvirt on non-x86* architectures are often quite limited and not very useful. For compatibility reasons libvirt picks the machine type that was the first one support for this architecture. virt-installer therefore maintains its own settings.
This patchset implements the same setting for libvirt if the user does not explicitly specify a machine type.

@dmacvicar - could you have a look?

@dmacvicar
Copy link
Owner

Thanks. I am unsure if this is a good idea. The default for the setting would not longer be depending on the schema. Why is this default not improved in libvirt itself?

@e4t
Copy link
Contributor Author

e4t commented May 22, 2023

Thanks. I am unsure if this is a good idea. The default for the setting would not longer be depending on the schema. Why is this default not improved in libvirt itself?

The machine type set in libvirt is the one that was first implemented for this architecture. The argument for not changing it to a more capable machine type would break libvirt's backward compatibility guarantees see this email
As mentioned in this email, the issue has therefore been addressed on a higher level - ie 'virt-install'.
I would argue that this terraform provider is on the same level as 'virt-install' as it replaces it.

The fix avoids unpleasant surprises if the same terraform module that works on x86_64 fails horrifically on aarch64.
The user is still free to set the machine type explicitly. This is optional, however, in which case the default is what has been set elsewhere.

@beddari
Copy link

beddari commented Jul 4, 2023

This is a great change and would be in line with what most users expect. Also, /when/ they see the non-obvious default behaviour of libvirt, they tend to not know or find out about how to deal with this in a general way.

+1.

@dmacvicar
Copy link
Owner

Ok. I am convinced, but please add a comment explaining what we are doing above the call to getMachineTypeForArch, as nobody reading the code will come back to this PR.

@e4t
Copy link
Contributor Author

e4t commented Mar 20, 2024

Ok. I am convinced, but please add a comment explaining what we are doing above the call to getMachineTypeForArch, as nobody reading the code will come back to this PR.

Done. Please check if it fits.

e4t added 2 commits March 20, 2024 08:45
If no machine is set, use default types depending on the arch.
The selection is taken from https://github.com/virt-manager/virt-manager
file virtinst/guest.py function get_recommended_machine().

Signed-off-by: Egbert Eich <eich@suse.com>
Signed-off-by: Egbert Eich <eich@suse.com>
@dmacvicar dmacvicar merged commit d41792a into dmacvicar:main Mar 21, 2024
4 checks passed
@dmacvicar
Copy link
Owner

Thanks 🥳

@e4t
Copy link
Contributor Author

e4t commented Mar 22, 2024

@dmacvicar - Thank you!

dmacvicar pushed a commit to jimnydev/terraform-provider-libvirt that referenced this pull request Sep 28, 2024
…car#1014)

* Improve 'machine' type selection

If no machine is set, use default types depending on the arch.
The selection is taken from https://github.com/virt-manager/virt-manager
file virtinst/guest.py function get_recommended_machine().

Signed-off-by: Egbert Eich <eich@suse.com>

* Test for getMachineTypeForArch()

Signed-off-by: Egbert Eich <eich@suse.com>

---------

Signed-off-by: Egbert Eich <eich@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants