[pve-devel] [PATCH pve-container/qemu-server/pve-guest-common/pve-docs 0/1] Add pre/post-migrate hooks

[pve-devel] [PATCH pve-container/qemu-server/pve-guest-common/pve-docs 0/1] Add pre/post-migrate hooks

[Stefan Hanreich]

This patch adds pre/post-migrate hooks when the when the user migrates a CT/VM
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?

I have decided to create distinct event types for source/target nodes, since
otherwise the same script would run essentially twice on the source/target node.
With distinct event types, the hooks should be more flexible in their usage.

In my first try I simply ssh'ed into the target nodes and ran a perl script
hardcoded as string via perl -e. This seemed very dirty though, which is why I
have opted to move those parts into the respective binaries qm/pct. There are
other subcommands already that fulfill a similar purpose (qm mtunnel) which is
why I figured this might be the better way to implement this. Then I simply
need to call the respective subcommand in the hook methods. Would it be better
to name the subcommands pre/post-migrate-target?

I have added abstract methods to the abstract class, which I implement in the
respective backends. Those methods are virtually the same, only the binary that
gets executed on the target node is different in the backends (pct vs qm). It
might make sense to move both methods to the abstract class and parametrize the
string containing the names of the binaries. I have decided against this for
now, since usually such methods end up diverging anyway. Still, it might make
sense to move the implementation up to the parent class and only have very
simple methods in the specific backends that return the necessary name of the
binary. What do you think?

pve-guest-common:

Stefan Hanreich (1):
  Add abstract methods for pre/post-migrate hooks

 src/PVE/AbstractMigrate.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)


pve-container:

Stefan Hanreich (1):
  Add CT hooks for pre/post-migrate on target/source

 src/PVE/CLI/pct.pm     | 51 ++++++++++++++++++++++++++++++++++++++++++
 src/PVE/LXC/Migrate.pm | 23 +++++++++++++++++++
 2 files changed, 74 insertions(+)


qemu-server:

Stefan Hanreich (1):
  Add VM hooks for pre/post-migrate on target/source

 PVE/CLI/qm.pm                         | 50 +++++++++++++++++++++++++++
 PVE/QemuMigrate.pm                    | 23 ++++++++++++
 test/MigrationTest/QemuMigrateMock.pm |  4 +++
 3 files changed, 77 insertions(+)


pve-docs:

Stefan Hanreich (1):
  Add pre/post-migrate events for target and source to example
    hookscript

 examples/guest-example-hookscript.pl | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
-- 
2.30.2

[pve-devel] [PATCH pve-guest-common 1/1] Add abstract methods for pre/post-migrate hooks

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/AbstractMigrate.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/PVE/AbstractMigrate.pm b/src/PVE/AbstractMigrate.pm
index d90e5b7..5e03488 100644
--- a/src/PVE/AbstractMigrate.pm
+++ b/src/PVE/AbstractMigrate.pm
@@ -178,6 +178,8 @@ sub migrate {
 	        "public key authentication\n" if $@;
 	}
 
+	$self->pre_migration_hooks($self->{vmid});
+
 	&$eval_int($self, sub { $self->phase1($self->{vmid}); });
 	my $err = $@;
 	if ($err) {
@@ -228,6 +230,8 @@ sub migrate {
 	    $self->log('err', $err);
 	    $self->{errors} = 1;
 	}
+
+	$self->post_migration_hooks($self->{vmid});
     })};
 
     my $err = $@;
@@ -368,4 +372,14 @@ sub get_bwlimit {
     return $bwlimit;
 }
 
+sub pre_migration_hooks {
+    my ($self, $vmid) = @_;
+    die "abstract method - implement me";
+}
+
+sub post_migration_hooks {
+    my ($self, $vmid) = @_;
+    die "abstract method - implement me";
+}
+
 1;
-- 
2.30.2

[pve-devel] [PATCH pve-container 1/1] Add CT hooks for pre/post-migrate on target/source

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/CLI/pct.pm     | 51 ++++++++++++++++++++++++++++++++++++++++++
 src/PVE/LXC/Migrate.pm | 23 +++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index 23793ee..e80586d 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -803,6 +803,54 @@ __PACKAGE__->register_method ({
 	return undef;
     }});
 
