[pve-devel] [PATCH qemu-server 1/2] fix #4201: delete cloud-init disk on rollback

[pve-devel] [PATCH qemu-server 1/2] fix #4201: delete cloud-init disk on rollback

[Mira Limbeck]

If the config doesn't contain the cloud-init disk anymore after the
rollback, we have to clean it up since otherwise no further disk can be
attached unless the one still existing on the storage is deleted.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
 PVE/QemuConfig.pm | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 482c7ab..4a744cc 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -419,6 +419,17 @@ sub __snapshot_rollback_hook {
     if ($prepare) {
 	# we save the machine of the current config
 	$data->{oldmachine} = $conf->{machine};
+
+	# keep info about cloudinit disk in the config before the rollback
+	# will be used to later keep or delete possible leftover cloudinit disks
+	# since cloudinit disks are not part of the snapshots
+	$class->foreach_volume($conf, sub {
+	    my ($ds, $drive) = @_;
+
+	    return if !PVE::QemuServer::drive_is_cloudinit($drive);
+
+	    $data->{cloudinit} = $drive;
+	});
     } else {
 	# if we have a 'runningmachine' entry in the snapshot we use that
 	# for the forcemachine parameter, else we use the old logic
@@ -446,6 +457,29 @@ sub __snapshot_rollback_hook {
 	    # re-initializing its random number generator
 	    $conf->{vmgenid} = PVE::QemuServer::generate_uuid();
 	}
+
+	# config before rollback contained a cloudinit disk
+	# check if that is still the case after the rollback
+	if ($data->{cloudinit}) {
+	    my $found = 0;
+	    $class->foreach_volume($conf, sub {
+		my ($ds, $drive) = @_;
+
+		if (PVE::QemuServer::drive_is_cloudinit($drive)) {
+		    $found = 1;
+		    last;
+		}
+	    });
+
+	    # missing cloudinit disk after rollback
+	    # clean up existing cloudinit disk
+	    if (!$found) {
+                print "removing unreferenced cloud-init disk $data->{cloudinit}->{file}\n";
+
+		my $storecfg = PVE::Storage::config();
+		PVE::Storage::vdisk_free($storecfg, $data->{cloudinit}->{file});
+	    }
+	}
     }
 
     return;
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/2] reuse existing cloud-init disks

[Mira Limbeck]

When a disk exists but is not referenced in the config, it will be
reused instead of dying during the attempt to allocate the disk.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
This patch is not required to fix the rollback code, but might be nice
to have in addition since there will still be some users with cloud-init
disks left on the storage.

 PVE/API2/Qemu.pm | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 3ec31c2..73d6ab9 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -348,15 +348,28 @@ my $create_disks = sub {
 		$fmt = $disk->{format} // "raw";
 	    }
 
-	    # Initial disk created with 4 MB and aligned to 4MB on regeneration
-	    my $ci_size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
-	    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $name, $ci_size/1024);
-	    $disk->{file} = $volid;
+	    # since there was an issue with the rollback code not deleting cloud-init disks
+	    # there can be a case where leftover cloud-init disks are still on the storage.
+	    # since those will be overwritten anyway on each boot, we can just reuse them
+	    # if they already exist
+	    my $reused;
+	    my $ci_disk_found = PVE::Storage::vdisk_list($storecfg, $storeid, $vmid, ["$storeid:$name"], 'images');
+	    if ($ci_disk_found->{$storeid} && scalar($ci_disk_found->{$storeid}->@*)) {
+		print "Cloud-Init disk $name already exists on the storage '$storeid', reusing it.\n";
+		my $ci_disk = $ci_disk_found->{$storeid}[0];
+		$disk->{file} = $ci_disk->{volid};
+		$reused = 1;
+	    } else {
+		# Initial disk created with 4 MB and aligned to 4MB on regeneration
+		my $ci_size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
+		my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $name, $ci_size/1024);
+		$disk->{file} = $volid;
+	    }
 	    $disk->{media} = 'cdrom';
 	    push @$vollist, $volid;
 	    delete $disk->{format}; # no longer needed
 	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
