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.limb...@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;
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
