[pve-devel] [PATCH qemu-server] fix #1908: add vmgenid config/device

[Thomas Lamprecht]

On 9/18/18 10:19 AM, Dominik Csapak wrote:
> this adds a VM Generation ID device uses by Windows (Server) to determine
> some specific actions that may have happened with the vm
> such as rollback, restore, etc.
> 

shouldn't this be handled more like the MAC from a NIC?
I.e., explicitly generate one when needed, not doing magic auto stuff?

You could do this automatically for all VMs, while it comes from
windows it's intended to be guest OS agnostic and is exposed over
fw_cfg/ACPI, AFAIS.[0]

Maybe add none if there's any "hide that we virtualize" flag is on,
but else I do not really see a point in not doing this?
(allowing to let the user disable it with '0', seems nonetheless
a possible reasonable option)

> see:
> 
> https://docs.microsoft.com/en-us/windows/desktop/hyperv_v2/virtual-machine-generation-identifier
> 
> for details on how it works and when it should change
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/API2/Qemu.pm          |  5 +++++
>  PVE/QemuConfig.pm         |  4 ++++
>  PVE/QemuServer.pm         | 23 +++++++++++++++++++++++
>  PVE/QemuServer/Makefile   |  1 +
>  PVE/QemuServer/VMGenID.pm | 29 +++++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+)
>  create mode 100644 PVE/QemuServer/VMGenID.pm
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index c1cc01b..6ac3110 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -2732,6 +2732,11 @@ __PACKAGE__->register_method({
>  	    $smbios1->{uuid} = $uuid_str;
>  	    $newconf->{smbios1} = PVE::QemuServer::print_smbios1($smbios1);
>  
> +	    # auto generate a new vmgenid
> +	    if ($oldconf->{vmgenid}) {
> +		$newconf->{vmgenid} = 'auto';

why not actually just generate one here?
This seems to just unnecessarily add another in-between step?

As QEMU allows to use auto for this, auto-generating a new vmgenid at each
start, but you also detect auto and generate one here this results in
possible confusion/intransparency about who the generator is, at least when
looking at the code.
Also an user could think this gets passed directly to qemu and be confused too.


> +	    }
> +
>  	    delete $newconf->{template};
>  
>  	    if ($param->{name}) {
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index cd116bd..5a05c1b 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -300,6 +300,10 @@ sub __snapshot_rollback_hook {
>  	    # in the original config.
>  	    delete $conf->{machine} if $snap->{vmstate} && !defined($data->{oldmachine});
>  	}
> +
> +	if (defined($conf->{vmgenid})) {
> +	    $conf->{vmgenid} = 'auto';
> +	}
>      }
>  
>      return;
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index af0631d..1b68ddd 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -34,6 +34,7 @@ use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr);
>  use PVE::QemuServer::Memory;
>  use PVE::QemuServer::USB qw(parse_usb_device);
>  use PVE::QemuServer::Cloudinit;
> +use PVE::QemuServer::VMGenID;
>  use PVE::Systemd;
>  use Time::HiRes qw(gettimeofday);
>  use File::Copy qw(copy);
> @@ -559,6 +560,12 @@ EODESCR
>  	description => "Select BIOS implementation.",
>  	default => 'seabios',
>      },
> +    vmgenid => {
> +	type => 'string',
> +	pattern => '(?:auto)|(?:[a-fA-F0-9]{8}(?:-[a-fA-F0-9]{4}){3}-[a-fA-F0-9]{12})',
> +        description => "The VM Generation ID. Use the special value 'auto' to autogenerate one",
> +	optional => 1,
> +    },
>  };
>  
>  my $confdesc_cloudinit = {
> @@ -3191,6 +3198,10 @@ sub config_to_command {
>  	push @$cmd, '-smbios', "type=1,$conf->{smbios1}";
>      }
>  
> +    if ($conf->{vmgenid}) {

regarding this and ... (below)

> +	push @$devices, PVE::QemuServer::VMGenID::create_device($conf->{vmgenid}, 0);
> +    }
> +
>      if ($conf->{bios} && $conf->{bios} eq 'ovmf') {
>  	die "uefi base image not found\n" if ! -f $OVMF_CODE;
>  
> @@ -4593,6 +4604,14 @@ sub vmconfig_apply_pending {
>      }
>  }
>  
> +sub vmconfig_apply_vmgenid_generation {
> +    my ($vmid, $conf) = @_;
> +    if ($conf->{vmgenid}) {
> +	$conf->{vmgenid} = PVE::QemuServer::VMGenID::create_or_parse($conf->{vmgenid});

.. this: why not do the actual work here? This seems to just add indirection
for the sake of it, netto adding more line and rabbit holes to jump in?
(see also below at adding the new module diff)

> +    }
> +    PVE::QemuConfig->write_config($vmid, $conf);
> +}
> +
>  my $safe_num_ne = sub {
>      my ($a, $b) = @_;
>  
> @@ -4779,6 +4798,8 @@ sub vm_start {
>  
>  	die "VM $vmid already running\n" if check_running($vmid, undef, $migratedfrom);
>  
> +	vmconfig_apply_vmgenid_generation($vmid, $conf);
> +
>  	if (!$statefile && scalar(keys %{$conf->{pending}})) {
>  	    vmconfig_apply_pending($vmid, $conf, $storecfg);
>  	    $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> @@ -5554,6 +5575,8 @@ sub restore_update_config_line {
>  	} else {
>  	    print $outfd $line;
>  	}
> +    } elsif (($line =~ m/^(vmgenid: )(.*)/)) {
> +	print $outfd $1.'auto';
>      } elsif (($line =~ m/^(smbios1: )(.*)/) && $unique) {
>  	my ($uuid, $uuid_str);
>  	UUID::generate($uuid);
> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
> index afc39a3..06f359b 100644
> --- a/PVE/QemuServer/Makefile
> +++ b/PVE/QemuServer/Makefile
> @@ -5,6 +5,7 @@ SOURCES=PCI.pm		\
>  	OVF.pm		\
>  	Cloudinit.pm	\
>  	Agent.pm	\
> +	VMGenID.pm	\
>  
>  .PHONY: install
>  install: ${SOURCES}
> diff --git a/PVE/QemuServer/VMGenID.pm b/PVE/QemuServer/VMGenID.pm
> new file mode 100644
> index 0000000..c1aa3e4
> --- /dev/null
> +++ b/PVE/QemuServer/VMGenID.pm
> @@ -0,0 +1,29 @@
> +package PVE::QemuServer::VMGenID;


Not sure if this warrants a new module.
I know you did not want to extend the big chaotic QemuServer module with two
additional methods, which are really good intentions, but adding a specialised
module for only tho methods isn't the direction we want to go neither, at least
I hope so. As having 100s of modules just for the sake of splitting up sound not
ideal either.

We already have a few UUID users (e.g. smbios uuid) which may get grouped together.

Also, you probably can do without this modules adding only two methods in
QemuServer - which honestly seems nicer than one there anyway plus a separate
module for those two (see above).

Don't get me wrong, cleaning up QemuServer to be easier digest, and splitting
related stuff out in a fitting module would be really great, but this approach
seems not bringing us closer to that goal...

> +
> +use strict;
> +use warnings;
> +
> +use UUID;
> +
> +sub create_device {
> +    my ($uuid) = @_;
> +
> +    my $id = create_or_parse($uuid);
> +
> +    return ('-device', "vmgenid,guid=$id");
> +}
> +
> +sub create_or_parse {
> +    my ($uuid) = @_;
> +
> +    my $id;
> +    if ($uuid eq 'auto') {
> +	return UUID::uuid();
> +    } elsif (UUID::parse($uuid, $id) == 0) {
> +	return $uuid;
> +    }
> +
> +    die "invalid VM Generation ID\n";
> +}
> +
> +1;
> 

[0]: https://git.qemu.org/gitweb.cgi?p=qemu.git;a=blob_plain;f=docs/specs/vmgenid.txt;h=aa9f5186767c22698f9615995c19c05d1e85eb39;hb=HEAD