On 04/24/2018 03:21 AM, Peter Krempa wrote:
> On Mon, Apr 23, 2018 at 20:00:00 -0400, John Ferlan wrote:
>> The VM Generation ID is a mechanism to provide a unique 128-bit,
>> cryptographically random, and integer value identifier known as
>> the GUID (Globally Unique Identifier) to the guest OS. The value
>> is used to help notify the guest operating system when the virtual
>> machine is executed with a different configuration.
>>
>> This patch adds support for a new "genid" XML element similar to
>> the "uuid" element. The "genid" element can have two forms "<genid/>"
>> or "<genid>$GUID</genid>". If the $GUID is not provided, libvirt
>> will generate one.
>>
>> For the ABI checks add avoidance for the genid comparison if the
>> appropriate flag is set.
>>
>> Since adding support for a generated GUID (or UUID like) value to
>> be displayed only for an active guest, modifying the xml2xml test
>> to include virrandommock.so is necessary since it will generate a
>> "known" UUID value that can be compared against for the active test.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> docs/formatdomain.html.in | 29 ++++++++++++
>> docs/schemas/domaincommon.rng | 8 ++++
>> src/conf/domain_conf.c | 59 ++++++++++++++++++++++++
>> src/conf/domain_conf.h | 3 ++
>> tests/qemuxml2argvdata/genid-auto.xml | 32 +++++++++++++
>> tests/qemuxml2argvdata/genid.xml | 32 +++++++++++++
>> tests/qemuxml2xmloutdata/genid-active.xml | 32 +++++++++++++
>> tests/qemuxml2xmloutdata/genid-auto-active.xml | 32 +++++++++++++
>> tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +++++++++++++
>> tests/qemuxml2xmloutdata/genid-inactive.xml | 32 +++++++++++++
>> tests/qemuxml2xmltest.c | 5 +-
>> 11 files changed, 295 insertions(+), 1 deletion(-)
>> create mode 100644 tests/qemuxml2argvdata/genid-auto.xml
>> create mode 100644 tests/qemuxml2argvdata/genid.xml
>> create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml
>> create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml
>> create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml
>> create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index ada0df227f..6a140f3e40 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -34,6 +34,7 @@
>> <domain type='kvm' id='1'>
>> <name>MyGuest</name>
>> <uuid>4dea22b3-1d52-d8f3-2516-782e98ab3fa0</uuid>
>> + <genid>43dc0cf8-809b-4adb-9bea-a9abb5f3d90e</genid>
>
> Since we encourage to use the variant with this being empty I'd show
> that here.
>
I'd agree except for the fact this is a "live" XML example since
"id='1'", so it should stay as is unless of course it's desired to never
show the GUID for the running VM.
>> <title>A short description - title - of the domain</title>
>> <description>Some human readable description</description>
>> <metadata>
>> @@ -61,6 +62,34 @@
>> specification. <span class="since">Since 0.0.1, sysinfo
>> since 0.8.7</span></dd>
>>
>> + <dt><code>genid</code></dt>
>> + <dd><span class="since">Since 4.3.0</span>, the <code>genid</code>
>> + element can be used to add a Virtual Machine Generation ID which
>> + exposes a 128-bit, cryptographically random, integer value identifier,
>> + referred to as a Globally Unique Identifier (GUID) using the same
>> + format as the <code>uuid</code>. The value is used to help notify
>> + the guest operating system when the virtual machine is executed
>> + with a different configuration, such as:
>> +
>> + <ul>
>> + <li>snapshot execution</li>
>> + <li>backup recovery</li>
>> + <li>failover in a disaster recovery environment</li>
>> + <li>creation from template (import, copy, clone)</li>
>> + </ul>
>> +
>> + The guest operating system notices the change and is then able to
>> + react as appropriate by marking its copies of distributed databases
>> + as dirty, re-initializing its random number generator, etc.
>> +
>> + <p>
>> + When a GUID value is not provided, e.g. using the XML syntax
>> + <genid/>, then libvirt will automatically generate a GUID.
>> + This is the recommended configuration since the hypervisor then
>> + can handle changing the GUID value for specific state transitions.
>> + Using a static GUID value may result in failures for starting from
>> + snapshot, restoring from backup, starting a cloned domain, etc.</p></dd>
>> +
>> <dt><code>title</code></dt>
>> <dd>The optional element <code>title</code> provides space for a
>> short description of the domain. The title should not contain
>
> [...]
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 6d4db50998..8c30adf850 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -18793,6 +18793,34 @@ virDomainDefParseXML(xmlDocPtr xml,
>> VIR_FREE(tmp);
>> }
>>
>> + /* Extract domain genid - a genid can either be provided or generated */
>> + if ((n = virXPathNodeSet("./genid", ctxt, &nodes)) < 0)
>> + goto error;
>> +
>> + if (n > 0) {
>> + if (n != 1) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("element 'genid' can only appear once"));
>> + goto error;
>> + }
>> + def->genidRequested = true;
>> + if (!(tmp = virXPathString("string(./genid[1])", ctxt))) {
>> + if (virUUIDGenerate(def->genid) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + "%s", _("Failed to generate genid"));
>
> Generating this here is pointless, the code using it throws this away
> and generates a new one. While we don't show this value to the user and
> thus don't create any false impressions I think that the logic should be
> shuffled around so that it's generated only when it's required.
>
>
How so? Not generating one here would cause active xml-2-xml to fail (as
I expect) because the GUID would be 00000-0000-0000-0000-000000000000.
At the end of the series the usage of virUUIDGenerate for ->genid is
done for:
1. This code, virDomainDefParseXML
2. virDomainDefCopy when genidRequested and flags &
VIR_DOMAIN_DEF_COPY_NEWGENID which is currently only used by the
snapshot code, but could be used elsewhere - something I may have been
thinking about at least w/r/t one of the qemu_driver paths.
3. qemuProcessGenID *when* flags & VIR_QEMU_PROCESS_START_GEN_VMID
If anything, perhaps what you may be referring to is qemuProcessGenID
processing using the VIR_QEMU_PROCESS_START_GEN_VMID flag when called
from qemuDomainRevertToSnapshot. The transitions there caused me to be
"super cautious", but the Copy, then Start I think upon reflection does
overwrite. The same flag is used in qemuDomainSaveImageStartVM, which
yes does overwrite, but that's by rule/design.
Perhaps it'll help to summarize the transitions...
Change is not required for the following transitions:
-> Pause/Resume
-> VM Reboot
-> Host reboot
-> Live migrate
-> Failover in a clustered environment
Change is required for the following transitions:
-> Application of offline/online snapshot
-> Restoring from backup
-> Failover to replication target
-> Importing a VM (or copy or clone)
Change is "Unspecified" when a VM's configuration changes.
So the 'design decisions' were:
- For snapshot, the choice was use the virDomainDefCopy and ABI flags.
- For the restore from backup, the choice was regenerate and store in
the config during startup processing. The docs indicate "The restore
sequence will be modified to post process the restore target and apply
new generation identifiers to the restored configuration files." - so
where it was done would
- It's not 100% clear if we need something special for failover from
replication target. Seems like Snapshot or Save/Restore is in the same
category, but perhaps there's some hypervisor specific transition.
- For import and copy, changing the genid of the copy "seems right".
The tricky part is the clone code which would require a separate change
to virt-clone since it uses parses and formats XML in separate passes.
So given all that - are you still of the opinion that the design needs
to change even more? If so, then please also characterize your view of
how this should work.
Tks -
John
>> + }
>> + def->genidGenerated = true;
>> + } else {
>> + if (virUUIDParse(tmp, def->genid) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + "%s", _("malformed genid element"));
>> + goto error;
>> + }
>> + }
>> + }
>> + VIR_FREE(nodes);
>> +
>> /* Extract short description of domain (title) */
>> def->title = virXPathString("string(./title[1])", ctxt);
>> if (def->title && strchr(def->title, '\n')) {
>> @@ -21904,6 +21932,26 @@ virDomainDefCheckABIStabilityFlags(virDomainDefPtr src,
>> goto error;
>> }
>>
>> + if (src->genidRequested != dst->genidRequested) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("Target domain requested genid does not match source"));
>> + goto error;
>> + }
>> +
>> + if (src->genidRequested &&
>> + !(flags & VIR_DOMAIN_DEF_ABI_CHECK_SKIP_GENID) &&
>
> The above change will also possibly remove the need for this flag. In
> cases when it needs to change (snapshots) the code is skipping the check
> anyways. In cases when it needs to stay the same we will know that since
> it will be filled in.
>
>> + memcmp(src->genid, dst->genid, VIR_UUID_BUFLEN) != 0) {
>> + char guidsrc[VIR_UUID_STRING_BUFLEN];
>> + char guiddst[VIR_UUID_STRING_BUFLEN];
>> +
>> + virUUIDFormat(src->genid, guidsrc);
>> + virUUIDFormat(dst->genid, guiddst);
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("Target domain genid %s does not match source %s"),
>> + guiddst, guidsrc);
>> + goto error;
>> + }
>> +
>> /* Not strictly ABI related, but we want to make sure domains
>> * don't get silently re-named through the backdoor when passing
>> * custom XML into various APIs, since this would create havoc
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