-	    print "$ds: successfully created disk '$res->{$ds}'\n";
+	    print "$ds: successfully created disk '$res->{$ds}'\n" if !$reused;
 	} elsif ($volid =~ $NEW_DISK_RE) {
 	    my ($storeid, $size) = ($2 || $default_storage, $3);
 	    die "no storage ID specified (and no default storage)\n" if !$storeid;
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 2/2] reuse existing cloud-init disks

[Fiona Ebner]

Am 29.09.22 um 15:36 schrieb Mira Limbeck:
> When a disk exists but is not referenced in the config, it will be
> reused instead of dying during the attempt to allocate the disk.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> This patch is not required to fix the rollback code, but might be nice
> to have in addition since there will still be some users with cloud-init
> disks left on the storage.
> 

Nothing wrong with the patch AFAICT, just not fully convinced that it's
worth the added complexity. The $create_disks sub is already a bit
unwieldy unfortunately. So I'm fine with and without this patch :)

Re: [pve-devel] [PATCH qemu-server 1/2] fix #4201: delete cloud-init disk on rollback

[Fiona Ebner]

Am 29.09.22 um 15:36 schrieb Mira Limbeck:
> If the config doesn't contain the cloud-init disk anymore after the
> rollback, we have to clean it up since otherwise no further disk can be
> attached unless the one still existing on the storage is deleted.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
>  PVE/QemuConfig.pm | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 482c7ab..4a744cc 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -419,6 +419,17 @@ sub __snapshot_rollback_hook {
>      if ($prepare) {
>  	# we save the machine of the current config
>  	$data->{oldmachine} = $conf->{machine};
> +
> +	# keep info about cloudinit disk in the config before the rollback
> +	# will be used to later keep or delete possible leftover cloudinit disks
> +	# since cloudinit disks are not part of the snapshots
> +	$class->foreach_volume($conf, sub {
> +	    my ($ds, $drive) = @_;
> +
> +	    return if !PVE::QemuServer::drive_is_cloudinit($drive);
> +
> +	    $data->{cloudinit} = $drive;
> +	});

You could re-use the has_cloudinit() helper here (not passing any $skip
parameter) ;)

>      } else {
>  	# if we have a 'runningmachine' entry in the snapshot we use that
>  	# for the forcemachine parameter, else we use the old logic
> @@ -446,6 +457,29 @@ sub __snapshot_rollback_hook {
>  	    # re-initializing its random number generator
>  	    $conf->{vmgenid} = PVE::QemuServer::generate_uuid();
>  	}
> +
> +	# config before rollback contained a cloudinit disk
> +	# check if that is still the case after the rollback
> +	if ($data->{cloudinit}) {
> +	    my $found = 0;
> +	    $class->foreach_volume($conf, sub {
> +		my ($ds, $drive) = @_;
> +
> +		if (PVE::QemuServer::drive_is_cloudinit($drive)) {
> +		    $found = 1;
> +		    last;

We're not in a loop here (at least not a native Perl loop), but in a
'sub', so 'last' is out of place and results in a warning:
Exiting subroutine via last at /usr/share/perl5/PVE/QemuConfig.pm line 470.

Could also re-use the helper.

> +		}
> +	    });
> +
> +	    # missing cloudinit disk after rollback
> +	    # clean up existing cloudinit disk

This is missing the case where the storage of the cloud-init disk
changed. It still needs to be freed then.

If you wanted, you could also leverage the existing logic for
unreferenced disks upon rollback:
Namely, change __snapshot_rollback_get_unused() to also return the
cloudinit disk (if it is unused), and overwrite the add_unused_volume()
implementation here, freeing any cloud-init disk that comes along and
calling the parent implementation for all other disks.

Might be a bit more future-proof, but your approach is also fine.

> +	    if (!$found) {
> +                print "removing unreferenced cloud-init disk $data->{cloudinit}->{file}\n";
> +
> +		my $storecfg = PVE::Storage::config();
> +		PVE::Storage::vdisk_free($storecfg, $data->{cloudinit}->{file});
> +	    }
> +	}
>      }
>  
>      return;

