Re: [libvirt] [PATCH v2 06/11] conf: Add VM Generation ID parse/format support

On Tue, Apr 24, 2018 at 15:38:40 -0400, John Ferlan wrote: > > > 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(a)redhat.com&gt; >>> --- >>> 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 @@ >>> &lt;domain type='kvm' id='1'&gt; >>> &lt;name&gt;MyGuest&lt;/name&gt; >>> &lt;uuid&gt;4dea22b3-1d52-d8f3-2516-782e98ab3fa0&lt;/uuid&gt; >>> + &lt;genid&gt;43dc0cf8-809b-4adb-9bea-a9abb5f3d90e&lt;/genid&gt; >> >> 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. Hmm, right. It feels slightly wrong though that we are describing configuration on the example of a live XML.

> >>> &lt;title&gt;A short description - title - of the domain&lt;/title&gt; >>> &lt;description&gt;Some human readable description&lt;/description&gt; >>> &lt;metadata&gt; [...] >>> 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 Basically because a GUID for an inactive definition is nonsense. It will necessarily change upon startup, so having it generated in the parser is pointless.

The parser only needs to restore a user-provided GUID or the one which is store > 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 Both above are the same emulator instance. > -> Host reboot This is wrong. Host reboot causes the VM to be killed. So when booting the GUID will change since we start a new process. While it should not result in any problems if the ID would not change (since the guest OS rebooted anyways) we should treat this as start of the VM.

> -> Live migrate This is technically the same emulator instance. No rollback of execution was possible. > -> Failover in a clustered environment Thankfully it does not apply at this point since we don't support any of this implicitly. It also probably depends on the way the failover scenario is executed. In the qemu world I read only about coarse-grained lockstepping as a case of failover and that has situations where some rollback could happen, so in that case the VM id should probably change. But as said, thankfully we don't have to deal with it currenly and also it would not be possible since qemu can't change the id during runtime. > Change is required for the following transitions: > > -> Application of offline/online snapshot > -> Restoring from backup This is too vague. If you mean a disk backup, the VM will boot, thus the ID should change (unless the user specified an explicit one).

> -> Failover to replication target See above. > -> Importing a VM (or copy or clone) > > Change is "Unspecified" when a VM's configuration changes. So in our case it will change.

> > So the 'design decisions' were: > > - For snapshot, the choice was use the virDomainDefCopy and ABI flags. Actually, the GUID when creating the snapshot is irrelevant. When reverting a snapshot the ID should always change thus it will always require an emulator restart.

> - 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 As pointed out above, this is vague. If it's a restore of the disk state only, the guid will necessarily change in our design when we start the new qemu process.

> - 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. What is a failover here? You mean if a management starts a new VM after a different host with VM has failed? In such case it's a new start of VM for us and thus will get a new GUID.

> - 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. That depends on the implementation. Currently yes. > 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. Well, that depends. I did not read the docs for this thoroughly enough. If it is okay for us to generate a new GUID upon every boot of a VM then this will be for a rather simple implementation, since we have a very limited set of situations when we are starting a new qemu process which should NOT change the GUID and we will change it in all other scenarios.

If the documentation does not allow for the above it will be more complex and we'll need to discuss that further.

A second consideration then is whether to allow user-specified GUID at all, but I guess mgmt applications may have a different idea when it's necessary to change and thus it makes sense to allow that. For that situation the provided GUID should be always honoured. This means that we'll probably need a new attribute which will specify that the GUID is custom (or the opposite, that it was generated). If that property is in the default state the startup procedure should generate a new GUID exept for the migration case. We also might want to consider the managed-save case to bear the same GUID after startup, since we know that we start the new qemu process from the same state as we've saved it.