[PATCH v2 0/8] Introduce network backed NVRAM

[Rohit Kumar]

Thanks for taking the time to review this, Ani!

On 09/04/22 8:22 pm, Ani Sinha wrote:
> On Fri, Apr 8, 2022 at 11:19 PM Rohit Kumar <rohit.kumar3 at nutanix.com> wrote:
>> Libvirt domain XML currently allows only local filepaths
>> that can be used to specify a NVRAM disk.
>> Since, VMs can migrate across hypervisor hosts, it should be
>> possible to allocate NVRAM disks on network storage for
>> uninterrupted access.
>>
>> This series extends the NVRAM element to support hosting over
>> network-backed disks, for high availability.
>> It achieves this by embedding virStorageSource pointer for
>> nvram into _virDomainLoaderDef.
>>
>> It introduces a 'type' attribute for NVRAM element to
>> specify 'file' vs 'network' backed NVRAM.
>>
>> Changes v1->v2:
>>   - Split the patch into smaller patches
>>   - Added unit test
>>   - Updated the doc
>>   - Addressed Peter's comment on v1 (https://urldefense.proofpoint.com/v2/url?u=https-3A__listman.redhat.com_archives_libvir-2Dlist_2022-2DMarch_229684.html&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs&m=2qaiZSbuvF3-NV3omL5Oy7HR2A7UtREZi77Fq1EtOI0rL4dFj8bT74YoovtK373G&s=UFWNI0THYsFToxpv6P_ZHpTzpIB45eECG3Ltl8rRYHc&e= )
> Ok so without going deeper into the actual change, following are some
> quick comments based on some of my own experience of introducing new
> conf options in libvirt:
>
> (a) Please update NEWS.rst t to document the new xml attribute support
> for the next release. This should be the last patch in the series
> preferrably.
Yes, thanks. I will update it.
> (b) Please put patch #2 and #5 together. Also please prefix the first
> line with "conf:" I think patch #4 should also go together but I will
> let others comment.
On patch v1, Peter suggested to saperate the cleanups in a different patch.
Sure, putting  #2 and #5 would be good idea.
> (c) It's better that the unit tests (patches #7 and #8) go along with
> the changes in the same patch. Then when cherry picking the unit tests
> will be picked along with the change itself.
Yes, this seems logical. I would also prefer to wait for Peter's review 
before sending out the next patch.
> (d) also I have commented separately, your validation patch needs
> additional unit tests to check the validation actually works.
Ack., thanks.
>
>
>
>
>
>> Rohit Kumar (8):
>>    Make NVRAM a virStorageSource type.
>>    Add support to parse/format virStorageSource type NVRAM
>>    Validate remote store NVRAM
>>    Cleanup diskSourceNetwork and diskSourceFile schema
>>    Update XML schema to support network backed NVRAM
>>    Update NVRAM documentation
>>    Add unit test for network backed NVRAM
>>    Add unit test to support new 'file' type NVRAM
>>
>>   docs/formatdomain.rst                         | 43 +++++++--
>>   src/conf/domain_conf.c                        | 88 ++++++++++++++++---
>>   src/conf/domain_conf.h                        |  2 +-
>>   src/conf/schemas/domaincommon.rng             | 80 +++++++++++------
>>   src/qemu/qemu_cgroup.c                        |  3 +-
>>   src/qemu/qemu_command.c                       |  2 +-
>>   src/qemu/qemu_domain.c                        | 14 +--
>>   src/qemu/qemu_driver.c                        |  5 +-
>>   src/qemu/qemu_firmware.c                      | 23 +++--
>>   src/qemu/qemu_namespace.c                     |  5 +-
>>   src/qemu/qemu_process.c                       |  5 +-
>>   src/qemu/qemu_validate.c                      | 22 +++++
>>   src/security/security_dac.c                   |  6 +-
>>   src/security/security_selinux.c               |  6 +-
>>   src/security/virt-aa-helper.c                 |  5 +-
>>   src/vbox/vbox_common.c                        |  2 +-
>>   .../bios-nvram-file.x86_64-latest.args        | 37 ++++++++
>>   tests/qemuxml2argvdata/bios-nvram-file.xml    | 23 +++++
>>   .../bios-nvram-network.x86_64-latest.args     | 37 ++++++++
>>   tests/qemuxml2argvdata/bios-nvram-network.xml | 25 ++++++
>>   tests/qemuxml2argvtest.c                      |  2 +
>>   21 files changed, 360 insertions(+), 75 deletions(-)
>>   create mode 100644 tests/qemuxml2argvdata/bios-nvram-file.x86_64-latest.args
>>   create mode 100644 tests/qemuxml2argvdata/bios-nvram-file.xml
>>   create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.x86_64-latest.args
>>   create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml
>>
>> --
>> 2.25.1
>>