+__PACKAGE__->register_method ({
+    name => 'pre_migrate',
+    path => 'pre_migrate',
+    method => 'POST',
+    description => "Run pre-migrate hook for VM <vmid> - do not use manually.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
+	    'source-node' => get_standard_option('pve-node'),
+	},
+    },
+    returns => { type => 'null'},
+    code => sub {
+	my ($param) = @_;
+
+	my $vmid = $param->{vmid};
+	my $source_node = $param->{'source-node'};
+	my $conf = PVE::LXC::Config->load_config($vmid, $source_node);
+
+	PVE::GuestHelpers::exec_hookscript($conf, $vmid, "pre-migrate-target", 1);
+
+	return;
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'post_migrate',
+    path => 'post_migrate',
+    method => 'POST',
+    description => "Run post-migrate hook for VM <vmid> - do not use manually.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
+	},
+    },
+    returns => { type => 'null'},
+    code => sub {
+	my ($param) = @_;
+
+	my $vmid = $param->{vmid};
+	my $conf = PVE::LXC::Config->load_config($vmid);
+
+	PVE::GuestHelpers::exec_hookscript($conf, $vmid, "post-migrate-target");
+
+	return;
+    }});
+
 our $cmddef = {
     list=> [ 'PVE::API2::LXC', 'vmlist', [], { node => $nodename }, sub {
 	my $res = shift;
@@ -874,6 +922,9 @@ our $cmddef = {
     rescan  => [ __PACKAGE__, 'rescan', []],
     cpusets => [ __PACKAGE__, 'cpusets', []],
     fstrim => [ __PACKAGE__, 'fstrim', ['vmid']],
+
+    'pre-migrate' => [ __PACKAGE__, 'pre_migrate', ['vmid', 'source-node']],
+    'post-migrate' => [ __PACKAGE__, 'post_migrate', ['vmid']],
 };
 
 1;
diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index 2ef1cce..4089c2c 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -416,4 +416,27 @@ sub final_cleanup {
 
 }
 
+sub pre_migration_hooks {
+    my ($self, $vmid) = @_;
+
+    PVE::GuestHelpers::exec_hookscript($self->{vmconf}, $vmid, 'pre-migrate-source', 1);
+
+    my $node = PVE::INotify::nodename();
+    my $cmd = [ @{$self->{rem_ssh}}, "pct", "pre-migrate", $vmid, $node ];
+
+    eval { $self->cmd($cmd); };
+    die "$@\n" if $@;
+}
+
+sub post_migration_hooks {
+    my ($self, $vmid) = @_;
+
+    PVE::GuestHelpers::exec_hookscript($self->{vmconf}, $vmid, 'post-migrate-source');
+
+    my $cmd = [ @{$self->{rem_ssh}}, "pct", "post-migrate", $vmid ];
+
+    eval { $self->cmd($cmd); };
+    die "$@\n" if $@;
+}
+
 1;
-- 
2.30.2

[pve-devel] [PATCH pve-docs 1/1] Add pre/post-migrate events for target and source to example hookscript

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 examples/guest-example-hookscript.pl | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/examples/guest-example-hookscript.pl b/examples/guest-example-hookscript.pl
index adeed59..e4adf6d 100755
--- a/examples/guest-example-hookscript.pl
+++ b/examples/guest-example-hookscript.pl
@@ -54,6 +54,34 @@ if ($phase eq 'pre-start') {
 
     print "$vmid stopped. Doing cleanup.\n";
 
+} elsif ($phase eq 'pre-migrate-source') {
+
+    # Phase 'pre-migrate-source' will be run before a migration on the source
+    # node of the VM/CT
+
+    print "preparing $vmid for migration on source.\n";
+
+} elsif ($phase eq 'post-migrate-source') {
+
+    # Phase 'post-migrate-source' will be run after a migration on the source
+    # node of the VM/CT
+
+    print "$vmid finished migrating on source.\n";
+
+} elsif ($phase eq 'pre-migrate-target') {
+
+    # Phase 'pre-migrate-target' will be run before a migration on the target
+    # node of the VM/CT
+
+    print "preparing $vmid for migration on target.\n";
+
+} elsif ($phase eq 'post-migrate-target') {
+
+    # Phase 'post-migrate-target' will be run after a migration on the target
+    # node of the VM/CT
+
+    print "$vmid finished migrating on target.\n";
+
 } else {
     die "got unknown phase '$phase'\n";
 }
-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/1] Add VM hooks for pre/post-migrate on target/source

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 PVE/CLI/qm.pm                         | 50 +++++++++++++++++++++++++++
 PVE/QemuMigrate.pm                    | 23 ++++++++++++
 test/MigrationTest/QemuMigrateMock.pm |  4 +++
 3 files changed, 77 insertions(+)

diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 6a2e161..4161e6e 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -838,6 +838,54 @@ __PACKAGE__->register_method({
 	return;
     }});
 
+__PACKAGE__->register_method ({
+    name => 'pre_migrate',
+    path => 'pre_migrate',
+    method => 'POST',
+    description => "Run pre-migrate hook for VM <vmid> - do not use manually.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
+	    'source-node' => get_standard_option('pve-node'),
+	},
+    },
+    returns => { type => 'null'},
+    code => sub {
+	my ($param) = @_;
+
+	my $vmid = $param->{vmid};
+	my $source_node = $param->{'source-node'};
+	my $conf = PVE::QemuConfig->load_config($vmid, $source_node);
+
+	PVE::GuestHelpers::exec_hookscript($conf, $vmid, "pre-migrate-target", 1);
+
+	return;
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'post_migrate',
+    path => 'post_migrate',
+    method => 'POST',
+    description => "Run post-migrate hook for VM <vmid> - do not use manually.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
+	},
+    },
+    returns => { type => 'null'},
+    code => sub {
+	my ($param) = @_;
+
+	my $vmid = $param->{vmid};
+	my $conf = PVE::QemuConfig->load_config($vmid);
+
+	PVE::GuestHelpers::exec_hookscript($conf, $vmid, "post-migrate-target");
+
+	return;
+    }});
+
 my $print_agent_result = sub {
     my ($data) = @_;
 
@@ -988,6 +1036,8 @@ our $cmddef = {
 	    }],
     },
 
