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

[Mira Limbeck <m.limbeck@proxmox.com>]

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;