[pve-devel] [PATCH qemu-server] migrate: keep VM paused after migration if it was before

[pve-devel] [PATCH qemu-server] migrate: keep VM paused after migration if it was before

[Fabian Ebner]

Also cannot issue a guest agent command in that case.

Reported in the community forum:
https://forum.proxmox.com/threads/106618

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Best viewed with -w.

 PVE/QemuMigrate.pm | 54 ++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 104e62ce..891edfb2 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -158,6 +158,8 @@ sub prepare {
 		$self->{forcecpu} = PVE::QemuServer::CPUConfig::get_cpu_from_running_vm($pid);
 	    }
 	}
+
+	$self->{vm_was_paused} = 1 if PVE::QemuServer::vm_is_paused($vmid);
     }
 
     my $loc_res = PVE::QemuServer::check_local_resources($conf, 1);
@@ -1146,31 +1148,37 @@ sub phase3_cleanup {
 	    }
 	}
 
-	# config moved and nbd server stopped - now we can resume vm on target
-	if ($tunnel && $tunnel->{version} && $tunnel->{version} >= 1) {
-	    eval {
-		PVE::Tunnel::write_tunnel($tunnel, 30, "resume $vmid");
-	    };
-	    if (my $err = $@) {
-		$self->log('err', $err);
-		$self->{errors} = 1;
-	    }
-	} else {
-	    my $cmd = [@{$self->{rem_ssh}}, 'qm', 'resume', $vmid, '--skiplock', '--nocheck'];
-	    my $logf = sub {
-		my $line = shift;
-		$self->log('err', $line);
-	    };
-	    eval { PVE::Tools::run_command($cmd, outfunc => sub {}, errfunc => $logf); };
-	    if (my $err = $@) {
-		$self->log('err', $err);
-		$self->{errors} = 1;
+	if (!$self->{vm_was_paused}) {
+	    # config moved and nbd server stopped - now we can resume vm on target
+	    if ($tunnel && $tunnel->{version} && $tunnel->{version} >= 1) {
+		eval {
+		    PVE::Tunnel::write_tunnel($tunnel, 30, "resume $vmid");
+		};
+		if (my $err = $@) {
+		    $self->log('err', $err);
+		    $self->{errors} = 1;
+		}
+	    } else {
+		my $cmd = [@{$self->{rem_ssh}}, 'qm', 'resume', $vmid, '--skiplock', '--nocheck'];
+		my $logf = sub {
+		    my $line = shift;
+		    $self->log('err', $line);
+		};
+		eval { PVE::Tools::run_command($cmd, outfunc => sub {}, errfunc => $logf); };
+		if (my $err = $@) {
+		    $self->log('err', $err);
+		    $self->{errors} = 1;
+		}
 	    }
-	}
 
-	if ($self->{storage_migration} && PVE::QemuServer::parse_guest_agent($conf)->{fstrim_cloned_disks} && $self->{running}) {
-	    my $cmd = [@{$self->{rem_ssh}}, 'qm', 'guest', 'cmd', $vmid, 'fstrim'];
-	    eval{ PVE::Tools::run_command($cmd, outfunc => sub {}, errfunc => sub {}) };
+	    if (
+		$self->{storage_migration}
+		&& PVE::QemuServer::parse_guest_agent($conf)->{fstrim_cloned_disks}
+		&& $self->{running}
+	    ) {
+		my $cmd = [@{$self->{rem_ssh}}, 'qm', 'guest', 'cmd', $vmid, 'fstrim'];
+		eval{ PVE::Tools::run_command($cmd, outfunc => sub {}, errfunc => sub {}) };
+	    }
 	}
     }
 
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server] migrate: keep VM paused after migration if it was before

[Fabian Grünbichler]

On March 18, 2022 8:51 am, Fabian Ebner wrote:
> Also cannot issue a guest agent command in that case.
> 
> Reported in the community forum:
> https://forum.proxmox.com/threads/106618
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Best viewed with -w.
> 
>  PVE/QemuMigrate.pm | 54 ++++++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 23 deletions(-)

