Change save/restore nvram data logic

[antonios-f]

Save NVRAM data not only if BiosType is Q35_SECURE_BOOT Also save if Q35_OVMF because it also includes EFI boot variables With Q35_OVMF we unable to boot non-fallback bootloaders in place different from /EFI/BOOT/BOOT.EFI

To check this it needed to:

1. Run VM with installed Linux (it doesn't matter what distro) 2) Add a bootloader via efibbotmgr
   efibootmgr -c -d -L "Some Linux" -l \EFI\\grubx64.efi
2. Look at created efi boot variable
   efibootmgr -v
   It's present
3. Shutdown VM
4. Run it again
5. Check efi boot variables again
   efibootmgr -v
6. We got non bootable VM if fallback bootloader is broken or boot default esp/EFI/BOOT/BOOT.EFI
   otherwise

[mz-pdm]

The idea looks OK to me, IIRC there are no inherent limitations when keeping NVRAM data. It adds some Vdsm traffic and other machinery, even if the EFI variables are not modified I think, but this doesn't differ from using secure boot.

One thing I suggest to check with this patch is whether the NVRAM data is deleted when switching the VM to secure boot and back. Should it be deleted or not?

[antonios-f]

NVRAM data will be deleted when switching the VM from secure boot, by this trigger

CREATE OR REPLACE FUNCTION remove_nvram_data ()
RETURNS TRIGGER AS $$
BEGIN
    IF OLD.bios_type = 4 AND NEW.bios_type != 4 THEN
        DELETE FROM vm_nvram_data
        WHERE vm_id = OLD.vm_guid;
    END IF;
    RETURN NEW;
END;$$
LANGUAGE plpgsql;

CREATE TRIGGER remove_nvram_data_on_update AFTER
UPDATE
    ON vm_static
FOR EACH ROW
EXECUTE FUNCTION remove_nvram_data();

When switching from Uefi to Secure Boot it will be not deleted.

[mz-pdm]

Now the question is whether it's good to delete the NVRAM data when switching from secure boot. I'd say it's on the safe side -- deleting any possible secrets stored there. It will delete the other EFI variables too though but one shouldn't switch an installed VM from secure boot without a good reason so it doesn't look like a big problem. Do you agree?

[antonios-f]

I think yes.

[smelamud]

CREATE OR REPLACE FUNCTION remove_nvram_data ()
RETURNS TRIGGER AS $$
BEGIN
IF OLD.bios_type = 4 AND NEW.bios_type != 4 THEN

Since now bios_type = 3 can also contain NVRAM data. So, if I understand correctly, switching from it should also delete the data.

[smelamud]

Looks good, just need to check the stored procedures again, as mentioned above.

[mz-pdm]

Looks good to me and I think we agreed on the behavior regarding data deletion on BIOS switching.

[smelamud]

@mz-pdm With this patch, NVRAM data will be available with BIOS type Q35_OVMF (bios_type = 3). But remove_nvram_data will not delete it when switching bios_type from 3 to 2, for example. While it is deleted when switching from 4 to 2. Correct me, if I'm wrong.

[mz-pdm]

@smelamud Ah, I see your point now. Do you think it should be deleted in such a case? There is some reason to delete it when switching from secure boot (due to possible presence of secrets, not to keep them around unnecessarily). Perhaps as a general cleanup in other situations? A related question would be whether there is a confirmation dialog for the given deletion that should be updated too.

[smelamud]

@mz-pdm Yes, as a general cleanup, I think. And the confirmation dialog should be shown, because BIOS type change may be unintentional.

[antonios-f]

The confirmation dialog was added, as you described. Please look at this.

[barpavel]

LGTM, some minor suggestions, up to you.

[antonios-f]

Everything has been fixed as posted. Please look at this.

[barpavel]

Everything has been fixed as posted. Please look at this.

Except 2 small suggestions as a follow-up of the previous code review, LGTM.

[barpavel]

My comments were addressed, LGTM.
But suggest to wait for more approvals.

[sandrobonazzola]

Hi, looks like DCO test is failing, can you please follow instructions on https://github.com/oVirt/ovirt-engine/pull/887/checks?check_run_id=23100801322 for signing off your commits?

[antonios-f]

All done. Please look at this.

[sandrobonazzola]

Build fails on:

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.10.1:compile (default-compile) on project uicommonweb: Compilation failure: Compilation failure: 
Error:  /github/home/rpmbuild/BUILD/ovirt-engine-4.5.5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java:[799,112] illegal start of expression
Error:  /github/home/rpmbuild/BUILD/ovirt-engine-4.5.5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java:[800,117] ';' expected
Error:  /github/home/rpmbuild/BUILD/ovirt-engine-4.5.5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java:[807,15] 'else' without 'if'

[antonios-f]

I'm sorry. already fixed

[smelamud]

Looks good, thanks!

[sandrobonazzola]

/ost

[sandrobonazzola]

/ost

[sandrobonazzola]

/ost

[sandrobonazzola]

OST passed, merging based on previous approvals