[pve-devel] [PATCH qemu-server 2/4] improve snapshot machine logic

Fabian Grünbichler f.gruenbichler at proxmox.com
Thu Sep 13 12:54:11 CEST 2018


On Thu, Sep 13, 2018 at 12:15:05PM +0200, Dominik Csapak wrote:
> instead of overwriting the 'machine' config in the snapshot,
> use its own 'runningmachine' config only for the snapshot
> 
> this way, we do not lose the machine type if it was
> explicitely set during the snapshot, but deleted afterwards
> 
> we also have to adapt the tests for this

LGTM, small nit inline.

> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/QemuConfig.pm                                  | 26 +++++++++++++---------
>  PVE/QemuServer.pm                                  |  9 +++++++-
>  test/snapshot-expected/create/qemu-server/102.conf |  2 +-
>  test/snapshot-expected/create/qemu-server/104.conf |  2 +-
>  test/snapshot-expected/create/qemu-server/106.conf |  2 +-
>  .../snapshot-expected/prepare/qemu-server/102.conf |  2 +-
>  .../snapshot-expected/prepare/qemu-server/104.conf |  2 +-
>  test/snapshot-test.pm                              |  2 +-
>  8 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 5cc2d48..35ed9b6 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -146,9 +146,7 @@ sub __snapshot_save_vmstate {
>      my $scfg = PVE::Storage::storage_config($storecfg, $target);
>      $name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
>      $snap->{vmstate} = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $name, $size*1024);
> -    # always overwrite machine if we save vmstate. This makes sure we
> -    # can restore it later using correct machine type
> -    $snap->{machine} = PVE::QemuServer::get_current_qemu_machine($vmid);
> +    $snap->{runningmachine} = PVE::QemuServer::get_current_qemu_machine($vmid);
>  }
>  
>  sub __snapshot_check_running {
> @@ -288,13 +286,21 @@ sub __snapshot_rollback_hook {
>  	# we save the machine of the current config
>  	$data->{oldmachine} = $conf->{machine};
>      } else {
> -	# Note: old code did not store 'machine', so we try to be smart
> -	# and guess the snapshot was generated with kvm 1.4 (pc-i440fx-1.4).
> -	$data->{forcemachine} = $conf->{machine} || 'pc-i440fx-1.4';
> -
> -	# we remove the 'machine' configuration if not explicitly specified
> -	# in the original config.
> -	delete $conf->{machine} if $snap->{vmstate} && !defined($data->{oldmachine});
> +	# if we have a 'runningmachine' entry in the snapshot
> +	# we use that for the forcemachine parameter,
> +	# else we use the old logic
> +	if (defined($conf->{runningmachine})) {
> +	    $data->{forcemachine} = $conf->{runningmachine};
> +	    delete $conf->{runningmachine};
> +	} else {
> +	    # Note: old code did not store 'machine', so we try to be smart
> +	    # and guess the snapshot was generated with kvm 1.4 (pc-i440fx-1.4).
> +	    $data->{forcemachine} = $conf->{machine} || 'pc-i440fx-1.4';
> +
> +	    # we remove the 'machine' configuration if not explicitly specified
> +	    # in the original config.
> +	    delete $conf->{machine} if $snap->{vmstate} && !defined($data->{oldmachine});
> +	}
>      }
>  
>      return;
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 015f8f7..a3e602d 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -528,6 +528,13 @@ EODESCR
>  	description => "Default storage for VM state volumes/files.",
>  	optional => 1,
>      }),
> +    runningmachine => {
> +	description => "Specifies the Qemu machine type of the running vm. This is used internally for snapshots.",
> +	type => 'string',
> +	pattern => '(pc|pc(-i440fx)?-\d+\.\d+(\.pxe)?|q35|pc-q35-\d+\.\d+(\.pxe)?)',
> +	maxLength => 40,
> +	optional => 1,

IMHO this should become a standard option and used for all occurences.

> +    },
>      machine => {
>  	description => "Specific the Qemu machine type.",
>  	type => 'string',
> @@ -2255,7 +2262,7 @@ sub json_config_properties {
>      my $prop = shift;
>  
>      foreach my $opt (keys %$confdesc) {
> -	next if $opt eq 'parent' || $opt eq 'snaptime' || $opt eq 'vmstate';
> +	next if $opt eq 'parent' || $opt eq 'snaptime' || $opt eq 'vmstate' || $opt eq 'runningmachine';
>  	$prop->{$opt} = $confdesc->{$opt};
>      }
>  
> diff --git a/test/snapshot-expected/create/qemu-server/102.conf b/test/snapshot-expected/create/qemu-server/102.conf
> index c521154..9b57004 100644
> --- a/test/snapshot-expected/create/qemu-server/102.conf
> +++ b/test/snapshot-expected/create/qemu-server/102.conf
> @@ -20,12 +20,12 @@ bootdisk: ide0
>  cores: 4
>  ide0: local:snapshotable-disk-1,discard=on,size=32G
>  ide2: none,media=cdrom
> -machine: somemachine
>  memory: 8192
>  name: win
>  net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
>  numa: 0
>  ostype: win7
> +runningmachine: somemachine
>  smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
>  snaptime: 1234567890
>  sockets: 1
> diff --git a/test/snapshot-expected/create/qemu-server/104.conf b/test/snapshot-expected/create/qemu-server/104.conf
> index ef7285e..54f1c21 100644
> --- a/test/snapshot-expected/create/qemu-server/104.conf
> +++ b/test/snapshot-expected/create/qemu-server/104.conf
> @@ -39,13 +39,13 @@ bootdisk: ide0
>  cores: 4
>  ide0: local:snapshotable-disk-1,discard=on,size=32G
>  ide2: none,media=cdrom
> -machine: somemachine
>  memory: 8192
>  name: win
>  net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
>  numa: 0
>  ostype: win7
>  parent: test
> +runningmachine: somemachine
>  smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
>  snaptime: 1234567890
>  sockets: 1
> diff --git a/test/snapshot-expected/create/qemu-server/106.conf b/test/snapshot-expected/create/qemu-server/106.conf
> index c521154..9b57004 100644
> --- a/test/snapshot-expected/create/qemu-server/106.conf
> +++ b/test/snapshot-expected/create/qemu-server/106.conf
> @@ -20,12 +20,12 @@ bootdisk: ide0
>  cores: 4
>  ide0: local:snapshotable-disk-1,discard=on,size=32G
>  ide2: none,media=cdrom
> -machine: somemachine
>  memory: 8192
>  name: win
>  net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
>  numa: 0
>  ostype: win7
> +runningmachine: somemachine
>  smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
>  snaptime: 1234567890
>  sockets: 1
> diff --git a/test/snapshot-expected/prepare/qemu-server/102.conf b/test/snapshot-expected/prepare/qemu-server/102.conf
> index 4ab1787..92db74a 100644
> --- a/test/snapshot-expected/prepare/qemu-server/102.conf
> +++ b/test/snapshot-expected/prepare/qemu-server/102.conf
> @@ -18,12 +18,12 @@ bootdisk: ide0
>  cores: 4
>  ide0: somestore:somedisk,discard=on,size=32G
>  ide2: none,media=cdrom
> -machine: somemachine
>  memory: 8192
>  name: win
>  net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
>  numa: 0
>  ostype: win7
> +runningmachine: somemachine
>  smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
>  snapstate: prepare
>  snaptime: 1234567890
> diff --git a/test/snapshot-expected/prepare/qemu-server/104.conf b/test/snapshot-expected/prepare/qemu-server/104.conf
> index b62f2c6..02e2d3c 100644
> --- a/test/snapshot-expected/prepare/qemu-server/104.conf
> +++ b/test/snapshot-expected/prepare/qemu-server/104.conf
> @@ -35,13 +35,13 @@ bootdisk: ide0
>  cores: 4
>  ide0: somestore:somedisk,discard=on,size=32G
>  ide2: none,media=cdrom
> -machine: somemachine
>  memory: 8192
>  name: win
>  net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
>  numa: 0
>  ostype: win7
>  parent: test
> +runningmachine: somemachine
>  smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
>  snapstate: prepare
>  snaptime: 1234567890
> diff --git a/test/snapshot-test.pm b/test/snapshot-test.pm
> index d95d77f..8988942 100644
> --- a/test/snapshot-test.pm
> +++ b/test/snapshot-test.pm
> @@ -295,7 +295,7 @@ sub __snapshot_save_vmstate {
>  
>      my $snap = $conf->{snapshots}->{$snapname};
>      $snap->{vmstate} = "somestorage:state-volume";
> -    $snap->{machine} = "somemachine";
> +    $snap->{runningmachine} = "somemachine"
>  }
>  # END mocked PVE::QemuConfig methods
>  
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel




More information about the pve-devel mailing list