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