[tools 1/2] testbot: Unify the Job cancel and restart actions.
Most of the code can be shared and this simplifies expanding the set of job control actions. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- testbot/bin/Engine.pl | 42 +++------ testbot/lib/WineTestBot/Engine/Notify.pm | 29 ++---- testbot/web/JobDetails.pl | 107 ++++++----------------- 3 files changed, 43 insertions(+), 135 deletions(-) diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl index 7880b8e80..da72ffb47 100755 --- a/testbot/bin/Engine.pl +++ b/testbot/bin/Engine.pl @@ -329,50 +329,31 @@ sub HandleJobStatusChange($$$) return "1OK"; } -sub HandleJobCancel($) +sub HandleJobControl($) { - my ($JobKey) = @_; + my ($Action, $JobKey) = @_; - my $Job = CreateJobs()->GetItem($JobKey); - if (! $Job) + if ($Action !~ /^(cancel|restart)$/) { - LogMsg "JobCancel for nonexistent job $JobKey\n"; - return "0Job $JobKey not found"; + LogMsg "Invalid $Action JobControl command\n"; + return "0Invalid $Action JobControl command"; } - # We've already determined that JobKey is valid, untaint it - $JobKey =~ m/^(.*)$/; - $JobKey = $1; - - my $ErrMessage = $Job->Cancel(); - if (defined($ErrMessage)) - { - LogMsg "Cancel problem: $ErrMessage\n"; - return "0$ErrMessage"; - } - - ScheduleJobs(); - - return "1OK"; -} - -sub HandleJobRestart($) -{ - my ($JobKey) = @_; + $Action = $1; my $Job = CreateJobs()->GetItem($JobKey); - if (! $Job) + if (!$Job) { - LogMsg "JobRestart for nonexistent job $JobKey\n"; + LogMsg "$Action JobControl for nonexistent job $JobKey\n"; return "0Job $JobKey not found"; } # We've already determined that JobKey is valid, untaint it $JobKey =~ m/^(.*)$/; $JobKey = $1; - my $ErrMessage = $Job->Restart(); + my $ErrMessage = $Action eq "cancel" ? $Job->Cancel() : $Job->Restart(); if (defined($ErrMessage)) { - LogMsg "Restart problem: $ErrMessage\n"; + LogMsg "$Action problem: $ErrMessage\n"; return "0$ErrMessage"; } @@ -567,8 +548,7 @@ sub HandleGetScreenshot($) my %Handlers=( "getscreenshot" => \&HandleGetScreenshot, - "jobcancel" => \&HandleJobCancel, - "jobrestart" => \&HandleJobRestart, + "jobcontrol" => \&HandleJobControl, "jobstatuschange" => \&HandleJobStatusChange, "ping" => \&HandlePing, "shutdown" => \&HandleShutdown, diff --git a/testbot/lib/WineTestBot/Engine/Notify.pm b/testbot/lib/WineTestBot/Engine/Notify.pm index de7342564..82221c9ed 100644 --- a/testbot/lib/WineTestBot/Engine/Notify.pm +++ b/testbot/lib/WineTestBot/Engine/Notify.pm @@ -29,8 +29,8 @@ WineTestBot::Engine::Notify - Engine notification use Exporter 'import'; our $RunningInEngine; -our @EXPORT = qw(Shutdown PingEngine JobStatusChange JobCancel - JobRestart RescheduleJobs VMStatusChange +our @EXPORT = qw(Shutdown PingEngine JobStatusChange JobControl + RescheduleJobs VMStatusChange WinePatchMLSubmission WinePatchWebSubmission GetScreenshot); our @EXPORT_OK = qw($RunningInEngine); @@ -115,31 +115,14 @@ sub JobStatusChange($$$) return substr($Reply, 1); } -sub JobCancel($) +sub JobControl($$) { - my ($JobKey) = @_; + my ($Action, $JobKey) = @_; - my $Reply = SendCmdReceiveReply("jobcancel $JobKey\n"); + my $Reply = SendCmdReceiveReply("jobcontrol $Action $JobKey\n"); if (length($Reply) < 1) { - return "The Engine did not acknowledge the JobCancel command"; - } - if (substr($Reply, 0, 1) eq "1") - { - return undef; - } - - return substr($Reply, 1); -} - -sub JobRestart($) -{ - my ($JobKey) = @_; - - my $Reply = SendCmdReceiveReply("jobrestart $JobKey\n"); - if (length($Reply) < 1) - { - return "The Engine did not acknowledge the JobRestart command"; + return "The Engine did not acknowledge the $Action JobControl command"; } if (substr($Reply, 0, 1) eq "1") { diff --git a/testbot/web/JobDetails.pl b/testbot/web/JobDetails.pl index 99c68bcb8..87a56941e 100644 --- a/testbot/web/JobDetails.pl +++ b/testbot/web/JobDetails.pl @@ -158,73 +158,33 @@ sub GetItemActions($) # Actions handling # -sub GetActions($) +sub CanDoJobControl($$) { - my ($self) = @_; - - # These are mutually exclusive - return ["Cancel job"] if (!defined $self->CanCancel()); - return ["Restart job"] if (!defined $self->CanRestart()); - return []; -} - -sub CanCancel($) -{ - my ($self) = @_; + my ($self, $Action) = @_; my $Job = $self->{EnclosingPage}->GetJob(); - my $Status = $Job->Status; - if ($Status ne "queued" && $Status ne "running") - { - return "Job already $Status"; - } - my $Session = $self->{EnclosingPage}->GetCurrentSession(); - if (! defined($Session)) + my $CurrentUser = $Session->User if (defined $Session); + if (!$CurrentUser or + (!$CurrentUser->HasRole("admin") and + $Job->User->GetKey() ne $CurrentUser->GetKey())) { - return "You are not authorized to cancel this job"; - } - my $CurrentUser = $Session->User; - if (! $CurrentUser->HasRole("admin") && - $Job->User->GetKey() ne $CurrentUser->GetKey()) - { - return "You are not authorized to cancel this job"; + return "You are not authorized to $Action this job"; } - return undef; + my %AllowedStatus = ( + "cancel" => {"queued" => 1, "running" => 1}, + "restart" => {"boterror" => 1, "canceled" => 1}, + ); + return $AllowedStatus{$Action}->{$Job->Status} ? undef : + "Cannot $Action a ". $Job->Status ." job"; } -sub CanRestart($) +sub OnJobControl($$) { - my ($self) = @_; - - my $Job = $self->{EnclosingPage}->GetJob(); - my $Status = $Job->Status; - if ($Status ne "boterror" && $Status ne "canceled") - { - return "Not a failed / canceled Job"; - } - - my $Session = $self->{EnclosingPage}->GetCurrentSession(); - if (! defined($Session)) - { - return "You are not authorized to restart this job"; - } - my $CurrentUser = $Session->User; - if (! $CurrentUser->HasRole("admin") && - $Job->User->GetKey() ne $CurrentUser->GetKey()) # FIXME: Admin only? - { - return "You are not authorized to restart this job"; - } - - return undef; -} - -sub OnCancel($) -{ - my ($self) = @_; + my ($self, $Action) = @_; - my $ErrMessage = $self->CanCancel(); + my $ErrMessage = $self->CanDoJobControl($Action); if (defined $ErrMessage) { $self->{EnclosingPage}->SetError(undef, $ErrMessage); @@ -232,7 +192,7 @@ sub OnCancel($) } my $JobId = $self->{EnclosingPage}->GetJob()->Id; - $ErrMessage = JobCancel($JobId); + $ErrMessage = JobControl($Action, $JobId); if (defined $ErrMessage) { $self->{EnclosingPage}->SetError(undef, $ErrMessage); @@ -240,43 +200,28 @@ sub OnCancel($) } # Ideally this would use something like GetMoreInfoLink() to rebuild a Get - # URL to preserve which logs and screenshots have been expanded. But the - # anchor would likely be lost anyway. + # URL to preserve which logs and screenshots have been expanded. But if the + # job got restarted those would be gone anyway. # So just do a basic reload to refresh the tasks' status. exit($self->{EnclosingPage}->Redirect($ENV{"SCRIPT_NAME"} ."?Key=$JobId")); } -sub OnRestart($) +sub GetActions($) { my ($self) = @_; - my $ErrMessage = $self->CanRestart(); - if (defined $ErrMessage) - { - $self->{EnclosingPage}->SetError(undef, $ErrMessage); - return !1; - } - - my $JobId = $self->{EnclosingPage}->GetJob()->Id; - $ErrMessage = JobRestart($JobId); - if (defined $ErrMessage) - { - $self->{EnclosingPage}->SetError(undef, $ErrMessage); - return !1; - } - - # Reload to refresh the tasks' status. Note that all logs and screenshots - # have been cleared so there is no point specifying which one to open or - # adding an anchor. - exit($self->{EnclosingPage}->Redirect($ENV{"SCRIPT_NAME"} ."?Key=$JobId")); + # These are mutually exclusive + return ["Cancel job"] if (!defined $self->CanDoJobControl("cancel")); + return ["Restart job"] if (!defined $self->CanDoJobControl("restart")); + return []; } sub OnAction($$) { my ($self, $Action) = @_; - return $Action eq "Cancel job" ? $self->OnCancel() : - $Action eq "Restart job" ? $self->OnRestart() : + return $Action eq "Cancel job" ? $self->OnJobControl("cancel") : + $Action eq "Restart job" ? $self->OnJobControl("restart") : $self->SUPER::OnAction($Action); } -- 2.30.2
This renames the 'cancel' action to 'stop' since it's reversible now. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- Naming is a bit tricky: * Cancel implies too much that the job cannot be resumed. * Pause implies too much that this is a temporary state and that the job is meant to be resumed eventually. That's not the case: once stopped it's considered to be "done" and will be garbage collected after a while. * So I settled on Stop. This can be used to let other jobs run for instance, and what's really new is that one can then resume the job wherease before one had to restart it from scratch (which is still an option). --- testbot/bin/Engine.pl | 6 ++- testbot/lib/WineTestBot/Jobs.pm | 65 +++++++++++++++++++++++++++++++-- testbot/web/JobDetails.pl | 15 +++++--- 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl index da72ffb47..88852730e 100755 --- a/testbot/bin/Engine.pl +++ b/testbot/bin/Engine.pl @@ -333,7 +333,7 @@ sub HandleJobControl($) { my ($Action, $JobKey) = @_; - if ($Action !~ /^(cancel|restart)$/) + if ($Action !~ /^(stop|resume|restart)$/) { LogMsg "Invalid $Action JobControl command\n"; return "0Invalid $Action JobControl command"; @@ -350,7 +350,9 @@ sub HandleJobControl($) $JobKey =~ m/^(.*)$/; $JobKey = $1; - my $ErrMessage = $Action eq "cancel" ? $Job->Cancel() : $Job->Restart(); + my $ErrMessage = $Action eq "stop" ? $Job->Stop() : + $Action eq "resume" ? $Job->Resume() : + $Job->Restart(); if (defined($ErrMessage)) { LogMsg "$Action problem: $ErrMessage\n"; diff --git a/testbot/lib/WineTestBot/Jobs.pm b/testbot/lib/WineTestBot/Jobs.pm index c082a36ab..e4d76d356 100644 --- a/testbot/lib/WineTestBot/Jobs.pm +++ b/testbot/lib/WineTestBot/Jobs.pm @@ -75,7 +75,7 @@ Once all the Steps have completed the Job's Status is updated to reflect the overall result: completed, badpatch, etc. =item * -If the Job is canceled by the user, then the Status field is set to canceled. +If the Job is stopped by the user, then the Status field is set to canceled. =back @@ -280,9 +280,9 @@ sub UpdateStatus($) =pod =over 12 -=item C<Cancel()> +=item C<Stop()> -Cancels the Job, preserving existing results. +Stops the Job, preserving existing results. More precisely, goes through all of that Job's 'queued' and 'running' tasks, killing all the running ones and marking them, and all the queued tasks, as @@ -294,7 +294,7 @@ Returns undef if successful, the error message otherwise. =back =cut -sub Cancel($) +sub Stop($) { my ($self) = @_; my $ErrMessage; @@ -339,6 +339,63 @@ sub Cancel($) =pod =over 12 +=item C<Resume()> + +Resume running the Job. + +More precisely, marks the 'skipped' and 'canceled' tasks as 'queued' +again and updates the job status accordingly. This preserves existing +results unlike Restart(). + +Returns undef if successful, the error message otherwise. + +=back +=cut + +sub Resume($) +{ + my ($self) = @_; + + if ($self->Status ne "canceled") + { + return "The job has not been stopped and is currently ". $self->Status; + } + + my $Steps = $self->Steps; + $Steps->AddFilter("Status", ["canceled", "skipped"]); + foreach my $Step (@{$Steps->GetSortedItems()}) + { + if ($Step->PreviousNo) + { + my $PrevStatus = $Steps->GetItem($Step->PreviousNo)->Status; + if ($PrevStatus ne "queued" && $PrevStatus ne "running" && + $PrevStatus ne "completed") + { + # The previous step was supposed to provide binaries but it failed. + # So this one cannot resume. + continue; + } + } + + my $Tasks = $Step->Tasks; + $Tasks->AddFilter("Status", ["canceled", "skipped"]); + foreach my $Task (@{$Tasks->GetItems()}) + { + $Task->Status("queued"); + $Task->Started(undef); + } + $Step->Status("queued"); + } + # Mark the job as queued and let UpdateStatus() set it to running if + # appropriate. + $self->Status("queued"); + $self->UpdateStatus(); + return undef; +} + +=pod +=over 12 + =item C<Restart()> Restarts the Job from scratch. diff --git a/testbot/web/JobDetails.pl b/testbot/web/JobDetails.pl index 87a56941e..329ab150c 100644 --- a/testbot/web/JobDetails.pl +++ b/testbot/web/JobDetails.pl @@ -173,7 +173,8 @@ sub CanDoJobControl($$) } my %AllowedStatus = ( - "cancel" => {"queued" => 1, "running" => 1}, + "stop" => {"queued" => 1, "running" => 1}, + "resume" => {"canceled" => 1}, "restart" => {"boterror" => 1, "canceled" => 1}, ); return $AllowedStatus{$Action}->{$Job->Status} ? undef : @@ -210,17 +211,19 @@ sub GetActions($) { my ($self) = @_; - # These are mutually exclusive - return ["Cancel job"] if (!defined $self->CanDoJobControl("cancel")); - return ["Restart job"] if (!defined $self->CanDoJobControl("restart")); - return []; + my @Actions; + push @Actions, "Stop job" if (!$self->CanDoJobControl("stop")); + push @Actions, "Resume job" if (!$self->CanDoJobControl("resume")); + push @Actions, "Restart job" if (!$self->CanDoJobControl("restart")); + return \@Actions; } sub OnAction($$) { my ($self, $Action) = @_; - return $Action eq "Cancel job" ? $self->OnJobControl("cancel") : + return $Action eq "Stop job" ? $self->OnJobControl("stop") : + $Action eq "Resume job" ? $self->OnJobControl("resume") : $Action eq "Restart job" ? $self->OnJobControl("restart") : $self->SUPER::OnAction($Action); } -- 2.30.2
participants (1)
-
Francois Gouget