On 11/11/22 15:02, Stefan Hanreich wrote: > > On 11/11/22 14:58, Daniel Tschlatscher wrote: >> The new hookscript example works nicely out of the box. >> >> I tested restore for both VMs and containers via the GUI, the restore >> and create commands in the respective CLI commands and with the API. >> >> One thing which might some more consideration: >> When restoring a backup that does not configure a hookscript, the >> 'pre-restore' hook will run, however, the 'post-restore' will not. This >> was very confusing at first. >> Similarly, if the config does not include a hookscript, but the backup >> does, then the 'pre-restore' will not run but the 'post-restore' will. >> While this is not breaking, it is definitely very unexpected for an >> unsuspecting user. > > Yes, it might be smarter to use the old config for both pre/post-restore > and not mix both configurations I think.
+1 for not mixing the configurations Based on our discussion off-list: I feel like it might be even better to make this setting config-version agnostic. Or at least, to not overwrite/include it when making or restoring backups, and to keep it the same each time. At least that feels like the way I would expect it to work intuitively. > > Because of this and the minor issues with the example hookscript I will > create a v3. Some input on whether to use the old configuration for both > pre/post-restore or not would be much appreciated > > Ty for the review! > >> >> Otherwise, the core part of the series works as intended, therefore: >> >> Tested-by: Daniel Tschlatscher <d.tschlatsc...@proxmox.com> >> >> >> On 11/10/22 16:33, Stefan Hanreich wrote: >>> This patch adds hooks that run when the user restores a backup from >>> the Web UI >>> / CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are >>> there any >>> other places where the hook should get triggered that I missed? >>> >>> Changes compared to v1: >>> - slightly moved the call site of the exec_hookscript in qemu-server and >>> pve-container, so necessary checks are run before the hookscript runs. >>> >>> >>> pve-container: >>> >>> Stefan Hanreich (1): >>> Add pre/post-restore hooks to CTs >>> >>> src/PVE/API2/LXC.pm | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> >>> pve-docs: >>> >>> Stefan Hanreich (1): >>> add pre/post-restore events to example hookscript >>> >>> examples/guest-example-hookscript.pl | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> >>> qemu-server: >>> >>> Stefan Hanreich (1): >>> Add pre/post-restore hooks to VMs >>> >>> PVE/API2/Qemu.pm | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >> >> _______________________________________________ >> 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