+    'pre-migrate' => [ __PACKAGE__, 'pre_migrate', ['vmid', 'source-node']],
+    'post-migrate' => [ __PACKAGE__, 'post_migrate', ['vmid']],
 };
 
 1;
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index d52dc8d..b113dec 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -1284,4 +1284,27 @@ sub round_powerof2 {
     return 2 << int(log($_[0]-1)/log(2));
 }
 
+sub pre_migration_hooks {
+    my ($self, $vmid) = @_;
+
+    PVE::GuestHelpers::exec_hookscript($self->{vmconf}, $vmid, 'pre-migrate-source', 1);
+
+    my $node = PVE::INotify::nodename();
+    my $cmd = [ @{$self->{rem_ssh}}, "qm", "pre-migrate", $vmid, $node ];
+
+    eval { $self->cmd($cmd); };
+    die "$@\n" if $@;
+}
+
+sub post_migration_hooks {
+    my ($self, $vmid) = @_;
+
+    PVE::GuestHelpers::exec_hookscript($self->{vmconf}, $vmid, 'post-migrate-source');
+
+    my $cmd = [ @{$self->{rem_ssh}}, "qm", "post-migrate", $vmid ];
+
+    eval { $self->cmd($cmd); };
+    die "$@\n" if $@;
+}
+
 1;
diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
index f2c0281..461b390 100644
--- a/test/MigrationTest/QemuMigrateMock.pm
+++ b/test/MigrationTest/QemuMigrateMock.pm
@@ -298,6 +298,10 @@ $MigrationTest::Shared::tools_module->mock(
 			return 0;
 		    } elsif ($cmd eq 'stop') {
 			return 0;
+		    } elsif ($cmd eq 'pre-migrate') {
+			return 0;
+		    } elsif ($cmd eq 'post-migrate') {
+			return 0;
 		    }
 		    die "run_command (mocked) ssh qm command - implement me: ${cmd_msg}";
 		} elsif ($cmd eq 'pvesm') {
-- 
2.30.2

Re: [pve-devel] [PATCH pve-guest-common 1/1] Add abstract methods for pre/post-migrate hooks

[Thomas Lamprecht]

Am 22/09/2022 um 16:13 schrieb Stefan Hanreich:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  src/PVE/AbstractMigrate.pm | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

for the record, if we do it like this (not much rationale given in the commit message)
this breaks containers and qemu-server without such an implementation and needs the
respective Breaks/Depends entries in d/control (which you cannot add as you cannot
predict the exact version this would get actually added).

> diff --git a/src/PVE/AbstractMigrate.pm b/src/PVE/AbstractMigrate.pm
> index d90e5b7..5e03488 100644
> --- a/src/PVE/AbstractMigrate.pm
> +++ b/src/PVE/AbstractMigrate.pm
> @@ -178,6 +178,8 @@ sub migrate {
>  	        "public key authentication\n" if $@;
>  	}
>  
> +	$self->pre_migration_hooks($self->{vmid});
> +
>  	&$eval_int($self, sub { $self->phase1($self->{vmid}); });
>  	my $err = $@;
>  	if ($err) {
> @@ -228,6 +230,8 @@ sub migrate {
>  	    $self->log('err', $err);
>  	    $self->{errors} = 1;
>  	}
> +
> +	$self->post_migration_hooks($self->{vmid});
>      })};
>  
>      my $err = $@;
> @@ -368,4 +372,14 @@ sub get_bwlimit {
>      return $bwlimit;
>  }
>  
> +sub pre_migration_hooks {
> +    my ($self, $vmid) = @_;
> +    die "abstract method - implement me";
> +}
> +
> +sub post_migration_hooks {
> +    my ($self, $vmid) = @_;
> +    die "abstract method - implement me";
> +}
> +
>  1;