patch looks good to me - it might make sense to restructure the 
conditionals a bit to log that resuming/fstrim was skipped though to 
reduce confusion (user that paused VM and user doing the migration might 
not be the same entity after all)?

one other thing I noticed (pre-existing, but the changes here made me 
look and my search came up short), inside phase2:

- start block job(s) without autocompletion and wait for them to 
  converge
- start RAM/state migration without autocompletion and wait for it to 
  converge
X both source and target VMs are paused now with "identical" state, 
irrespective of the source being paused or not initially
- cancel block job(s) (to close NBD writer(s) so that switchover can 
  proceed in phase3_cleanup)

if something happens after X in phase2, we enter phase2_cleanup, and 
attempt to cancel the migration, remove the lock, cancel the block jobs 
again, clean up bitmaps, stop the target VM, clean up remote disks, tear 
down the tunnel, and effectively exit the migration at that point BUT - 
we don't handle the paused state? is there a resume source (with this 
patch, guarded by source was not paused) missing or am I missing 
something?

[pve-devel] applied: [PATCH qemu-server] migrate: keep VM paused after migration if it was before

[Thomas Lamprecht]

On 18.03.22 08:51, Fabian Ebner wrote:
> Also cannot issue a guest agent command in that case.
> 
> Reported in the community forum:
> https://forum.proxmox.com/threads/106618
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Best viewed with -w.
> 
>  PVE/QemuMigrate.pm | 54 ++++++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 23 deletions(-)
> 
>

It makes definitively sense to check Fabians findings more closely, but as they're pre-existing
behavior and as I run into this recently quite often too:

applied, thanks!

Re: [pve-devel] [PATCH qemu-server] migrate: keep VM paused after migration if it was before

[Fabian Ebner]

Am 20.04.22 um 14:43 schrieb Fabian Grünbichler:
> On March 18, 2022 8:51 am, Fabian Ebner wrote:
>> Also cannot issue a guest agent command in that case.
>>
>> Reported in the community forum:
>> https://forum.proxmox.com/threads/106618
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Best viewed with -w.
>>
>>  PVE/QemuMigrate.pm | 54 ++++++++++++++++++++++++++--------------------
>>  1 file changed, 31 insertions(+), 23 deletions(-)
> 
> patch looks good to me - it might make sense to restructure the 
> conditionals a bit to log that resuming/fstrim was skipped though to 
> reduce confusion (user that paused VM and user doing the migration might 
> not be the same entity after all)?

Yes, I'll add a log line as a followup.

> 
> one other thing I noticed (pre-existing, but the changes here made me 
> look and my search came up short), inside phase2:
> 
> - start block job(s) without autocompletion and wait for them to 
>   converge
> - start RAM/state migration without autocompletion and wait for it to 
>   converge
> X both source and target VMs are paused now with "identical" state, 
> irrespective of the source being paused or not initially
> - cancel block job(s) (to close NBD writer(s) so that switchover can 
>   proceed in phase3_cleanup)
> 
> if something happens after X in phase2, we enter phase2_cleanup, and 
> attempt to cancel the migration, remove the lock, cancel the block jobs 
> again, clean up bitmaps, stop the target VM, clean up remote disks, tear 
> down the tunnel, and effectively exit the migration at that point BUT - 
> we don't handle the paused state? is there a resume source (with this 
> patch, guarded by source was not paused) missing or am I missing 
> something?

Quickly tested it, but there is no resume call for the source (with or
without the patch) in this scenario. I don't think there is any real
downside to try and resume on the source in phase2_cleanup().

Re: [pve-devel] [PATCH qemu-server] migrate: keep VM paused after migration if it was before

[Fabian Grünbichler]

