From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 6352A1FF2AD for ; Thu, 4 Jul 2024 12:16:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 72EB23021A; Thu, 4 Jul 2024 12:16:38 +0200 (CEST) MIME-Version: 1.0 In-Reply-To: <20240529114519.2419494-1-d.csapak@proxmox.com> References: <20240529114519.2419494-1-d.csapak@proxmox.com> From: Fabian =?utf-8?q?Gr=C3=BCnbichler?= To: Proxmox VE development discussion Date: Thu, 04 Jul 2024 12:15:58 +0200 Message-ID: <172008815851.63060.5652157369713340865@yuna.proxmox.com> User-Agent: alot/0.10 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.050 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH guest-common] abstract config: snapshot create/rollback/remove: print more log statements X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Quoting Dominik Csapak (2024-05-29 13:45:19) > this increases verbosity of the actions, especially when including the > snapshot name, since that will appear in the task logs wereas before > there was no mention of any action, just the storage specific output for > creating/rollback/removal. > > Logs are printed at all main actions that can fail or take potentially > long, so that it's clear what started/finished. > > Signed-off-by: Dominik Csapak > --- > replaces https://lists.proxmox.com/pipermail/pve-devel/2024-May/064010.html > > src/PVE/AbstractConfig.pm | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm > index 5d5f9b4..b685b7f 100644 > --- a/src/PVE/AbstractConfig.pm > +++ b/src/PVE/AbstractConfig.pm > @@ -806,6 +806,8 @@ sub __snapshot_activate_storages { > sub snapshot_create { > my ($class, $vmid, $snapname, $save_vmstate, $comment) = @_; > > + print "Creating snapshot '$snapname'\n"; > + > my $snap = $class->__snapshot_prepare($vmid, $snapname, $save_vmstate, $comment); > > $save_vmstate = 0 if !$snap->{vmstate}; > @@ -820,24 +822,30 @@ sub snapshot_create { > $class->__snapshot_activate_storages($conf, 0); > > if ($freezefs) { > + print "Trying to freeze guest filesystems\n"; > $class->__snapshot_freeze($vmid, 0); > + print "Succesfully frozen guest filesystems\n"; > } > > $class->__snapshot_create_vol_snapshots_hook($vmid, $snap, $running, "before"); > > + print "Creating volume snapshots\n"; > $class->foreach_volume($snap, sub { > my ($vs, $volume) = @_; > > $class->__snapshot_create_vol_snapshot($vmid, $vs, $volume, $snapname); > $drivehash->{$vs} = 1; > }); > + print "Succesfully created volume snapshots\n"; > }; > my $err = $@; > > if ($running) { > $class->__snapshot_create_vol_snapshots_hook($vmid, $snap, $running, "after"); > if ($freezefs) { > + print "Trying to thaw guest filesystems\n"; > $class->__snapshot_freeze($vmid, 1); > + print "Succesfully thawed guest filesystems\n"; > } > $class->__snapshot_create_vol_snapshots_hook($vmid, $snap, $running, "after-unfreeze"); > } > @@ -850,6 +858,8 @@ sub snapshot_create { > } > > $class->__snapshot_commit($vmid, $snapname); > + > + print "Succesfully created snapshot '$snapname'\n"; > } > > # Check if the snapshot might still be needed by a replication job. > @@ -895,6 +905,8 @@ my $snapshot_delete_assert_not_needed_by_replication = sub { > sub snapshot_delete { > my ($class, $vmid, $snapname, $force, $drivehash) = @_; > > + print "Removing snapshot '$snapname'\n"; > + > my $unused = []; > > my $conf = $class->load_config($vmid); > @@ -963,12 +975,15 @@ sub snapshot_delete { > > # now remove vmstate file > if ($snap->{vmstate}) { > + print "Removing vmstate file\n"; > $class->__snapshot_delete_vmstate_file($snap, $force); > > # save changes (remove vmstate from snapshot) > $class->lock_config($vmid, $remove_drive, 'vmstate') if !$force; > + print "Succesfully removed vmstate file\n"; > }; > > + print "Removing volume snapshots\n"; > # now remove all volume snapshots > $class->foreach_volume($snap, sub { > my ($vs, $volume) = @_; > @@ -985,6 +1000,7 @@ sub snapshot_delete { > # save changes (remove drive from snapshot) > $class->lock_config($vmid, $remove_drive, $vs) if !$force; > }); > + print "Succesfully removed volume snapshots\n"; > > # now cleanup config > $class->lock_config($vmid, sub { > @@ -1010,6 +1026,8 @@ sub snapshot_delete { > > $class->write_config($vmid, $conf); > }); > + > + print "Succesfully removed snapshot '$snapname'\n"; > } > > # Remove replication snapshots to make a rollback possible. > @@ -1082,6 +1100,8 @@ my $rollback_remove_replication_snapshots = sub { > sub snapshot_rollback { > my ($class, $vmid, $snapname) = @_; > > + print "Rolling back to snapshot '$snapname'\n"; > + > my $prepare = 1; > > my $data = {}; > @@ -1121,6 +1141,7 @@ sub snapshot_rollback { > > if ($prepare) { > $class->check_lock($conf); > + print "Stopping guest\n"; nit: this can be a bit misleading, since it is also printed if the guest isn't running.. for containers, the implementation of this is guarded by check_running, for VMs it basically is as well (since this only calls vm_stop, which returns immediately if the VM is not running). should we just add that check to the if condition here? and then drop it from LXC::Config, since it's redundant then? > $class->__snapshot_rollback_vm_stop($vmid); > } > > @@ -1149,20 +1170,25 @@ sub snapshot_rollback { > $class->write_config($vmid, $conf); > > if (!$prepare && $snap->{vmstate}) { > + print "Starting guest\n"; > $class->__snapshot_rollback_vm_start($vmid, $snap->{vmstate}, $data); > } > }; > > $class->lock_config($vmid, $updatefn); > > + print "Rolling back volume snapshots\n"; > $class->foreach_volume($snap, sub { > my ($vs, $volume) = @_; > > $class->__snapshot_rollback_vol_rollback($volume, $snapname); > }); > + print "Succesfully rolled back volume snapshots\n"; > > $prepare = 0; > $class->lock_config($vmid, $updatefn); > + > + print "Succesfully rolled back to snapshot '$snapname'\n"; > } > > # Calculate a derived property from a configuration. Derived properties are: > -- > 2.39.2 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel