fix(subtask): do not auto-resume parent task of subtask if parent res…

…olution 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>
ovh · Oct 14, 2022 · 02f7721 · 02f7721
1 parent 029bf53
commit 02f7721
Show file tree

Hide file tree

Showing 5 changed files with 102 additions and 43 deletions.
diff --git 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
@@ -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
@@ -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,14 +1181,22 @@ 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
 	i := time.Duration(0)
 	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
@@ -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
@@ -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
+}