[libvirt] [PATCH 3/6] conf: Introduce infrastructure to add config validation to define time

[Cole Robinson]

On 05/18/2016 08:43 AM, Pavel Hrdina wrote:
> On Tue, May 17, 2016 at 04:25:39PM +0200, Peter Krempa wrote:
>> Recently there were a few patches adding checks to the post parse
>> callback infrastructure that rejected previously accepted configuration.
>> This unfortunately was not acceptable since we would fail to load the
>> configs from the disk and the VMs would vanish.
>>
>> This patch adds helpers which are called in the appropriate places after
>> the config is parsed when defining/starting a VM but not when reloading
>> the configs.
>>
>> This infrastructure is meant for invalid configurations that were
>> accepted previously and thus it's not executed when starting a VM from
>> any saved state. Any checks that are necessary to be executed at that
>> point still need to be added to qemuProcessValidateXML.
>> ---
>>  src/conf/domain_conf.c   | 67 ++++++++++++++++++++++++++++++++++++++----------
>>  src/conf/domain_conf.h   |  2 ++
>>  src/libvirt_private.syms |  1 +
>>  src/qemu/qemu_conf.h     |  2 ++
>>  src/qemu/qemu_domain.c   | 21 +++++++++++++++
>>  src/qemu/qemu_driver.c   |  6 +++++
>>  src/qemu/qemu_process.c  | 22 +++++++++++-----
>>  7 files changed, 100 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> 
> [...]
> 
>> +/**
>> + * virDomainDefValidate:
>> + * @def: domain definition
>> + *
>> + * This validation function is designed to take checks of globally invalid
>> + * configurations that the parser needs to accept so that VMs don't vanish upon
>> + * daemon restart. Such definition can be rejected upon startup or define, where
>> + * this function shall be called.
>> + *
>> + * Returns 0 if domain definition is valid, -1 on error and reports an
>> + * appropriate message.
>> + */
>> +int
>> +virDomainDefValidate(const virDomainDef *def)
>> +{
>> +    size_t i;
>> +
>> +    /* check configuration of individual devices */
>> +    VIR_DOMAIN_DEF_VALIDATE_DEVICES(disk, virDomainDiskDefValidate);
>> +
>> +    return 0;
>> +}
>> +
>> +#undef VIR_DOMAIN_DEF_VALIDATE_DEVICES
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> @@ -5343,3 +5343,24 @@ qemuDomainDefValidateDiskLunSource(const virStorageSource *src)
>>
>>      return 0;
>>  }
>> +
>> +
>> +/**
>> + * qemuDomainDefValidate:
>> + * @def: domain definition
>> + *
>> + * Validates that the domain definition is valid for the qemu driver. This check
>> + * is run when the domain is defined or started as new. This check is explicitly
>> + * not executed when starting a VM from snapshot/saved state/migration to allow
>> + * partially invalid configs to still run.
>> + *
>> + * Returns 0 on success, -1 on error.
>> + */
>> +int
>> +qemuDomainDefValidate(const virDomainDef *def)
>> +{
>> +    if (virDomainDefValidate(def) < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 7f8dfc8..e6d22ec 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1799,6 +1799,9 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
>>      if (virDomainCreateXMLEnsureACL(conn, def) < 0)
>>          goto cleanup;
>>
>> +    if (qemuDomainDefValidate(def) < 0)
>> +        goto cleanup;
>> +
>>      if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator)))
>>          goto cleanup;
>>
>> @@ -7275,6 +7278,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>>      if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
>>          goto cleanup;
>>
>> +    if (qemuDomainDefValidate(def) < 0)
>> +        goto cleanup;
>> +
>>      if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator)))
>>          goto cleanup;
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> 
> [...]
> 
>> @@ -4634,6 +4627,21 @@ qemuProcessStartValidateXML(virDomainObjPtr vm,
>>          return -1;
>>      }
>>
>> +    /* checks below should not be executed when starting a qemu process for a
>> +     * VM that was running before (migration, snapshots, save). It's more
>> +     * important to start such VM than keep the configuration clean */
>> +    if (!migration && !snapshot) {
>> +        if (qemuDomainDefValidate(vm->def) < 0)
>> +            return -1;
>> +
>> +        if (virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
>> +            return -1;
>> +    }
>> +
>> +    /* checks below validate config that would make qemu fail to start */
>> +    if (qemuValidateCpuCount(vm->def, qemuCaps) < 0)
>> +        return -1;
>> +
>>      return 0;
>>  }
> 
> I would suggest to do the opposite.  That would mean adding the
> virDomainDefValidate() after postParse callbacks and there you would call
> xmlopt->config.domainDefValidate().  Calling virDomainDefValidate() after
> postParse would be conditional based on a new VIR_DOMAIN_DEF_PARSE_... flag
> which would have to be set at the appropriate places so we would not run this
> new validation while starting a libvirtd and the migration/snapshot/save case.
> 
> The motivation to do it this way is:
> 
> 1. reuse existing code,
> 2. all places, that has to be skipped would be covered and we wouldn't miss
> calling the qemuDomainDefValidate() for tests like this patch do,
> 3. if you add something new, where the validation needs to be skipped, you would
> notice it while testing the new code and that would force you to think about it.
> 

Yes, this was my thinking too. We already have a targeted flag that does this
as a one off: VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS. Converting that stuff
to a PostParse validation function and renaming the flag could be a good
initial imple, since it already has test suite representation IIRC

Another topic related to this: we already have another chunk of qemu specific
validation in qemuBuildCommandLineValidate, and lots of bits sprinkled within
the command line builder. Do we eventually want to wire that up to PostParse
too? So users will get immediate feedback if they try to define XML with some
feature that their qemu binary doesn't support.

Regardless, after the initial plumbing patches are pushed, we should decide
what the convention will be and explicitly try to enforce it: all new generic
domain conf validation checks go in function XX, qemu specific define time
checks go in YY, optionally qemu startup time checks go in ZZ. Rather than the
mishmash of checks sprinkled across the XML parser, cli builder, address
assignment, plus specific validation functions.

Thanks,
Cole