Re: [pve-devel] [PATCH qemu-server 1/2] fix #4201: delete cloud-init disk on rollback

[Mira Limbeck]

On 9/30/22 10:21, Fiona Ebner wrote:
> Am 29.09.22 um 15:36 schrieb Mira Limbeck:
>> If the config doesn't contain the cloud-init disk anymore after the
>> rollback, we have to clean it up since otherwise no further disk can be
>> attached unless the one still existing on the storage is deleted.
>>
>> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
>> ---
>>   PVE/QemuConfig.pm | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
>> index 482c7ab..4a744cc 100644
>> --- a/PVE/QemuConfig.pm
>> +++ b/PVE/QemuConfig.pm
>> @@ -419,6 +419,17 @@ sub __snapshot_rollback_hook {
>>       if ($prepare) {
>>   	# we save the machine of the current config
>>   	$data->{oldmachine} = $conf->{machine};
>> +
>> +	# keep info about cloudinit disk in the config before the rollback
>> +	# will be used to later keep or delete possible leftover cloudinit disks
>> +	# since cloudinit disks are not part of the snapshots
>> +	$class->foreach_volume($conf, sub {
>> +	    my ($ds, $drive) = @_;
>> +
>> +	    return if !PVE::QemuServer::drive_is_cloudinit($drive);
>> +
>> +	    $data->{cloudinit} = $drive;
>> +	});
> You could re-use the has_cloudinit() helper here (not passing any $skip
> parameter) ;)
Ah yes, I didn't revisit this patch after talking to you about that. 
Thanks for the suggestion!
>>       } else {
>>   	# if we have a 'runningmachine' entry in the snapshot we use that
>>   	# for the forcemachine parameter, else we use the old logic
>> @@ -446,6 +457,29 @@ sub __snapshot_rollback_hook {
>>   	    # re-initializing its random number generator
>>   	    $conf->{vmgenid} = PVE::QemuServer::generate_uuid();
>>   	}
>> +
>> +	# config before rollback contained a cloudinit disk
>> +	# check if that is still the case after the rollback
>> +	if ($data->{cloudinit}) {
>> +	    my $found = 0;
>> +	    $class->foreach_volume($conf, sub {
>> +		my ($ds, $drive) = @_;
>> +
>> +		if (PVE::QemuServer::drive_is_cloudinit($drive)) {
>> +		    $found = 1;
>> +		    last;
> We're not in a loop here (at least not a native Perl loop), but in a
> 'sub', so 'last' is out of place and results in a warning:
> Exiting subroutine via last at /usr/share/perl5/PVE/QemuConfig.pm line 470.
>
> Could also re-use the helper.
Yes, will definitely use the helper.
>> +		}
>> +	    });
>> +
>> +	    # missing cloudinit disk after rollback
>> +	    # clean up existing cloudinit disk
> This is missing the case where the storage of the cloud-init disk
> changed. It still needs to be freed then.
>
> If you wanted, you could also leverage the existing logic for
> unreferenced disks upon rollback:
> Namely, change __snapshot_rollback_get_unused() to also return the
> cloudinit disk (if it is unused), and overwrite the add_unused_volume()
> implementation here, freeing any cloud-init disk that comes along and
> calling the parent implementation for all other disks.
>
> Might be a bit more future-proof, but your approach is also fine.
I'll take a look at doing it that way.
>> +	    if (!$found) {
>> +                print "removing unreferenced cloud-init disk $data->{cloudinit}->{file}\n";
>> +
>> +		my $storecfg = PVE::Storage::config();
>> +		PVE::Storage::vdisk_free($storecfg, $data->{cloudinit}->{file});
>> +	    }
>> +	}
>>       }
>>   
>>       return;