[PATCH 1/3] testbot/Engine: Kill the child before forking a new one for sacrifices.

[Francois Gouget]

VM::Run() now checks that no process is actively using the VM before
starting the new one. Killing the current process makes more sense
anyway and could help ensure it won't interfere with the new one by
sending TestAgent commands.

Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com>
---

Same as before but integrated into the patch series and fixed the commit 
message.

Now that I have been able to reproduce the issue I have been able to 
confirm that this patch avoids the occurence of the circumstance that 
caused the Engine to crash last week-end.


 testbot/lib/WineTestBot/Engine/Scheduler.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/testbot/lib/WineTestBot/Engine/Scheduler.pm b/testbot/lib/WineTestBot/Engine/Scheduler.pm
index e9a3ff9fe..737533785 100644
--- a/testbot/lib/WineTestBot/Engine/Scheduler.pm
+++ b/testbot/lib/WineTestBot/Engine/Scheduler.pm
@@ -765,6 +765,7 @@ sub _SacrificeVM($$$)
   $Host->{$Victim->Status}--;
   $Host->{dirty}++;
   $Victim->RecordStatus($Sched->{records}, $Victim->Status eq "dirty" ? "dirty poweroff" : "dirty sacrifice");
+  $Victim->KillChild();
   $Victim->RunPowerOff();
   return 1;
 }
-- 
2.20.1

[PATCH 2/3] testbot/Engine: Let the caller trace VM::Run() errors.

[Francois Gouget]

Libraries are not supposed to spam the log and most callers already 
trace VM::Run() errors.

Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com>
---
 testbot/lib/WineTestBot/Engine/Scheduler.pm | 3 ++-
 testbot/lib/WineTestBot/VMs.pm              | 5 +----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/testbot/lib/WineTestBot/Engine/Scheduler.pm b/testbot/lib/WineTestBot/Engine/Scheduler.pm
index 737533785..4128e9e1e 100644
--- a/testbot/lib/WineTestBot/Engine/Scheduler.pm
+++ b/testbot/lib/WineTestBot/Engine/Scheduler.pm
@@ -766,7 +766,8 @@ sub _SacrificeVM($$$)
   $Host->{dirty}++;
   $Victim->RecordStatus($Sched->{records}, $Victim->Status eq "dirty" ? "dirty poweroff" : "dirty sacrifice");
   $Victim->KillChild();
-  $Victim->RunPowerOff();
+  my $ErrMessage = $Victim->RunPowerOff();
+  LogMsg "$ErrMessage\n" if (defined $ErrMessage);
   return 1;
 }
 
diff --git a/testbot/lib/WineTestBot/VMs.pm b/testbot/lib/WineTestBot/VMs.pm
index d85df1d08..d240fb964 100644
--- a/testbot/lib/WineTestBot/VMs.pm
+++ b/testbot/lib/WineTestBot/VMs.pm
@@ -379,10 +379,7 @@ sub Run($$$$$$)
 
   if (defined $self->ChildPid)
   {
-    my $ErrMessage = "Cannot run ". ShArgv2Cmd(@$Args) ." because the ". $self->ChildPid ." process is already using the ". $self->Name ." VM";
-    require WineTestBot::Log;
-    WineTestBot::Log::LogMsg("$ErrMessage\n");
-    return $ErrMessage;
+    return "Cannot run ". ShArgv2Cmd(@$Args) ." because process ". $self->ChildPid ." is already using the ". $self->Name ." VM";
   }
 
   # There are two $VM->ChildPid race conditions to avoid:
-- 
2.20.1

[PATCH 3/3] testbot/Engine: Fix recording the new sacrificed VM status.

[Francois Gouget]

The status will not be 'dirty' if VM::RunPowerOff() failed.
Recording the right status is important otherwise ScheduleJobs() will
attempt to record de VM's current status in its catch-all loop,
resulting in a fatal 'Could not change key' error.

Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com>
---

This prevents the Engine from crashing, even if VM::RunPowerOff() does 
fail.

 testbot/lib/WineTestBot/Engine/Scheduler.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testbot/lib/WineTestBot/Engine/Scheduler.pm b/testbot/lib/WineTestBot/Engine/Scheduler.pm
index 4128e9e1e..530a2ee1e 100644
--- a/testbot/lib/WineTestBot/Engine/Scheduler.pm
+++ b/testbot/lib/WineTestBot/Engine/Scheduler.pm
@@ -764,10 +764,10 @@ sub _SacrificeVM($$$)
   $Sched->{busyvms}->{$VictimKey} = 1;
   $Host->{$Victim->Status}--;
   $Host->{dirty}++;
-  $Victim->RecordStatus($Sched->{records}, $Victim->Status eq "dirty" ? "dirty poweroff" : "dirty sacrifice");
   $Victim->KillChild();
   my $ErrMessage = $Victim->RunPowerOff();
   LogMsg "$ErrMessage\n" if (defined $ErrMessage);
+  $Victim->RecordStatus($Sched->{records}, $Victim->Status eq "dirty" ? "dirty poweroff" : $VM->Status ." sacrifice");
   return 1;
 }
 
-- 
2.20.1