Re: [pve-devel] [PATCH qemu-server 1/1] Add VM hooks for pre/post-migrate on target/source

[Thomas Lamprecht]

I don't like that there's no commit message (the cover letter is more for general/meta
info, it doesn't gets into git after all, would require doing pull requests which we
(currently) don't do in most cases).

Besides that I'd rather avoid extending SSH usage further, a long term goal is to
allow user running PVE clusters with all its features without any SSH connection
required. For VM migration we already got the mtunnel which can be used for such
control stuff; otherwise one can get an API client instead, but there we'd need to
make some sort of execution path part of the public API, can be fine; as the CLI
really isn't /that/ different from the API in publicity/stableness regard (if its
there it will be used, a description comment is a very weak gate), so I'd find a
fully internal handling (i.e., neither CLI nor API exposure) nicer, mtunnel should
make that relatively simple, for CTs one may need to add a similar mechanism.

Also, this requires the script to be available on all notes, the docs should reflect
that.

Am 22/09/2022 um 16:13 schrieb Stefan Hanreich:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  PVE/CLI/qm.pm                         | 50 +++++++++++++++++++++++++++
>  PVE/QemuMigrate.pm                    | 23 ++++++++++++
>  test/MigrationTest/QemuMigrateMock.pm |  4 +++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 6a2e161..4161e6e 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -838,6 +838,54 @@ __PACKAGE__->register_method({
>  	return;
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'pre_migrate',
> +    path => 'pre_migrate',
> +    method => 'POST',
> +    description => "Run pre-migrate hook for VM <vmid> - do not use manually.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
> +	    'source-node' => get_standard_option('pve-node'),
> +	},
> +    },
> +    returns => { type => 'null'},
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $vmid = $param->{vmid};
> +	my $source_node = $param->{'source-node'};
> +	my $conf = PVE::QemuConfig->load_config($vmid, $source_node);
> +
> +	PVE::GuestHelpers::exec_hookscript($conf, $vmid, "pre-migrate-target", 1);
> +
> +	return;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'post_migrate',
> +    path => 'post_migrate',
> +    method => 'POST',
> +    description => "Run post-migrate hook for VM <vmid> - do not use manually.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
> +	},
> +    },
> +    returns => { type => 'null'},
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $vmid = $param->{vmid};
> +	my $conf = PVE::QemuConfig->load_config($vmid);
> +
> +	PVE::GuestHelpers::exec_hookscript($conf, $vmid, "post-migrate-target");
> +
> +	return;
> +    }});
> +
>  my $print_agent_result = sub {
>      my ($data) = @_;
>  
> @@ -988,6 +1036,8 @@ our $cmddef = {
>  	    }],
>      },
>  
> +    'pre-migrate' => [ __PACKAGE__, 'pre_migrate', ['vmid', 'source-node']],
> +    'post-migrate' => [ __PACKAGE__, 'post_migrate', ['vmid']],
>  };
>  
>  1;
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index d52dc8d..b113dec 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -1284,4 +1284,27 @@ sub round_powerof2 {
>      return 2 << int(log($_[0]-1)/log(2));
>  }
>  
> +sub pre_migration_hooks {
> +    my ($self, $vmid) = @_;
> +
> +    PVE::GuestHelpers::exec_hookscript($self->{vmconf}, $vmid, 'pre-migrate-source', 1);
> +
> +    my $node = PVE::INotify::nodename();
> +    my $cmd = [ @{$self->{rem_ssh}}, "qm", "pre-migrate", $vmid, $node ];
> +
> +    eval { $self->cmd($cmd); };
> +    die "$@\n" if $@;
> +}
> +
> +sub post_migration_hooks {
> +    my ($self, $vmid) = @_;
> +
> +    PVE::GuestHelpers::exec_hookscript($self->{vmconf}, $vmid, 'post-migrate-source');
> +
> +    my $cmd = [ @{$self->{rem_ssh}}, "qm", "post-migrate", $vmid ];
> +
> +    eval { $self->cmd($cmd); };
> +    die "$@\n" if $@;
> +}
> +
>  1;
> diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
> index f2c0281..461b390 100644
> --- a/test/MigrationTest/QemuMigrateMock.pm
> +++ b/test/MigrationTest/QemuMigrateMock.pm
> @@ -298,6 +298,10 @@ $MigrationTest::Shared::tools_module->mock(
>  			return 0;
>  		    } elsif ($cmd eq 'stop') {
>  			return 0;
> +		    } elsif ($cmd eq 'pre-migrate') {
> +			return 0;
> +		    } elsif ($cmd eq 'post-migrate') {
> +			return 0;
>  		    }
>  		    die "run_command (mocked) ssh qm command - implement me: ${cmd_msg}";
>  		} elsif ($cmd eq 'pvesm') {

Re: [pve-devel] [PATCH pve-container/qemu-server/pve-guest-common/pve-docs 0/1] Add pre/post-migrate hooks

[Thomas Lamprecht]

Am 22/09/2022 um 16:13 schrieb Stefan Hanreich:
> I have decided to create distinct event types for source/target nodes, since
> otherwise the same script would run essentially twice on the source/target node.
> With distinct event types, the hooks should be more flexible in their usage.

just make that a parameter, same flexibility but less cmd explosion and complexity.

Also, _iff_ (see reply  we keep the CLI entries for pct/qm it should just be a single command
there, any difference should be handled in the parameters; it's internal after all
and we want to avoid that there's more internal commands then externals someday ;)

Target and source should be part of the parameters on either call (pre/post, src/target),
it is relevant info and should be easily available. Some param info like offline/online
migration could be relevant too, but we can always extend on that, so in that regard it
can be fine to stop smaller, to avoid going over board and having to keep all that info
for backward compat. Any parameter would need to be encoded in the example then.

Some more general note, the example is better than nothing, but a nice list/table
directly in the docs would be really good to have. This could be done upfront, before
adding new hooks - best for now to duplicate it for both CT and VM chapter (if sensible
it can live in its own guest-hook-list.adoc and just get included twice). Including
the example script as an appendix would be a nice touch too.

Re: [pve-devel] [PATCH qemu-server 1/1] Add VM hooks for pre/post-migrate on target/source

[Stefan Hanreich]

On 9/26/22 17:38, Thomas Lamprecht wrote:
> I don't like that there's no commit message (the cover letter is more for general/meta
> info, it doesn't gets into git after all, would require doing pull requests which we
> (currently) don't do in most cases).
> 
> Besides that I'd rather avoid extending SSH usage further, a long term goal is to
> allow user running PVE clusters with all its features without any SSH connection
> required. For VM migration we already got the mtunnel which can be used for such
> control stuff; otherwise one can get an API client instead, but there we'd need to
> make some sort of execution path part of the public API, can be fine; as the CLI
> really isn't /that/ different from the API in publicity/stableness regard (if its
> there it will be used, a description comment is a very weak gate), so I'd find a
> fully internal handling (i.e., neither CLI nor API exposure) nicer, mtunnel should
> make that relatively simple, for CTs one may need to add a similar mechanism.
> 
> Also, this requires the script to be available on all notes, the docs should reflect
> that.

Makes sense, I will look into using mtunnel for this! Will also add a 
short note to the documentation.

> Am 22/09/2022 um 16:13 schrieb Stefan Hanreich:
>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>> ---
>>   PVE/CLI/qm.pm                         | 50 +++++++++++++++++++++++++++
>>   PVE/QemuMigrate.pm                    | 23 ++++++++++++
>>   test/MigrationTest/QemuMigrateMock.pm |  4 +++
>>   3 files changed, 77 insertions(+)
>>
>> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
>> index 6a2e161..4161e6e 100755
>> --- a/PVE/CLI/qm.pm
>> +++ b/PVE/CLI/qm.pm
>> @@ -838,6 +838,54 @@ __PACKAGE__->register_method({
>>   	return;
>>       }});
>>   
>> +__PACKAGE__->register_method ({
>> +    name => 'pre_migrate',
>> +    path => 'pre_migrate',
>> +    method => 'POST',
>> +    description => "Run pre-migrate hook for VM <vmid> - do not use manually.",
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
>> +	    'source-node' => get_standard_option('pve-node'),
>> +	},
>> +    },
>> +    returns => { type => 'null'},
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $vmid = $param->{vmid};
>> +	my $source_node = $param->{'source-node'};
>> +	my $conf = PVE::QemuConfig->load_config($vmid, $source_node);
>> +
>> +	PVE::GuestHelpers::exec_hookscript($conf, $vmid, "pre-migrate-target", 1);
>> +
>> +	return;
>> +    }});
>> +
>> +__PACKAGE__->register_method ({
>> +    name => 'post_migrate',
>> +    path => 'post_migrate',
>> +    method => 'POST',
>> +    description => "Run post-migrate hook for VM <vmid> - do not use manually.",
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
>> +	},
>> +    },
>> +    returns => { type => 'null'},
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $vmid = $param->{vmid};
>> +	my $conf = PVE::QemuConfig->load_config($vmid);
>> +
>> +	PVE::GuestHelpers::exec_hookscript($conf, $vmid, "post-migrate-target");
>> +
>> +	return;
>> +    }});
>> +
>>   my $print_agent_result = sub {
>>       my ($data) = @_;
>>   
>> @@ -988,6 +1036,8 @@ our $cmddef = {
>>   	    }],
>>       },
>>   
>> +    'pre-migrate' => [ __PACKAGE__, 'pre_migrate', ['vmid', 'source-node']],
>> +    'post-migrate' => [ __PACKAGE__, 'post_migrate', ['vmid']],
>>   };
>>   
>>   1;
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index d52dc8d..b113dec 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -1284,4 +1284,27 @@ sub round_powerof2 {
>>       return 2 << int(log($_[0]-1)/log(2));
>>   }
>>   
>> +sub pre_migration_hooks {
>> +    my ($self, $vmid) = @_;
>> +
>> +    PVE::GuestHelpers::exec_hookscript($self->{vmconf}, $vmid, 'pre-migrate-source', 1);
>> +
>> +    my $node = PVE::INotify::nodename();
>> +    my $cmd = [ @{$self->{rem_ssh}}, "qm", "pre-migrate", $vmid, $node ];
>> +
>> +    eval { $self->cmd($cmd); };
>> +    die "$@\n" if $@;
>> +}
>> +
>> +sub post_migration_hooks {
>> +    my ($self, $vmid) = @_;
>> +
>> +    PVE::GuestHelpers::exec_hookscript($self->{vmconf}, $vmid, 'post-migrate-source');
>> +
>> +    my $cmd = [ @{$self->{rem_ssh}}, "qm", "post-migrate", $vmid ];
>> +
>> +    eval { $self->cmd($cmd); };
>> +    die "$@\n" if $@;
>> +}
>> +
>>   1;
>> diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
>> index f2c0281..461b390 100644
>> --- a/test/MigrationTest/QemuMigrateMock.pm
>> +++ b/test/MigrationTest/QemuMigrateMock.pm
>> @@ -298,6 +298,10 @@ $MigrationTest::Shared::tools_module->mock(
>>   			return 0;
>>   		    } elsif ($cmd eq 'stop') {
>>   			return 0;
>> +		    } elsif ($cmd eq 'pre-migrate') {
>> +			return 0;
>> +		    } elsif ($cmd eq 'post-migrate') {
>> +			return 0;
>>   		    }
>>   		    die "run_command (mocked) ssh qm command - implement me: ${cmd_msg}";
>>   		} elsif ($cmd eq 'pvesm') {
> 

Re: [pve-devel] [PATCH pve-container/qemu-server/pve-guest-common/pve-docs 0/1] Add pre/post-migrate hooks

[Stefan Hanreich]

On 9/26/22 17:51, Thomas Lamprecht wrote:
> Am 22/09/2022 um 16:13 schrieb Stefan Hanreich:
>> I have decided to create distinct event types for source/target nodes, since
>> otherwise the same script would run essentially twice on the source/target node.
>> With distinct event types, the hooks should be more flexible in their usage.
> 
> just make that a parameter, same flexibility but less cmd explosion and complexity.
> 
> Also, _iff_ (see reply  we keep the CLI entries for pct/qm it should just be a single command
> there, any difference should be handled in the parameters; it's internal after all
> and we want to avoid that there's more internal commands then externals someday ;)
> 
> Target and source should be part of the parameters on either call (pre/post, src/target),
> it is relevant info and should be easily available. Some param info like offline/online
> migration could be relevant too, but we can always extend on that, so in that regard it
> can be fine to stop smaller, to avoid going over board and having to keep all that info
> for backward compat. Any parameter would need to be encoded in the example then.
>

This is also an option I explored. One thing that I wasn't sure about 
was where the scripts run then? Does the pre event run on the source 
node and the post event on the target node? Dominik made an interesting 
point, that it might actually be desirable the other way around since 
you might want to do some setup code in the pre-hook, which would be 
nice on the target node. It might also be nice to run some cleanup code 
on the post-event which would be more suited to running on the source node.

Do you think it would be smart to implement it as positional parameters 
to the script? Like 'qm pre-migrate <target> <source>' ? Since there are 
already ideas of adding additional contextual information, might it be 
smarter to expose all the additional info to the script in a dictionary? 
Not sure about this, but I could see us ending up with a situation where 
you have many additional variables only accessible by knowing their 
indexes. This has other downsides of course..

> Some more general note, the example is better than nothing, but a nice list/table
> directly in the docs would be really good to have. This could be done upfront, before
> adding new hooks - best for now to duplicate it for both CT and VM chapter (if sensible
> it can live in its own guest-hook-list.adoc and just get included twice). Including
> the example script as an appendix would be a nice touch too.

I looked at the documentation as well and found it a bit lacking, I 
thought it would be nice to overhaul this in an additional patch series, 
after all the hooks are merged. I figured it might be okay to silently 
add these features and document them afterwards in a subsequent patch. I 
will add a short documentation section for each hook to the 
documentation in the respective patches as well and then we can maybe 
overhaul/unify them afterwards.

Re: [pve-devel] [PATCH pve-guest-common 1/1] Add abstract methods for pre/post-migrate hooks

[Stefan Hanreich]

On 9/26/22 17:27, Thomas Lamprecht wrote:
> Am 22/09/2022 um 16:13 schrieb Stefan Hanreich:
>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>> ---
>>   src/PVE/AbstractMigrate.pm | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
> 
> for the record, if we do it like this (not much rationale given in the commit message)
> this breaks containers and qemu-server without such an implementation and needs the
> respective Breaks/Depends entries in d/control (which you cannot add as you cannot
> predict the exact version this would get actually added).
>

I figured this might pose some problems when releasing. Is there any way 
I can work around this (also for future patches)? Or do we have to bump 
all 3 packages at once? Anything I should change in particular in this 
case (if we stick to having the hooks in the common package..)

Do you think it might be smarter to move the hooks into the respective 
backend? It shouldn't be too much of a hassle. Maybe it is a smarter 
idea after all, since it allows for more fine-grained control of when 
the hooks should be run exactly.

>> diff --git a/src/PVE/AbstractMigrate.pm b/src/PVE/AbstractMigrate.pm
>> index d90e5b7..5e03488 100644
>> --- a/src/PVE/AbstractMigrate.pm
>> +++ b/src/PVE/AbstractMigrate.pm
>> @@ -178,6 +178,8 @@ sub migrate {
>>   	        "public key authentication\n" if $@;
>>   	}
>>   
>> +	$self->pre_migration_hooks($self->{vmid});
>> +
>>   	&$eval_int($self, sub { $self->phase1($self->{vmid}); });
>>   	my $err = $@;
>>   	if ($err) {
>> @@ -228,6 +230,8 @@ sub migrate {
>>   	    $self->log('err', $err);
>>   	    $self->{errors} = 1;
>>   	}
>> +
>> +	$self->post_migration_hooks($self->{vmid});
>>       })};
>>   
>>       my $err = $@;
>> @@ -368,4 +372,14 @@ sub get_bwlimit {
>>       return $bwlimit;
>>   }
>>   
>> +sub pre_migration_hooks {
>> +    my ($self, $vmid) = @_;
>> +    die "abstract method - implement me";
>> +}
>> +
>> +sub post_migration_hooks {
>> +    my ($self, $vmid) = @_;
>> +    die "abstract method - implement me";
>> +}
>> +
>>   1;
> 