On April 21, 2022 9:20 am, Fabian Ebner wrote:
> Am 20.04.22 um 14:43 schrieb Fabian Grünbichler:
>> one other thing I noticed (pre-existing, but the changes here made me 
>> look and my search came up short), inside phase2:
>> 
>> - start block job(s) without autocompletion and wait for them to 
>>   converge
>> - start RAM/state migration without autocompletion and wait for it to 
>>   converge
>> X both source and target VMs are paused now with "identical" state, 
>> irrespective of the source being paused or not initially
>> - cancel block job(s) (to close NBD writer(s) so that switchover can 
>>   proceed in phase3_cleanup)
>> 
>> if something happens after X in phase2, we enter phase2_cleanup, and 
>> attempt to cancel the migration, remove the lock, cancel the block jobs 
>> again, clean up bitmaps, stop the target VM, clean up remote disks, tear 
>> down the tunnel, and effectively exit the migration at that point BUT - 
>> we don't handle the paused state? is there a resume source (with this 
>> patch, guarded by source was not paused) missing or am I missing 
>> something?
> 
> Quickly tested it, but there is no resume call for the source (with or
> without the patch) in this scenario. I don't think there is any real
> downside to try and resume on the source in phase2_cleanup().

exactly - I expected such a call to be there somewhere in error handling 
(similar to the vzdump flow), but didn't find it. it was just your patch 
that made me look, not that your patch made anything worse ;)

Re: [pve-devel] [PATCH qemu-server] migrate: keep VM paused after migration if it was before

[Fabian Ebner]

Am 20.04.22 um 14:43 schrieb Fabian Grünbichler:
> On March 18, 2022 8:51 am, Fabian Ebner wrote:
>> Also cannot issue a guest agent command in that case.
>>
>> Reported in the community forum:
>> https://forum.proxmox.com/threads/106618
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Best viewed with -w.
>>
>>  PVE/QemuMigrate.pm | 54 ++++++++++++++++++++++++++--------------------
>>  1 file changed, 31 insertions(+), 23 deletions(-)
> 
> patch looks good to me - it might make sense to restructure the 
> conditionals a bit to log that resuming/fstrim was skipped though to 
> reduce confusion (user that paused VM and user doing the migration might 
> not be the same entity after all)?
> 
> one other thing I noticed (pre-existing, but the changes here made me 
> look and my search came up short), inside phase2:
> 
> - start block job(s) without autocompletion and wait for them to 
>   converge
> - start RAM/state migration without autocompletion and wait for it to 
>   converge
> X both source and target VMs are paused now with "identical" state, 
> irrespective of the source being paused or not initially
> - cancel block job(s) (to close NBD writer(s) so that switchover can 
>   proceed in phase3_cleanup)
> 
> if something happens after X in phase2, we enter phase2_cleanup, and 
> attempt to cancel the migration

If migrate_cancel actually cancels the migration, the VM will be running
on the source node again :)

If migrate_cancel fails, resume might also fail?

There is an edge case however:
If migration actually finished, but we aborted because of e.g. too many
query-migrate failures, then migrate_cancel will succeed (because there
is no active migration) and the VM will be in post-migrate state on the
source node. Here, resume would help.

> , remove the lock, cancel the block jobs 
> again, clean up bitmaps, stop the target VM, clean up remote disks, tear 
> down the tunnel, and effectively exit the migration at that point BUT - 
> we don't handle the paused state? is there a resume source (with this 
> patch, guarded by source was not paused) missing or am I missing 
> something?
> 

Re: [pve-devel] [PATCH qemu-server] migrate: keep VM paused after migration if it was before

[Fabian Grünbichler]

