search

dmacvicar / terraform-provider-libvirt

Terraform provider to provision infrastructure with Linux's KVM using libvirt
Apache License 2.0
1.54k stars 457 forks source link

Default machine #1014

Closed e4t closed 3 months ago

e4t commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 12 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

dmacvicar commented 3 months ago

Thanks :partying_face:

e4t commented 3 months ago

@dmacvicar - Thank you!