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.

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

Reply via email to