[pve-devel] [PATCH guest-common] abstract config: snapshot create/rollback/remove: print more log statements

[Fabian Grünbichler]

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 <d.csapak at proxmox.com>
> ---
> 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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>