From 02f7721d72eeff328725d2d3ad8b870dbf39fbc4 Mon Sep 17 00:00:00 2001 From: Romain Beuque <556072+rbeuque74@users.noreply.github.com> Date: Fri, 14 Oct 2022 11:19:55 +0200 Subject: [PATCH] fix(subtask): do not auto-resume parent task of subtask if parent resolution is paused (#367) * fix(subtask): do not auto-resume parent task of subtask if parent resolution is paused Most of the time, parent resolution in state Paused means that an human operator has stopped the processing for patch management. We should not auto-resume the parent task to prevent any issue. Signed-off-by: Romain Beuque <556072+rbeuque74@users.noreply.github.com> * fix(): moved task.ShouldResumeParentTask to taskutils to prevent cyclic-imports Signed-off-by: Romain Beuque <556072+rbeuque74@users.noreply.github.com> * fix(): make tests pass Signed-off-by: Romain Beuque <556072+rbeuque74@users.noreply.github.com> --- api/handler/task.go | 2 +- engine/engine.go | 3 +- engine/engine_test.go | 64 +++++++++++++++++++++++++++++++++----- models/task/task.go | 33 -------------------- pkg/taskutils/taskutils.go | 43 +++++++++++++++++++++++++ 5 files changed, 102 insertions(+), 43 deletions(-) diff --git a/api/handler/task.go b/api/handler/task.go index 0bd6e29c..6c53c7a4 100644 --- a/api/handler/task.go +++ b/api/handler/task.go @@ -464,7 +464,7 @@ func WontfixTask(c *gin.Context, in *wontfixTaskIn) error { return err } - parentTask, err := t.ShouldResumeParentTask(dbp) + parentTask, err := taskutils.ShouldResumeParentTask(dbp, t) if err == nil && parentTask != nil { go func() { logrus.Debugf("resuming parent task %q resolution %q", parentTask.PublicID, *parentTask.Resolution) diff --git a/engine/engine.go b/engine/engine.go index b26ee844..42bfd13b 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -29,6 +29,7 @@ import ( "github.com/ovh/utask/pkg/jsonschema" "github.com/ovh/utask/pkg/metadata" "github.com/ovh/utask/pkg/now" + "github.com/ovh/utask/pkg/taskutils" "github.com/ovh/utask/pkg/utils" ) @@ -592,7 +593,7 @@ forLoop: } func resumeParentTask(dbp zesty.DBProvider, currentTask *task.Task, sm *semaphore.Weighted, debugLogger *logrus.Entry) error { - parentTask, err := currentTask.ShouldResumeParentTask(dbp) + parentTask, err := taskutils.ShouldResumeParentTask(dbp, currentTask) if err != nil { return err } diff --git a/engine/engine_test.go b/engine/engine_test.go index b227ffc6..aa638e92 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -40,6 +40,7 @@ import ( "github.com/ovh/utask/pkg/plugins/builtin/echo" "github.com/ovh/utask/pkg/plugins/builtin/script" pluginsubtask "github.com/ovh/utask/pkg/plugins/builtin/subtask" + "github.com/ovh/utask/pkg/taskutils" ) const ( @@ -170,6 +171,7 @@ func templateFromYAML(dbp zesty.DBProvider, filename string) (*tasktemplate.Task if err := tmpl.Valid(); err != nil { return nil, err } + tmpl.Normalize() if err := dbp.DB().Insert(&tmpl); err != nil { intErr := pgjuju.Interpret(err) if !errors.IsAlreadyExists(intErr) { @@ -1151,13 +1153,7 @@ func TestResolveSubTask(t *testing.T) { dbp, err := zesty.NewDBProvider(utask.DBName) require.Nil(t, err) - tt, err := templateFromYAML(dbp, "variables.yaml") - require.Nil(t, err) - tt.Normalize() - assert.Equal(t, "variableeval", tt.Name) - require.Nil(t, tt.Valid()) - - err = dbp.DB().Insert(tt) + _, err = templateFromYAML(dbp, "variables.yaml") require.Nil(t, err) res, err := createResolution("subtask.yaml", map[string]interface{}{}, nil) @@ -1185,6 +1181,14 @@ func TestResolveSubTask(t *testing.T) { assert.Equal(t, step.StateDone, v.State, "not valid state for step %s", k) } + subtask, err = task.LoadFromPublicID(dbp, subtaskPublicID) + require.Nil(t, err) + assert.Equal(t, task.StateDone, subtask.State) + parentTaskToResume, err := taskutils.ShouldResumeParentTask(dbp, subtask) + require.Nil(t, err) + require.NotNil(t, parentTaskToResume) + assert.Equal(t, res.TaskID, parentTaskToResume.ID) + // checking if the parent task is picked up after that the subtask is resolved. // need to sleep a bit because the parent task is resumed asynchronously ti := time.Second @@ -1192,7 +1196,7 @@ func TestResolveSubTask(t *testing.T) { for i < ti { res, err = resolution.LoadFromPublicID(dbp, res.PublicID) require.Nil(t, err) - if res.State != resolution.StateError { + if res.State != resolution.StateWaiting { break } @@ -1216,6 +1220,50 @@ func TestResolveSubTask(t *testing.T) { assert.Equal(t, resolution.StateDone, res.State) } +func TestResolveSubTaskParentTaskPaused(t *testing.T) { + dbp, err := zesty.NewDBProvider(utask.DBName) + require.Nil(t, err) + + _, err = templateFromYAML(dbp, "variables.yaml") + require.Nil(t, err) + + res, err := createResolution("subtask.yaml", map[string]interface{}{}, nil) + require.Nil(t, err, "failed to create resolution: %s", err) + + res, err = runResolution(res) + require.Nil(t, err) + require.NotNil(t, res) + assert.Equal(t, resolution.StateWaiting, res.State) + + subtaskCreationOutput := res.Steps["subtaskCreation"].Output.(map[string]interface{}) + subtaskPublicID := subtaskCreationOutput["id"].(string) + + // pausing parent task + res.SetState(resolution.StatePaused) + res.Update(dbp) + + subtask, err := task.LoadFromPublicID(dbp, subtaskPublicID) + require.Nil(t, err) + assert.Equal(t, task.StateTODO, subtask.State) + + subtaskResolution, err := resolution.Create(dbp, subtask, nil, "", false, nil) + require.Nil(t, err) + + subtaskResolution, err = runResolution(subtaskResolution) + require.Nil(t, err) + assert.Equal(t, task.StateDone, subtaskResolution.State) + for k, v := range subtaskResolution.Steps { + assert.Equal(t, step.StateDone, v.State, "not valid state for step %s", k) + } + + subtask, err = task.LoadFromPublicID(dbp, subtaskPublicID) + require.Nil(t, err) + assert.Equal(t, task.StateDone, subtask.State) + parentTaskToResume, err := taskutils.ShouldResumeParentTask(dbp, subtask) + require.Nil(t, parentTaskToResume) + require.Nil(t, err) +} + func TestResolveCallback(t *testing.T) { res, err := createResolution("callback.yaml", map[string]interface{}{}, nil) require.NoError(t, err) diff --git a/models/task/task.go b/models/task/task.go index 3142c750..0e4ab63d 100644 --- a/models/task/task.go +++ b/models/task/task.go @@ -20,7 +20,6 @@ import ( "github.com/ovh/utask/engine/values" "github.com/ovh/utask/models" "github.com/ovh/utask/models/tasktemplate" - "github.com/ovh/utask/pkg/constants" "github.com/ovh/utask/pkg/notify" "github.com/ovh/utask/pkg/now" "github.com/ovh/utask/pkg/utils" @@ -765,35 +764,3 @@ func (t *Task) NotifyStepState(stepName, stepState string) { notify.ListActions().TaskStepUpdateAction, ) } - -func (t *Task) ShouldResumeParentTask(dbp zesty.DBProvider) (*Task, error) { - switch t.State { - case StateDone, StateWontfix, StateCancelled: - default: - return nil, nil - } - if t.Tags == nil { - return nil, nil - } - parentTaskID, ok := t.Tags[constants.SubtaskTagParentTaskID] - if !ok { - return nil, nil - } - - parentTask, err := LoadFromPublicID(dbp, parentTaskID) - if err != nil { - return nil, err - } - switch parentTask.State { - case StateBlocked, StateRunning, StateWaiting: - default: - // not allowed to resume a parent task that is not either Waiting, Running or Blocked. - // Todo state should not be runned as it might need manual resolution from a granted resolver - return nil, nil - } - if parentTask.Resolution == nil { - return nil, nil - } - - return parentTask, nil -} diff --git a/pkg/taskutils/taskutils.go b/pkg/taskutils/taskutils.go index aa8b3a73..69ddc433 100644 --- a/pkg/taskutils/taskutils.go +++ b/pkg/taskutils/taskutils.go @@ -11,6 +11,7 @@ import ( "github.com/ovh/utask/models/task" "github.com/ovh/utask/models/tasktemplate" "github.com/ovh/utask/pkg/auth" + "github.com/ovh/utask/pkg/constants" ) // CreateTask creates a task with the given inputs, and creates a resolution if autorunnable @@ -67,3 +68,45 @@ func CreateTask(c context.Context, dbp zesty.DBProvider, tt *tasktemplate.TaskTe return t, nil } + +func ShouldResumeParentTask(dbp zesty.DBProvider, t *task.Task) (*task.Task, error) { + switch t.State { + case task.StateDone, task.StateWontfix, task.StateCancelled: + default: + return nil, nil + } + if t.Tags == nil { + return nil, nil + } + parentTaskID, ok := t.Tags[constants.SubtaskTagParentTaskID] + if !ok { + return nil, nil + } + + parentTask, err := task.LoadFromPublicID(dbp, parentTaskID) + if err != nil { + return nil, err + } + switch parentTask.State { + case task.StateBlocked, task.StateRunning, task.StateWaiting: + default: + // not allowed to resume a parent task that is not either Waiting, Running or Blocked. + // Todo state should not be runned as it might need manual resolution from a granted resolver + return nil, nil + } + if parentTask.Resolution == nil { + return nil, nil + } + + r, err := resolution.LoadFromPublicID(dbp, *parentTask.Resolution) + if err != nil { + return nil, err + } + + switch r.State { + case resolution.StateCrashed, resolution.StatePaused: + return nil, nil + } + + return parentTask, nil +}