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@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.
>
> 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.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
