[pve-devel] [PATCH v2 qemu-server 1/2] fix VM clone from snapshot with cloudinit disk

[pve-devel] [PATCH v2 qemu-server 1/2] fix VM clone from snapshot with cloudinit disk

[Mira Limbeck]

All volumes contained in $vollist are activated. In this case a snapshot
of the volume. For cloudinit disks no snapshots are created so don't add
it to the list of volumes to activate as it otherwise fails with no
logical volume found.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
v2: unchanged

 PVE/API2/Qemu.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8da616a..8e1bbc2 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3000,6 +3000,7 @@ __PACKAGE__->register_method({
 				if !PVE::Storage::volume_has_feature($storecfg, 'clone', $drive->{file}, $snapname, $running);
 			}
 			$drives->{$opt} = $drive;
+			next if PVE::QemuServer::drive_is_cloudinit($drive);
 			push @$vollist, $drive->{file};
 		    }
 		} else {
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 2/2] fix clone_disk failing for nonexistent cloudinit disk

[Mira Limbeck]

After migration or a rollback the cloudinit disk might not be allocated, so
volume_size_info() fails. As we override the value anyway for cloudinit
and efi disks simply move the volume_size_info() call into the 'else'
branch.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
v2: changed subject

 PVE/QemuServer.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 2747c66..49765b7 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6895,10 +6895,10 @@ sub clone_disk {
 	$storeid = $storage if $storage;
 
 	my $dst_format = resolve_dst_disk_format($storecfg, $storeid, $volname, $format);
-	my ($size) = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 3);
 
 	print "create full clone of drive $drivename ($drive->{file})\n";
 	my $name = undef;
+	my $size = undef;
 	if (drive_is_cloudinit($drive)) {
 	    $name = "vm-$newvmid-cloudinit";
 	    $name .= ".$dst_format" if $dst_format ne 'raw';
@@ -6906,6 +6906,8 @@ sub clone_disk {
 	    $size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
 	} elsif ($drivename eq 'efidisk0') {
 	    $size = get_efivars_size($conf);
+	} else {
+	    ($size) = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 3);
 	}
 	$size /= 1024;
 	$newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $newvmid, $dst_format, $name, $size);
-- 
2.20.1

Re: [pve-devel] [PATCH v2 qemu-server 2/2] fix clone_disk failing for nonexistent cloudinit disk

[Thomas Lamprecht]

On 28.09.20 10:36, Mira Limbeck wrote:
> After migration or a rollback the cloudinit disk might not be allocated, so
> volume_size_info() fails. As we override the value anyway for cloudinit
> and efi disks simply move the volume_size_info() call into the 'else'
> branch.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> v2: changed subject
> 
>  PVE/QemuServer.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 2747c66..49765b7 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6895,10 +6895,10 @@ sub clone_disk {
>  	$storeid = $storage if $storage;
>  
>  	my $dst_format = resolve_dst_disk_format($storecfg, $storeid, $volname, $format);
> -	my ($size) = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 3);
>  
>  	print "create full clone of drive $drivename ($drive->{file})\n";
>  	my $name = undef;
> +	my $size = undef;
>  	if (drive_is_cloudinit($drive)) {
>  	    $name = "vm-$newvmid-cloudinit";
>  	    $name .= ".$dst_format" if $dst_format ne 'raw';
> @@ -6906,6 +6906,8 @@ sub clone_disk {
>  	    $size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
>  	} elsif ($drivename eq 'efidisk0') {
>  	    $size = get_efivars_size($conf);
> +	} else {
> +	    ($size) = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 3);
>  	}
>  	$size /= 1024;

doesn't this logs a "use of undefined value in division" or something like that
somewhere in the non-else case?

>  	$newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $newvmid, $dst_format, $name, $size);
> 

Re: [pve-devel] [PATCH v2 qemu-server 2/2] fix clone_disk failing for nonexistent cloudinit disk

[Mira Limbeck]

On 10/5/20 5:35 PM, Thomas Lamprecht wrote:
> On 28.09.20 10:36, Mira Limbeck wrote:
>> After migration or a rollback the cloudinit disk might not be allocated, so
>> volume_size_info() fails. As we override the value anyway for cloudinit
>> and efi disks simply move the volume_size_info() call into the 'else'
>> branch.
>>
>> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
>> ---
>> v2: changed subject
>>
>>   PVE/QemuServer.pm | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 2747c66..49765b7 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -6895,10 +6895,10 @@ sub clone_disk {
>>   	$storeid = $storage if $storage;
>>   
>>   	my $dst_format = resolve_dst_disk_format($storecfg, $storeid, $volname, $format);
>> -	my ($size) = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 3);
>>   
>>   	print "create full clone of drive $drivename ($drive->{file})\n";
>>   	my $name = undef;
>> +	my $size = undef;
>>   	if (drive_is_cloudinit($drive)) {
>>   	    $name = "vm-$newvmid-cloudinit";
>>   	    $name .= ".$dst_format" if $dst_format ne 'raw';
>> @@ -6906,6 +6906,8 @@ sub clone_disk {
>>   	    $size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
>>   	} elsif ($drivename eq 'efidisk0') {
>>   	    $size = get_efivars_size($conf);
>> +	} else {
>> +	    ($size) = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 3);
>>   	}
>>   	$size /= 1024;
> doesn't this logs a "use of undefined value in division" or something like that
> somewhere in the non-else case?
No, in the cloudinit case we set it to a constant. In the efidisk case 
we call get_efivars_size() which dies if efivars is not a file, 
otherwise we get a size (-s). And in the else case we also die if we 
can't get the size. So size is set in every case before the first use.
>
>>   	$newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $newvmid, $dst_format, $name, $size);
>>

