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

[Fiona Ebner <f.ebner@proxmox.com>]

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;