Francois Gouget : testbot: Handle staging at the Job level and avoid race conditions.
Alexandre Julliard
julliard at winehq.org
Mon Jun 11 09:49:49 CDT 2018
Module: tools
Branch: master
Commit: bc395ccc993d13b48df3e2d5c807e78d5c6ca6c4
URL: https://source.winehq.org/git/tools.git/?a=commit;h=bc395ccc993d13b48df3e2d5c807e78d5c6ca6c4
Author: Francois Gouget <fgouget at codeweavers.com>
Date: Thu Jun 7 00:31:43 2018 +0200
testbot: Handle staging at the Job level and avoid race conditions.
When a patch or executable is submitted for testing it is stored in the
staging directory for two reasons, first because the corresponding Job
does not exist yet, and second because the process may not have write
permissions to the Job directory.
So far transfering the file to the Job directory was the responsibility
of the Step that needs it. However this breaks down when more than one
Step needs that file (e.g. a Windows executable build Step and a Wine
testing one).
There is also a (very unlikely) race condition between the scheduler
and the Job creation: the Scheduler could run the first saved Task, mark
it and the Job as completed before the Job's other Steps and Tasks get
saved to the database. Those would then never get run since the Jobs
has completed.
So this patch makes staging the responsibility of the Job and introduces
a staging Status for it.
It also introduces a new Status which indicates the Job is still being
set up to avoid race conditions.
Note that this patch preserves the Step-based staging mechanism for
backward compatibility during the transition.
Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
Signed-off-by: Alexandre Julliard <julliard at winehq.org>
---
testbot/bin/CheckForWinetestUpdate.pl | 2 ++
testbot/ddl/update36.sql | 4 ++++
testbot/ddl/winetestbot.sql | 2 +-
testbot/lib/WineTestBot/Engine/Scheduler.pm | 30 ++++++++++++++++++++++++++++-
testbot/lib/WineTestBot/Jobs.pm | 20 +++++++++++++++----
testbot/lib/WineTestBot/Patches.pm | 1 +
testbot/web/Submit.pl | 1 +
7 files changed, 54 insertions(+), 6 deletions(-)
diff --git a/testbot/bin/CheckForWinetestUpdate.pl b/testbot/bin/CheckForWinetestUpdate.pl
index f41148f..17dab82 100755
--- a/testbot/bin/CheckForWinetestUpdate.pl
+++ b/testbot/bin/CheckForWinetestUpdate.pl
@@ -200,6 +200,7 @@ sub AddJob($$$)
# First create a new job
my $Jobs = CreateJobs();
my $NewJob = $Jobs->Add();
+ $NewJob->Status("queued");
$NewJob->User(GetBatchUser());
$NewJob->Priority($BaseJob && $Bits == 32 ? 8 : 9);
$NewJob->Remarks($Remarks);
@@ -243,6 +244,7 @@ sub AddReconfigJob()
# First create a new job
my $Jobs = CreateJobs();
my $NewJob = $Jobs->Add();
+ $NewJob->Status("queued");
$NewJob->User(GetBatchUser());
$NewJob->Priority(3);
$NewJob->Remarks($Remarks);
diff --git a/testbot/ddl/update36.sql b/testbot/ddl/update36.sql
new file mode 100644
index 0000000..2f4bf7f
--- /dev/null
+++ b/testbot/ddl/update36.sql
@@ -0,0 +1,4 @@
+USE winetestbot;
+
+ALTER TABLE Jobs
+ MODIFY Status ENUM('new', 'staging', 'queued', 'running', 'completed', 'badpatch', 'badbuild', 'boterror', 'canceled') NOT NULL;
diff --git a/testbot/ddl/winetestbot.sql b/testbot/ddl/winetestbot.sql
index 9a28385..1e7b1ed 100644
--- a/testbot/ddl/winetestbot.sql
+++ b/testbot/ddl/winetestbot.sql
@@ -113,7 +113,7 @@ CREATE TABLE Jobs
BranchName VARCHAR(20) NOT NULL,
UserName VARCHAR(40) NOT NULL,
Priority INT(1) NOT NULL,
- Status ENUM('queued', 'running', 'completed', 'badpatch', 'badbuild', 'boterror', 'canceled') NOT NULL,
+ Status ENUM('new', 'staging', 'queued', 'running', 'completed', 'badpatch', 'badbuild', 'boterror', 'canceled') NOT NULL,
Remarks VARCHAR(128) NULL,
Submitted DATETIME NOT NULL,
Ended DATETIME NULL,
diff --git a/testbot/lib/WineTestBot/Engine/Scheduler.pm b/testbot/lib/WineTestBot/Engine/Scheduler.pm
index 10c0b70..85bc0f3 100644
--- a/testbot/lib/WineTestBot/Engine/Scheduler.pm
+++ b/testbot/lib/WineTestBot/Engine/Scheduler.pm
@@ -30,6 +30,8 @@ WineTestBot::Engine::Scheduler - Schedules the TestBot tasks
use Exporter 'import';
our @EXPORT = qw(ScheduleJobs CheckJobs);
+use File::Copy;
+
use WineTestBot::Config;
use WineTestBot::Engine::Events;
use WineTestBot::Jobs;
@@ -524,11 +526,37 @@ sub _ScheduleTasks($)
# Process the jobs in decreasing priority order
my $JobRank;
my $Jobs = CreateJobs($Sched->{VMs});
- $Jobs->AddFilter("Status", ["queued", "running"]);
+ $Jobs->AddFilter("Status", ["staging", "queued", "running"]);
foreach my $Job (sort CompareJobPriority @{$Jobs->GetItems()})
{
$JobRank++;
+ if ($Job->Status eq "staging")
+ {
+ # Move the file(s) from the staging directory to the job directory
+ my %Staged;
+ my $JobDir = $Job->CreateDir();
+ foreach my $Step (@{$Job->Steps->Clone()->GetItems()})
+ {
+ # Ignore steps that need a file provided by the previous step
+ next if ($Step->PreviousNo or !defined $Step->FileName);
+ # Skip the step if its file has already been staged
+ next if ($Staged{$Step->FileName});
+
+ my $StagingFile = "job". $Job->Id ."_". $Step->FileName;
+ if (move("$DataDir/staging/$StagingFile", "$JobDir/". $Step->FileName))
+ {
+ $Staged{$Step->FileName} = 1;
+ }
+ else
+ {
+ LogMsg "Could not move the '$StagingFile' staging file: $!";
+ }
+ }
+ $Job->Status("queued");
+ $Job->Save();
+ }
+
# The per-step lists of VMs that should be getting ready to run
# before we prepare the next step
my %StepVMs = ("" => []); # no dependency for the first step
diff --git a/testbot/lib/WineTestBot/Jobs.pm b/testbot/lib/WineTestBot/Jobs.pm
index 13a3895..3fd78aa 100644
--- a/testbot/lib/WineTestBot/Jobs.pm
+++ b/testbot/lib/WineTestBot/Jobs.pm
@@ -1,6 +1,6 @@
# -*- Mode: Perl; perl-indent-level: 2; indent-tabs-mode: nil -*-
# Copyright 2009 Ge van Geldorp
-# Copyright 2012-2017 Francois Gouget
+# Copyright 2012-2018 Francois Gouget
#
# This library is free software; you can redistribute it and/or
# modify it under the terms of the GNU Lesser General Public
@@ -52,7 +52,19 @@ A Job's lifecycle is as follows:
=over
=item *
-A Job is created with Status set to queued which means it is ready to run.
+A Job is created with Status set to new. During this time the process setting
+up the Job can add new Steps and Tasks to it. It can also put the required
+input files into place in the staging directory with a name of the form
+'job<JobID>_<FileName>' where <JobId> is the Id of the new Job, and FileName is
+the name of a file a Step depends on.
+
+=item *
+Once the Job set up is complete its Status is set to staging. This indicates
+that the TestBot Engine can retrieve the files it depends on from the staging
+directory and move them to the Job's directory.
+
+=item *
+Then the Status is set to queued which indicates the Job is ready to run.
=item *
As soon as one of the Step starts running, the Job's Status field is set to
@@ -93,7 +105,7 @@ sub InitializeNew($$)
my ($self, $Collection) = @_;
$self->Branch(CreateBranches()->GetDefaultBranch());
- $self->Status("queued");
+ $self->Status("new");
$self->Submitted(time());
$self->SUPER::InitializeNew($Collection);
@@ -446,7 +458,7 @@ my @PropertyDescriptors = (
CreateItemrefPropertyDescriptor("Branch", "Branch", !1, 1, \&CreateBranches, ["BranchName"]),
CreateItemrefPropertyDescriptor("User", "Author", !1, 1, \&CreateUsers, ["UserName"]),
CreateBasicPropertyDescriptor("Priority", "Priority", !1, 1, "N", 1),
- CreateEnumPropertyDescriptor("Status", "Status", !1, 1, ['queued', 'running', 'completed', 'badpatch', 'badbuild', 'boterror', 'canceled']),
+ CreateEnumPropertyDescriptor("Status", "Status", !1, 1, ['new', 'staging', 'queued', 'running', 'completed', 'badpatch', 'badbuild', 'boterror', 'canceled']),
CreateBasicPropertyDescriptor("Remarks", "Remarks", !1, !1, "A", 128),
CreateBasicPropertyDescriptor("Submitted", "Submitted", !1, !1, "DT", 19),
CreateBasicPropertyDescriptor("Ended", "Ended", !1, !1, "DT", 19),
diff --git a/testbot/lib/WineTestBot/Patches.pm b/testbot/lib/WineTestBot/Patches.pm
index d5be1b3..4f42813 100644
--- a/testbot/lib/WineTestBot/Patches.pm
+++ b/testbot/lib/WineTestBot/Patches.pm
@@ -166,6 +166,7 @@ sub Submit($$$)
# Create a new job for this patch
my $NewJob = $Jobs->Add();
+ $NewJob->Status("queued");
$NewJob->User($User);
$NewJob->Priority(6);
my $PropertyDescriptor = $Jobs->GetPropertyDescriptorByName("Remarks");
diff --git a/testbot/web/Submit.pl b/testbot/web/Submit.pl
index ae69e7c..8e22621 100644
--- a/testbot/web/Submit.pl
+++ b/testbot/web/Submit.pl
@@ -774,6 +774,7 @@ sub OnSubmit($)
# First create a new job
my $Jobs = CreateJobs();
my $NewJob = $Jobs->Add();
+ $NewJob->Status("queued");
$NewJob->User($self->GetCurrentSession()->User);
$NewJob->Priority(5);
if ($self->GetParam("Remarks"))
More information about the wine-cvs
mailing list