Re: [pve-devel] [PATCH pve-container/qemu-server/pve-guest-common/pve-docs 0/1] Add pre/post-migrate hooks

[Thomas Lamprecht]

Am 27/09/2022 um 09:40 schrieb Stefan Hanreich:
> On 9/26/22 17:51, Thomas Lamprecht wrote:
>> Am 22/09/2022 um 16:13 schrieb Stefan Hanreich:
>>> I have decided to create distinct event types for source/target nodes, since
>>> otherwise the same script would run essentially twice on the source/target node.
>>> With distinct event types, the hooks should be more flexible in their usage.
>>
>> just make that a parameter, same flexibility but less cmd explosion and
>> complexity.
>>
>> Also, _iff_ (see reply  we keep the CLI entries for pct/qm it should just be
>> a single command there, any difference should be handled in the parameters;
>> it's internal after all and we want to avoid that there's more internal
>> commands then externals someday ;)
>>
>> Target and source should be part of the parameters on either call (pre/post,
>> src/target), it is relevant info and should be easily available. Some param
>> info like offline/online migration could be relevant too, but we can always
>> extend on that, so in that regard it can be fine to stop smaller, to avoid
>> going over board and having to keep all that info for backward compat. Any
>> parameter would need to be encoded in the example then.
>>
> 
> This is also an option I explored. One thing that I wasn't sure about was
> where the scripts run then? Does the pre event run on the source node and the
> post event on the target node? Dominik made an interesting point, that it
> might actually be desirable the other way around since you might want to do
> some setup code in the pre-hook, which would be nice on the target node. It
> might also be nice to run some cleanup code on the post-event which would be
> more suited to running on the source node.