Re: [pve-devel] [PATCH v2 qemu-server 2/2] fix clone_disk failing for nonexistent cloudinit disk

[Mira Limbeck]

On 10/6/20 10:56 AM, Mira Limbeck wrote:
>
> On 10/5/20 5:35 PM, Thomas Lamprecht wrote:
>> On 28.09.20 10:36, Mira Limbeck wrote:
>>> After migration or a rollback the cloudinit disk might not be 
>>> allocated, so
>>> volume_size_info() fails. As we override the value anyway for cloudinit
>>> and efi disks simply move the volume_size_info() call into the 'else'
>>> branch.
>>>
>>> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
>>> ---
>>> v2: changed subject
>>>
>>>   PVE/QemuServer.pm | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>>> index 2747c66..49765b7 100644
>>> --- a/PVE/QemuServer.pm
>>> +++ b/PVE/QemuServer.pm
>>> @@ -6895,10 +6895,10 @@ sub clone_disk {
>>>       $storeid = $storage if $storage;
>>>         my $dst_format = resolve_dst_disk_format($storecfg, 
>>> $storeid, $volname, $format);
>>> -    my ($size) = PVE::Storage::volume_size_info($storecfg, 
>>> $drive->{file}, 3);
>>>         print "create full clone of drive $drivename 
>>> ($drive->{file})\n";
>>>       my $name = undef;
>>> +    my $size = undef;
>>>       if (drive_is_cloudinit($drive)) {
>>>           $name = "vm-$newvmid-cloudinit";
>>>           $name .= ".$dst_format" if $dst_format ne 'raw';
>>> @@ -6906,6 +6906,8 @@ sub clone_disk {
>>>           $size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
>>>       } elsif ($drivename eq 'efidisk0') {
>>>           $size = get_efivars_size($conf);
>>> +    } else {
>>> +        ($size) = PVE::Storage::volume_size_info($storecfg, 
>>> $drive->{file}, 3);
>>>       }
>>>       $size /= 1024;
>> doesn't this logs a "use of undefined value in division" or something 
>> like that
>> somewhere in the non-else case?
> No, in the cloudinit case we set it to a constant. In the efidisk case 
> we call get_efivars_size() which dies if efivars is not a file, 
> otherwise we get a size (-s). And in the else case we also die if we 
> can't get the size. So size is set in every case before the first use.
To clarify, the returned size in volume_size_info can be undefined, but 
not the other 2 cases. Would a die be a good idea in case the 
volume_size_info call returns 'undef'?
>>
>>>       $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, 
>>> $newvmid, $dst_format, $name, $size);
>>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

[pve-devel] applied: [PATCH v2 qemu-server 1/2] fix VM clone from snapshot with cloudinit disk

[Thomas Lamprecht]

On 28.09.20 10:36, Mira Limbeck wrote:
> All volumes contained in $vollist are activated. In this case a snapshot
> of the volume. For cloudinit disks no snapshots are created so don't add
> it to the list of volumes to activate as it otherwise fails with no
> logical volume found.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> v2: unchanged
> 
>  PVE/API2/Qemu.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 qemu-server 2/2] fix clone_disk failing for nonexistent cloudinit disk

[Thomas Lamprecht]

On 28.09.20 10:36, Mira Limbeck wrote:
> After migration or a rollback the cloudinit disk might not be allocated, so
> volume_size_info() fails. As we override the value anyway for cloudinit
> and efi disks simply move the volume_size_info() call into the 'else'
> branch.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> v2: changed subject
> 
>  PVE/QemuServer.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH v2 qemu-server 2/2] fix clone_disk failing for nonexistent cloudinit disk

[Thomas Lamprecht]

On 06.10.20 13:10, Mira Limbeck wrote:
> On 10/6/20 10:56 AM, Mira Limbeck wrote:
>> On 10/5/20 5:35 PM, Thomas Lamprecht wrote:
>>> On 28.09.20 10:36, Mira Limbeck wrote:
>>>> @@ -6906,6 +6906,8 @@ sub clone_disk {
>>>>           $size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
>>>>       } elsif ($drivename eq 'efidisk0') {
>>>>           $size = get_efivars_size($conf);
>>>> +    } else {
>>>> +        ($size) = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 3);
>>>>       }
>>>>       $size /= 1024;
>>> doesn't this logs a "use of undefined value in division" or something like that
>>> somewhere in the non-else case?
>> No, in the cloudinit case we set it to a constant. In the efidisk case we call get_efivars_size() which dies if efivars is not a file, otherwise we get a size (-s). And in the else case we also die if we can't get the size. So size is set in every case before the first use.
> To clarify, the returned size in volume_size_info can be undefined, but not the other 2 cases. Would a die be a good idea in case the volume_size_info call returns 'undef'?

could be, could be possible to assert that in vdisk_alloc, as there
it rather seems that it's assumed to never be undef; but I just checked
the base and ZFS Plugin, so..