search

fog / fog-libvirt

libvirt provider for fog
MIT License
16 stars 42 forks source link

Remove NVRAM in undefine call #107

Closed NPellet closed 8 months ago

NPellet commented 2 years ago
  1. Use splat to pass any argument to the function call
  2. In undefine, pass the argument to remove the NVRAM

Arguably, the flag could come from the caller, and not from fog-libvirt itself. What do you think ?

PR related to https://github.com/vagrant-libvirt/vagrant-libvirt/pull/1372

ekohl commented 2 years ago

Could you help me understand what NVRAM is the context of libvirt and why it needs to be undefined in the stop action? What are the implications for machines?

NPellet commented 2 years ago

I'm probably not the right person to do that, but my understanding is that UEFI physical machines have some kind of NVRAM chip which stores some state permenantly (like the boot order and whatnot). In libvirt, OVMF emulates that using an OVMF_VARS.fd file somewhere on the host system and which needs to be defined in /domain/os/nvram. In libvirt, when using UEFI, one has to assign a unique NVRAM variable file per domain, as far as I understand.

When the domain is undefined, we need to tell libvirt if we should keep the NVRAM file or not, hence the flag I added. Without it, the undefine call fails.

lzap commented 2 years ago

LGTM however the only concern is compatibility. This was added in:

https://github.com/libvirt/libvirt/commit/273b6581ca8dae11e6ff40e3d13813fdbb37d41b

into 1.2.6 version, pretty old it looks like but RHEL 7 hypervisors are still pretty common. So to double check: https://access.redhat.com/downloads/content/libvirt/1.1.1-12.el7/x86_64/f21541eb/package

Therefore I recommend to make a decision to support libvirt 4.5 and higher (EL7 users are required to update to the latest version). Can you make a note in the README that the oldest supported libvirt version is 4.5?

Tests do fail.

ekohl commented 2 years ago

Can it use if defined?(Libvirt::Domain::UNDEFINE_NVRAM) for compatibility with older versions?

NPellet commented 2 years ago

Good points.

I'm thinking anyway whether to keep the NVRAM or not should be a parameter in the undefine call. I'll make the necessary modifications

ekohl commented 2 years ago

My intuition says it should be opt-in. That would also keep the API compatible. However, I don't know enough about it to answer it with certainty.

lzap commented 2 years ago

We are just about to close the 0.9.1 release, please push now if you want this in. An opt-in argument we can accept without fear.

github-actions[bot] commented 2 years ago

This pr has been marked inactive and will be closed if no further activity occurs.

ekohl commented 8 months ago

At this point I have fewer concerns for compatibility. By now there's at least a merge conflict with 085a7c89596e989921dccd33dd954afbb37df27a. I've opened https://github.com/fog/fog-libvirt/pull/132, which I think should fix implement this.