IMO they both need to run on both, that's the point of a migration hookscript
prepare source & target for leaving/incomming guest and then cleanup source &
target after the migration happened (failed or not).

> 
> Do you think it would be smart to implement it as positional parameters to
> the script? Like 'qm pre-migrate <target> <source>' ? Since there are already
> ideas of adding additional contextual information, might it be smarter to
> expose all the additional info to the script in a dictionary? Not sure about
> this, but I could see us ending up with a situation where you have many
> additional variables only accessible by knowing their indexes. This has other
> downsides of course..

_if_ we stay with this approach I'd use as much non-positional params as
possible. This is an internal command so user facing UX doesn't matter, the
only thing that matters is having some flexibility for forward/backward
extendability/compat, and there fixed params are worse than none, iow. they
have no benefit for a internal, automatic called script.

> 
>> Some more general note, the example is better than nothing, but a nice
>> list/table directly in the docs would be really good to have. This could be
>> done upfront, before adding new hooks - best for now to duplicate it for
>> both CT and VM chapter (if sensible it can live in its own
>> guest-hook-list.adoc and just get included twice). Including the example
>> script as an appendix would be a nice touch too.
> 
> I looked at the documentation as well and found it a bit lacking, I thought
> it would be nice to overhaul this in an additional patch series, after all
> the hooks are merged. I figured it might be okay to silently add these
> features and document them afterwards in a subsequent patch. I will add a
> short documentation section for each hook to the documentation in the
> respective patches as well and then we can maybe overhaul/unify them
> afterwards.