On April 21, 2022 9:44 am, Fabian Ebner wrote:
> Am 20.04.22 um 14:43 schrieb Fabian Grünbichler:
>> On March 18, 2022 8:51 am, Fabian Ebner wrote:
>>> Also cannot issue a guest agent command in that case.
>>>
>>> Reported in the community forum:
>>> https://forum.proxmox.com/threads/106618
>>>
>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>> ---
>>>
>>> Best viewed with -w.
>>>
>>>  PVE/QemuMigrate.pm | 54 ++++++++++++++++++++++++++--------------------
>>>  1 file changed, 31 insertions(+), 23 deletions(-)
>> 
>> patch looks good to me - it might make sense to restructure the 
>> conditionals a bit to log that resuming/fstrim was skipped though to 
>> reduce confusion (user that paused VM and user doing the migration might 
>> not be the same entity after all)?
>> 
>> one other thing I noticed (pre-existing, but the changes here made me 
>> look and my search came up short), inside phase2:
>> 
>> - start block job(s) without autocompletion and wait for them to 
>>   converge
>> - start RAM/state migration without autocompletion and wait for it to 
>>   converge
>> X both source and target VMs are paused now with "identical" state, 
>> irrespective of the source being paused or not initially
>> - cancel block job(s) (to close NBD writer(s) so that switchover can 
>>   proceed in phase3_cleanup)
>> 
>> if something happens after X in phase2, we enter phase2_cleanup, and 
>> attempt to cancel the migration
> 
> If migrate_cancel actually cancels the migration, the VM will be running
> on the source node again :)

see below - AFAIU migrate_cancel will resume if the migration reached 
the point of being completed and the VM was not paused, but running at 
that point.

> If migrate_cancel fails, resume might also fail?

possibly / probably, but at that point then error handling is already 
best-effort so it doesn't hurt to try I guess. the only way for 
migrate_cancel to fail (according to the docs ;)) is if QMP itself is 
not available..

> There is an edge case however:
> If migration actually finished, but we aborted because of e.g. too many
> query-migrate failures, then migrate_cancel will succeed (because there
> is no active migration) and the VM will be in post-migrate state on the
> source node. Here, resume would help.

this confused me, so I tried to look at different code paths - a 
transition from phase2 (after migration has started, any error before 
that is irrelevant since it couldn't have triggered a pause) to 
phase2_cleanup with $err / $self->{errors} set can only happen if

A) migration reached 'completed' state, VM state transitioned to 
FINISH_MIGRATE, but block-job completing code after the query-migrate 
loop died
B) query-migrate failed too often, VM state unknown
C) query-migrate returned no or an invalid status, VM state unknown
D) query-migrate returned failed or cancelled status, VM state unknown
E) migration was interrupted

in case A (which was my original sequence of events), the VM is now 
paused for sure (by the migration), and we only want to resume it (via 
migrate_cancel?) if it wasn't previously paused. so does migrate_cancel 
always trigger a resume? if so, do we want to (re-)pause the VM 
depending on whether it was originally paused?

in case B or C we don't know the state or what it means. we can attempt 
to migrate_cancel (which might resume the VM?), which might give us the 
same situation as A

in case D or E the VM might have been paused (and resumed again) by the 
migration, or maybe not. so we might still want to do the same as with 
situation A.

looking at the qemu source it should keep track of whether the VM was
running before it paused during the migration completion 
(MigrationState->vm_was_running).

this gets set when the migration converges (in migration_completion), 
before the VM transitions to FINISH_MIGRATE (i.e., paused while waiting 
for completion). from there on we have two options - either the 
migration status switches to failed, or to completed, and we BREAK the 
migration iteration (migration_iteration_run), and we end up in 
migration_iteration_finish.

migration_iteration_finish will transition the VM to POSTMIGRATE if the 
migration was completed. otherwise, if the migration got cancelled, it 
will start the VM again if it was marked as previously running but 
paused by migration (see above), or else if it entered convergence but 
didn't complete yet it will be switched to POSTMIGRATE (without 
vm_start). cancelling before the migration converged simple does nothing 
except cleaning up the migration - the VM is still running or paused 
depending on its original state, since the migration never paused it it 
also had no need to resume it.

