This allows the TestBot to start running the 64 bit tests even if there are still some 32 bit tests to run. It also lets the TestBot more freely schedule the tasks when a job involves many test units (and thus many steps).
Signed-off-by: Francois Gouget fgouget@codeweavers.com ---
This patch requires updating the database schema. So: * Apply testbot/ddl/update31.sql * Restart Engine.pl * Restart the web server.
testbot/ddl/update31.sql | 10 +++++ testbot/ddl/winetestbot.sql | 4 +- testbot/doc/winetestbot-schema.dia | 87 +++++++++++++++++++++++++++++++++++++- testbot/lib/WineTestBot/Jobs.pm | 82 +++++++++++++++++++++++++++-------- testbot/lib/WineTestBot/Patches.pm | 13 +++++- testbot/lib/WineTestBot/Steps.pm | 29 +++++++++++++ testbot/web/Submit.pl | 13 +++++- 7 files changed, 214 insertions(+), 24 deletions(-) create mode 100644 testbot/ddl/update31.sql
diff --git a/testbot/ddl/update31.sql b/testbot/ddl/update31.sql new file mode 100644 index 000000000..b0d1a0456 --- /dev/null +++ b/testbot/ddl/update31.sql @@ -0,0 +1,10 @@ +USE winetestbot; + +ALTER TABLE Steps + ADD PreviousNo INT(2) NULL + AFTER No, + ADD FOREIGN KEY (JobId, PreviousNo) REFERENCES Steps(JobId, No); + +UPDATE Steps + SET PreviousNo = 1 + WHERE No > 1; diff --git a/testbot/ddl/winetestbot.sql b/testbot/ddl/winetestbot.sql index cebc26556..41740159b 100644 --- a/testbot/ddl/winetestbot.sql +++ b/testbot/ddl/winetestbot.sql @@ -129,6 +129,7 @@ CREATE TABLE Steps ( JobId INT(5) NOT NULL, No INT(2) NOT NULL, + PreviousNo INT(2) NULL, Type ENUM('suite', 'single', 'build', 'reconfig') NOT NULL, Status ENUM('queued', 'running', 'completed', 'badpatch', 'badbuild', 'boterror', 'canceled', 'skipped') NOT NULL, FileName VARCHAR(100) NOT NULL, @@ -137,7 +138,8 @@ CREATE TABLE Steps DebugLevel INT(2) NOT NULL, ReportSuccessfulTests ENUM('Y', 'N') NOT NULL, PRIMARY KEY (JobId, No), - FOREIGN KEY (JobId) REFERENCES Jobs(Id) + FOREIGN KEY (JobId) REFERENCES Jobs(Id), + FOREIGN KEY (JobId, PreviousNo) REFERENCES Steps(JobId, No) ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
diff --git a/testbot/doc/winetestbot-schema.dia b/testbot/doc/winetestbot-schema.dia index 7a9483465..7d18b7271 100644 --- a/testbot/doc/winetestbot-schema.dia +++ b/testbot/doc/winetestbot-schema.dia @@ -1704,7 +1704,7 @@ <dia:point val="8.24583,-14.8834"/> </dia:attribute> <dia:attribute name="obj_bb"> - <dia:rectangle val="8.24583,-14.8834;22.2508,-6.2834"/> + <dia:rectangle val="8.24583,-14.8834;22.2508,-5.4834"/> </dia:attribute> <dia:attribute name="meta"> <dia:composite type="dict"/> @@ -1716,7 +1716,7 @@ <dia:real val="14.004999999999999"/> </dia:attribute> <dia:attribute name="elem_height"> - <dia:real val="8.5999999999999996"/> + <dia:real val="9.3999999999999986"/> </dia:attribute> <dia:attribute name="name"> dia:string#Steps#</dia:string> @@ -1783,6 +1783,29 @@ dia:string##</dia:string> </dia:attribute> </dia:composite> + <dia:composite type="table_attribute"> + <dia:attribute name="name"> + dia:string#Previous#</dia:string> + </dia:attribute> + <dia:attribute name="type"> + dia:string#INT(2)#</dia:string> + </dia:attribute> + <dia:attribute name="comment"> + dia:string##</dia:string> + </dia:attribute> + <dia:attribute name="primary_key"> + <dia:boolean val="false"/> + </dia:attribute> + <dia:attribute name="nullable"> + <dia:boolean val="true"/> + </dia:attribute> + <dia:attribute name="unique"> + <dia:boolean val="false"/> + </dia:attribute> + <dia:attribute name="default_value"> + dia:string##</dia:string> + </dia:attribute> + </dia:composite> <dia:composite type="table_attribute"> <dia:attribute name="name"> dia:string#Type#</dia:string> @@ -3556,5 +3579,65 @@ <dia:connection handle="1" to="O23" connection="12"/> </dia:connections> </dia:object> + <dia:object type="Database - Reference" version="0" id="O26"> + <dia:attribute name="obj_pos"> + <dia:point val="8.24583,-12.3834"/> + </dia:attribute> + <dia:attribute name="obj_bb"> + <dia:rectangle val="5.87763,-13.0334;8.29583,-11.5334"/> + </dia:attribute> + <dia:attribute name="meta"> + <dia:composite type="dict"/> + </dia:attribute> + <dia:attribute name="orth_points"> + <dia:point val="8.24583,-12.3834"/> + <dia:point val="5.92763,-12.3834"/> + <dia:point val="5.92763,-11.5834"/> + <dia:point val="8.24583,-11.5834"/> + </dia:attribute> + <dia:attribute name="orth_orient"> + <dia:enum val="0"/> + <dia:enum val="1"/> + <dia:enum val="0"/> + </dia:attribute> + <dia:attribute name="orth_autoroute"> + <dia:boolean val="false"/> + </dia:attribute> + <dia:attribute name="text_colour"> + <dia:color val="#000000ff"/> + </dia:attribute> + <dia:attribute name="line_colour"> + <dia:color val="#000000ff"/> + </dia:attribute> + <dia:attribute name="line_width"> + <dia:real val="0.10000000000000001"/> + </dia:attribute> + <dia:attribute name="line_style"> + <dia:enum val="0"/> + <dia:real val="1"/> + </dia:attribute> + <dia:attribute name="corner_radius"> + <dia:real val="0"/> + </dia:attribute> + <dia:attribute name="end_arrow"> + <dia:enum val="0"/> + </dia:attribute> + <dia:attribute name="start_point_desc"> + dia:string#0..1#</dia:string> + </dia:attribute> + <dia:attribute name="end_point_desc"> + dia:string#0..n#</dia:string> + </dia:attribute> + <dia:attribute name="normal_font"> + <dia:font family="monospace" style="0" name="Courier"/> + </dia:attribute> + <dia:attribute name="normal_font_height"> + <dia:real val="0.59999999999999998"/> + </dia:attribute> + dia:connections + <dia:connection handle="0" to="O11" connection="14"/> + <dia:connection handle="1" to="O11" connection="16"/> + </dia:connections> + </dia:object> </dia:layer> </dia:diagram> diff --git a/testbot/lib/WineTestBot/Jobs.pm b/testbot/lib/WineTestBot/Jobs.pm index 3eba27bc2..eea16864e 100644 --- a/testbot/lib/WineTestBot/Jobs.pm +++ b/testbot/lib/WineTestBot/Jobs.pm @@ -84,6 +84,28 @@ sub InitializeNew($$) $self->SUPER::InitializeNew($Collection); }
+=pod +=over 12 + +=item C<OnDelete()> + +Resets the Steps PreviousNo fields because the corresponding foreign key +references both this Job and the Steps, thus preventing their deletion. + +=back +=cut + +sub OnDelete($) +{ + my ($self) = @_; + + my $Steps = $self->Steps; + map { $_->PreviousNo(undef) } @{$Steps->GetItems()}; + my ($_ErrProperty, $ErrMessage) = $Steps->Save(); + + return $ErrMessage || $self->SUPER::OnDelete(); +} + sub GetDir($) { my ($self) = @_; @@ -843,20 +865,45 @@ sub _ScheduleTasks($) { $JobRank++;
- # The list of VMs that should be getting ready to run + # The per-step lists of VMs that should be getting ready to run # before we prepare the next step - my $PreviousVMs = []; + my %StepVMs = ("" => []); # no dependency for the first step
- my $StepRank = 0; + # Process the steps in increasing $Step->No order for the inter-step + # dependencies my $Steps = $Job->Steps; $Steps->AddFilter("Status", ["queued", "running"]); foreach my $Step (sort { $a->No <=> $b->No } @{$Steps->GetItems()}) { - # StepRank 0 contains the runnable tasks, 1 the 'may soon be runnable' - # ones, and 2 and greater tasks we don't care about yet - $Step->HandleStaging() if ($StepRank == 0 and $Step->Status eq "queued"); - - my $StepVMs = []; + my $StepRank; + my $Previous = ""; # Avoid undefined values for hash indices + if (!$Step->PreviousNo) + { + # The first step may need to get files from the staging area + $Step->HandleStaging() if ($Step->Status eq "queued"); + $StepRank = 0; + $StepVMs{$Step} = []; + } + else + { + $Previous = $Steps->GetItem($Step->PreviousNo); + if ($Previous->Status eq "completed") + { + # The previous step was successful so we can now run this one + $StepRank = 0; + $StepVMs{$Step} = []; + } + elsif ($StepVMs{$Previous}) + { + # The previous step is almost done. Prepare this one. + $StepRank = 1; + } + else + { + # The previous step is nowhere near done + $StepRank = 2; + } + }
my $Tasks = $Step->Tasks; $Tasks->AddFilter("Status", ["queued"]); @@ -872,17 +919,18 @@ sub _ScheduleTasks($) $Host->{queued}++; $Sched->{queued}++;
- if ($StepRank >= 2 or !$PreviousVMs) + if ($StepRank >= 2) { # The previous step is nowhere near done so skip this one for now next; } if ($StepRank == 1) { - # Passing $PreviousVMs ensures this VM will be reverted if and only - # if all of the previous step's tasks are about to run. + # Passing $StepVMs{$Previous} ensures this VM will be reverted + # if and only if all of the previous step's tasks are about to run. # See _HasMissingDependencies(). - _AddNeededVM($NeededVMs, $VM, $NEXT_BASE + $JobRank, $PreviousVMs); + _AddNeededVM($NeededVMs, $VM, $NEXT_BASE + $JobRank, + $StepVMs{$Previous}); next; } $Sched->{runnable}++; # $StepRank == 0 @@ -893,12 +941,12 @@ sub _ScheduleTasks($) # scheduled to be reverted for a task with a higher priority. # So this task won't be run before a while and thus there is # no point in preparing the next step. - $StepVMs = undef; + $StepVMs{$Step} = undef; next; }
# It's not worth preparing the next step for tasks that take so long - $StepVMs = undef if ($Task->Timeout > $BuildTimeout); + $StepVMs{$Step} = undef if ($Task->Timeout > $BuildTimeout);
my $VMKey = $VM->GetKey(); if ($VM->Status eq "idle") @@ -939,12 +987,10 @@ sub _ScheduleTasks($) { # We cannot use the VM because it is busy (running another task, # offline, etc.). So it is too early to prepare the next step. - $StepVMs = undef; + $StepVMs{$Step} = undef; } - push @$StepVMs, $VM if ($StepVMs); + push @{$StepVMs{$Step}}, $VM if ($StepVMs{$Step}); } - $PreviousVMs = $StepVMs; - $StepRank++; } }
diff --git a/testbot/lib/WineTestBot/Patches.pm b/testbot/lib/WineTestBot/Patches.pm index 42118e614..d572ec470 100644 --- a/testbot/lib/WineTestBot/Patches.pm +++ b/testbot/lib/WineTestBot/Patches.pm @@ -281,7 +281,15 @@ sub Submit($$$) my $Task = $NewStep->Tasks->Add(); $Task->VM($BuildVM); $Task->Timeout($BuildTimeout); - + + # Save this step (&job+task) so the others can reference it + my ($ErrKey, $ErrProperty, $ErrMessage) = $Jobs->Save(); + if (defined($ErrMessage)) + { + $self->Disposition("Failed to submit build step"); + return $ErrMessage; + } + foreach my $Unit (keys %{$Modules{$Module}}) { # Add 32 and 64-bit tasks @@ -294,6 +302,7 @@ sub Submit($$$) { # Create the corresponding Step $NewStep = $Steps->Add(); + $NewStep->PreviousNo(1); my $FileName = $Module; $FileName .= ".exe" if ($Modules{$Module}{$Unit} eq "patchprograms"); $FileName .= "_test"; @@ -317,7 +326,7 @@ sub Submit($$$) } }
- my ($ErrKey, $ErrProperty, $ErrMessage) = $Jobs->Save(); + ($ErrKey, $ErrProperty, $ErrMessage) = $Jobs->Save(); if (defined($ErrMessage)) { $self->Disposition("Failed to submit job"); diff --git a/testbot/lib/WineTestBot/Steps.pm b/testbot/lib/WineTestBot/Steps.pm index 95c0a45aa..45ec6359f 100644 --- a/testbot/lib/WineTestBot/Steps.pm +++ b/testbot/lib/WineTestBot/Steps.pm @@ -30,6 +30,10 @@ A Job is composed of multiple Steps that each do a specific operation: build the test executable, or run a given test, etc. A Step is in turn composed of a WineTestBot::Task object for each VM it should be run on.
+Note that the PreviousNo relation will prevent the deletion of the target Step. +It is the responsibility of the caller to delete the Steps in a suitable order, +or to reset their PreviousNo fields beforehand. + =cut
use File::Copy; @@ -61,6 +65,30 @@ sub InitializeNew($$) $self->SUPER::InitializeNew($Collection); }
+=pod +=over 12 + +=item C<Validate()> + +Enforces strict ordering to avoid loops. + +Note that a side effect is that processing steps in increasing step number +order is sufficient to ensure the dependencies are processed first. + +=back +=cut + +sub Validate($) +{ + my ($self) = @_; + + if ($self->PreviousNo and $self->PreviousNo >= $self->No) + { + return ("PreviousNo", "The previous step number must be less than this one's."); + } + return $self->SUPER::Validate(); +} + sub GetDir($) { my ($self) = @_; @@ -178,6 +206,7 @@ BEGIN { @PropertyDescriptors = ( CreateBasicPropertyDescriptor("No", "Step no", 1, 1, "N", 2), + CreateBasicPropertyDescriptor("PreviousNo", "Previous step", !1, !1, "N", 2), CreateEnumPropertyDescriptor("Status", "Status", !1, 1, ['queued', 'running', 'completed', 'badpatch', 'badbuild', 'boterror', 'canceled', 'skipped']), CreateEnumPropertyDescriptor("Type", "Step type", !1, 1, ['suite', 'single', 'build', 'reconfig']), CreateBasicPropertyDescriptor("FileName", "File name", !1, 1, "A", 100), diff --git a/testbot/web/Submit.pl b/testbot/web/Submit.pl index 0272a283d..875384ea3 100644 --- a/testbot/web/Submit.pl +++ b/testbot/web/Submit.pl @@ -834,6 +834,7 @@ sub OnSubmit($) my $Steps = $NewJob->Steps;
my $FileType = $self->GetParam("FileType"); + my $BuildStepNo; if ($FileType eq "patchdlls" || $FileType eq "patchprograms") { # This is a patch so add a build step... @@ -852,6 +853,15 @@ sub OnSubmit($) my $Task = $BuildStep->Tasks->Add(); $Task->VM($BuildVM); $Task->Timeout($BuildTimeout); + + # Save this step (&job+task) so the others can reference it + my ($ErrKey, $ErrProperty, $ErrMessage) = $Jobs->Save(); + if (defined($ErrMessage)) + { + $self->{ErrMessage} = $ErrMessage; + return !1; + } + $BuildStepNo = 1; }
# Add steps and tasks for the 32 and 64-bit tests @@ -875,6 +885,7 @@ sub OnSubmit($) { # First create the test step my $TestStep = $Steps->Add(); + $TestStep->PreviousNo($BuildStepNo); if ($FileType eq "patchdlls" || $FileType eq "patchprograms") { my $FileName=$self->GetParam("TestExecutable"); @@ -905,7 +916,7 @@ sub OnSubmit($) } }
- # Now save the whole thing + # Now save the whole thing (or whatever's left to save) my ($ErrKey, $ErrProperty, $ErrMessage) = $Jobs->Save(); if (defined($ErrMessage)) {