IMO the status quo is best documented before extending it.

Re: [pve-devel] [PATCH pve-guest-common 1/1] Add abstract methods for pre/post-migrate hooks

[Thomas Lamprecht]

Am 27/09/2022 um 09:40 schrieb Stefan Hanreich:
> On 9/26/22 17:27, Thomas Lamprecht wrote:
>> Am 22/09/2022 um 16:13 schrieb Stefan Hanreich:
>>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>>> ---
>>>   src/PVE/AbstractMigrate.pm | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>
>> for the record, if we do it like this (not much rationale given in the
>> commit message) this breaks containers and qemu-server without such an
>> implementation and needs the respective Breaks/Depends entries in d/control
>> (which you cannot add as you cannot predict the exact version this would get
>> actually added).
>>
> 
> I figured this might pose some problems when releasing. Is there any way I
> can work around this (also for future patches)? Or do we have to bump all 3
> packages at once? Anything I should change in particular in this case (if we
> stick to having the hooks in the common package..)

The only way to avoid that is to add such features in such a way that old
dependency consumers (qemu-server and pve-container in this case) can simply
ignore their existence (see below for how to do it in this case). Note that
this may not be always the thing one actually wants, sometimes it is the
cleanest way to break old dependants and and change them and just encode this
as breaks of old consumer packages in the provider package's d/control, and as
depends on new provider package in the consumer's d/control. This is itself
really not a problem, it's just a bit extra work and adds some sort of a
"dependency synchronization" boundary, making it much harder for users to
downgrade a single package below said boundary after, e.g., getting regressions
there with an upgrade; so one needs to weigh each specific feature/bug fix with
that in mind. In general, new opt-in features (like here) are more likely to
get away without a full break/depends while avoiding much extra code to handle
that.

> 
> Do you think it might be smarter to move the hooks into the respective
> backend? It shouldn't be too much of a hassle. Maybe it is a smarter idea
> after all, since it allows for more fine-grained control of when the hooks
> should be run exactly.

If you keep them here you could simply drop the die in the base method, then
you simply require a one way versioned depenency bump in the consumer packages
for the provider one, and even that for a higher level assurance that the hooks
are actually called (as code wise nothing would break).