On 04/25/2018 04:32 AM, Peter Krempa wrote:
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@redhat.com> --- 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.
Hmm, right. It feels slightly wrong though that we are describing configuration on the example of a live XML.
I can remove the id='1' and then use <genid/>... It's not that important to print the GUID to me...
<title>A short description - title - of the domain</title> <description>Some human readable description</description> <metadata>
[...]
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.
As noted in the other part of this reply thread - it changes for specific transactions. It's not clear "how" it's initially generated - whether user specified or self generated - only that for certain transitions, it must change and doing so is done by generating a new one.
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.
Well you may call it wrong, but that's what is in the spec.
-> 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).
Hopefully covered in the other half of this thread.
-> 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.
It can change based on transitions that require it or it can be used as found in the config file. This is the real bugger in the description.
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.
Wouldn't the GUID for the creation of the snapshot be whatever was set? The whole snapshot config and domain config saved within is not something I depend on others to understand in greater detail than I do. Still, my choice was alter the GUID when starting and causing an error for the transition to use qemuMonitorLoadSnapshot since we cannot modify the genid. Again, another area that I hoped review would provide details if this was the "wrong choice".
- 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.
Cannot disagree - the choice was to change only when/if we restart the VM since that's really the only time it matters.
- 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.
It was me commenting and trying not to read too much into things. Perhaps the shorter version of your failover comments from above.
- 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.
It's not really clear that a new GUID for every new boot is "required".
If the documentation does not allow for the above it will be more complex and we'll need to discuss that further.
We shouldn't over complicate things either. John [the rest is covered in the other thread - this is the hazards of cutting threads up like this]
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.