From c37696a5a770954f535fc59b7d368b2ff2769df7 Mon Sep 17 00:00:00 2001 From: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com> Date: Thu, 13 Jun 2024 10:50:19 +0200 Subject: [PATCH] fix: check plan.IsTerminated on IsPlanRunning (#432) --------- Signed-off-by: ThibaultFy --- changes/432.changed | 1 + changes/432.fixed | 1 + e2e/computeplan_test.go | 10 +++++++--- lib/persistence/computeplan_dbal.go | 2 +- lib/service/computeplan.go | 7 ++++++- lib/service/computetaskstate.go | 14 +------------- server/standalone/dbal/computeplan.go | 2 +- server/standalone/dbal/computeplan_test.go | 2 +- 8 files changed, 19 insertions(+), 20 deletions(-) create mode 100644 changes/432.changed create mode 100644 changes/432.fixed diff --git a/changes/432.changed b/changes/432.changed new file mode 100644 index 00000000..47e49483 --- /dev/null +++ b/changes/432.changed @@ -0,0 +1 @@ +Rename `ComputePlanDBAL().IsPlanRunning(key)` to `ComputePlanDBAL().ArePlanTaksRunning(key)` \ No newline at end of file diff --git a/changes/432.fixed b/changes/432.fixed new file mode 100644 index 00000000..a5d83811 --- /dev/null +++ b/changes/432.fixed @@ -0,0 +1 @@ +`IsPlanRunning` checks if a cancel of failure date is set before checking the tasks status. \ No newline at end of file diff --git a/e2e/computeplan_test.go b/e2e/computeplan_test.go index 366fec6b..c761eda6 100644 --- a/e2e/computeplan_test.go +++ b/e2e/computeplan_test.go @@ -703,10 +703,14 @@ func TestIsPlanRunning(t *testing.T) { require.Nil(t, err) resp = appClient.IsPlanRunning("cp2") - require.True(t, resp.IsRunning) + require.False(t, resp.IsRunning) - appClient.CancelTask("task-cp2") + appClient.RegisterComputePlan(client.DefaultComputePlanOptions().WithKeyRef("cp3")) + appClient.RegisterTasks(client.DefaultTrainTaskOptions().WithPlanRef("cp3").WithKeyRef("task-cp3")) + appClient.StartTask("task-cp3") - resp = appClient.IsPlanRunning("cp2") + appClient.CancelTask("task-cp3") + + resp = appClient.IsPlanRunning("cp3") require.False(t, resp.IsRunning) } diff --git a/lib/persistence/computeplan_dbal.go b/lib/persistence/computeplan_dbal.go index e5ce8f2e..e16542f0 100644 --- a/lib/persistence/computeplan_dbal.go +++ b/lib/persistence/computeplan_dbal.go @@ -15,7 +15,7 @@ type ComputePlanDBAL interface { SetComputePlanName(plan *asset.ComputePlan, name string) error CancelComputePlan(plan *asset.ComputePlan, cancelationDate time.Time) error FailComputePlan(plan *asset.ComputePlan, failureDate time.Time) error - IsPlanRunning(key string) (bool, error) + ArePlanTasksRunning(key string) (bool, error) } type ComputePlanDBALProvider interface { diff --git a/lib/service/computeplan.go b/lib/service/computeplan.go index ff78440d..a0e234c9 100644 --- a/lib/service/computeplan.go +++ b/lib/service/computeplan.go @@ -190,5 +190,10 @@ func (s *ComputePlanService) computePlanExists(key string) (bool, error) { // IsPlanRunning indicates whether there are tasks belonging to the compute plan // being executed or waiting to be executed func (s *ComputePlanService) IsPlanRunning(key string) (bool, error) { - return s.GetComputePlanDBAL().IsPlanRunning(key) + plan, err := s.GetPlan(key) + if plan.IsTerminated() { + return false, err + } else { + return s.GetComputePlanDBAL().ArePlanTasksRunning(key) + } } diff --git a/lib/service/computetaskstate.go b/lib/service/computetaskstate.go index 3f826c78..b0031bcd 100644 --- a/lib/service/computetaskstate.go +++ b/lib/service/computetaskstate.go @@ -135,19 +135,7 @@ func (s *ComputeTaskService) applyTaskAction(task *asset.ComputeTask, action ass case asset.ComputeTaskAction_TASK_ACTION_FAILED: transition = transitionFailed case asset.ComputeTaskAction_TASK_ACTION_DONE: - if task.ComputePlanKey != "" { - plan, err := s.GetComputePlanService().GetPlan(task.ComputePlanKey) - if err != nil { - return err - } - if plan.IsTerminated() { - transition = transitionCanceled - } else { - transition = transitionDone - } - } else { - transition = transitionDone - } + transition = transitionDone case asset.ComputeTaskAction_TASK_ACTION_BUILD_STARTED: transition = transitionBuilding case asset.ComputeTaskAction_TASK_ACTION_BUILD_FINISHED: diff --git a/server/standalone/dbal/computeplan.go b/server/standalone/dbal/computeplan.go index 72748fc3..4c669d69 100644 --- a/server/standalone/dbal/computeplan.go +++ b/server/standalone/dbal/computeplan.go @@ -173,7 +173,7 @@ func (d *DBAL) FailComputePlan(plan *asset.ComputePlan, failureDate time.Time) e return d.updateComputePlan(plan.Key, "failure_date", failureDate) } -func (d *DBAL) IsPlanRunning(key string) (bool, error) { +func (d *DBAL) ArePlanTasksRunning(key string) (bool, error) { stmt := getStatementBuilder(). Select("status", "COUNT(status)"). diff --git a/server/standalone/dbal/computeplan_test.go b/server/standalone/dbal/computeplan_test.go index f60c95e5..afd6243f 100644 --- a/server/standalone/dbal/computeplan_test.go +++ b/server/standalone/dbal/computeplan_test.go @@ -167,7 +167,7 @@ func TestIsPlanRunning(t *testing.T) { dbal := &DBAL{ctx: context.TODO(), tx: tx, channel: testChannel} - isRunning, err := dbal.IsPlanRunning(cpKey) + isRunning, err := dbal.ArePlanTasksRunning(cpKey) assert.NoError(t, err) assert.True(t, isRunning)