so there is case A, where we should end up in POSTMIGRATE and want to 
return to RUNNING or PAUSED, depending on original state. there is case 
B, where cancelling before convergence is fine, cancelling after 
convergence but before completion should resume iff originally running 
(fine), but end up in POSTMIGRATE instead of PAUSED if not (wrong), with 
cases C, D and E being similar but likely not resumed/completed.

but all of that would require some confirmation via tracing and 
corresponding 'die' statements, possibly I didn't get the full picture 
of how cancelling interacts with completion for example ;)

Re: [pve-devel] [PATCH qemu-server] migrate: keep VM paused after migration if it was before

[Fabian Ebner]

Am 21.04.22 um 11:15 schrieb Fabian Grünbichler:
> 
> this confused me, so I tried to look at different code paths - a 
> transition from phase2 (after migration has started, any error before 
> that is irrelevant since it couldn't have triggered a pause) to 
> phase2_cleanup with $err / $self->{errors} set can only happen if
> 
> A) migration reached 'completed' state, VM state transitioned to 
> FINISH_MIGRATE, but block-job completing code after the query-migrate 
> loop died
> B) query-migrate failed too often, VM state unknown
> C) query-migrate returned no or an invalid status, VM state unknown
> D) query-migrate returned failed or cancelled status, VM state unknown
> E) migration was interrupted
> 
> in case A (which was my original sequence of events), the VM is now 
> paused for sure (by the migration), and we only want to resume it (via 
> migrate_cancel?) if it wasn't previously paused. so does migrate_cancel 
> always trigger a resume? if so, do we want to (re-)pause the VM 
> depending on whether it was originally paused?
> 
> in case B or C we don't know the state or what it means. we can attempt 
> to migrate_cancel (which might resume the VM?), which might give us the 
> same situation as A
> 
> in case D or E the VM might have been paused (and resumed again) by the 
> migration, or maybe not. so we might still want to do the same as with 
> situation A.
> 
> looking at the qemu source it should keep track of whether the VM was
> running before it paused during the migration completion 
> (MigrationState->vm_was_running).
> 
> this gets set when the migration converges (in migration_completion), 
> before the VM transitions to FINISH_MIGRATE (i.e., paused while waiting 
> for completion). from there on we have two options - either the 
> migration status switches to failed, or to completed, and we BREAK the 
> migration iteration (migration_iteration_run), and we end up in 
> migration_iteration_finish.
> 
> migration_iteration_finish will transition the VM to POSTMIGRATE if the 
> migration was completed. otherwise, if the migration got cancelled, it 
> will start the VM again if it was marked as previously running but 
> paused by migration (see above), or else if it entered convergence but 
> didn't complete yet it will be switched to POSTMIGRATE (without 
> vm_start). cancelling before the migration converged simple does nothing 
> except cleaning up the migration - the VM is still running or paused 
> depending on its original state, since the migration never paused it it 
> also had no need to resume it.
> 
> so there is case A, where we should end up in POSTMIGRATE and want to 
> return to RUNNING or PAUSED, depending on original state. there is case 
> B, where cancelling before convergence is fine, cancelling after 
> convergence but before completion should resume iff originally running 
> (fine), but end up in POSTMIGRATE instead of PAUSED if not (wrong), with 
> cases C, D and E being similar but likely not resumed/completed.

If the VM was running before, I think we can just check if the state is
POSTMIGRATE (after migrate_cancel, both cases where we know for sure we
want to resume should end up there; if there is some weird corner case
we missed, I'd say adding it once it pops up is the way to go) and try
to go back to RUNNING by issuing a QMP 'cont'.

We can't actually go from POSTMIGRATE back to PAUSED directly though:

typedef struct {
    RunState from;
    RunState to;
} RunStateTransition;

static const RunStateTransition runstate_transitions_def[] = {
(...)

    { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
    { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
    { RUN_STATE_POSTMIGRATE, RUN_STATE_PRELAUNCH },

(...)

Not sure what we can do besides keeping it in POSTMIGRATE there? It
shouldn't be as relevant as in the "was running" case IMHO. Well,
enabling the start button in the UI would help ;)