[BUGFIX]Cancelation following a error of a step randomly fail to canc…

…el to all step of the same recipe/priority

recipe/do.go

@@ -12,8 +12,10 @@ var mu sync.Mutex
 var hadError bool
 
 //watchdog will run has a parallel goroutine until the context is cancelled or the Do sub push to the end channel
-func stepWatchdog(ctxRecipe context.Context, end chan bool, step common.Steper) {
+func stepWatchdog(ctxRecipe context.Context, wgWatchdog *sync.WaitGroup, end chan bool, step common.Steper) {
 	defer close(end)
+	defer wgWatchdog.Done()
+
 	select {
 	case <-ctxRecipe.Done(): // the context has been cancelled
 		//Step aborted, revert the action of the step if we can
@@ -25,10 +27,11 @@ func stepWatchdog(ctxRecipe context.Context, end chan bool, step common.Steper)
 }
 
 //stepExecutor will execute the step, cancel the context and raise the global fi
-func stepExecutor(ctxRecipe context.Context, step common.Steper, wgStep *sync.WaitGroup, cancelRecipe func(), end chan bool, hadError chan bool) {
+func stepExecutor(ctxRecipe context.Context, step common.Steper, wgStep *sync.WaitGroup, wgWatchdog *sync.WaitGroup, cancelRecipe func(), end chan bool, hadError chan bool) {
 	defer wgStep.Done()
 
-	go stepWatchdog(ctxRecipe, end, step)
+	wgWatchdog.Add(1)
+	go stepWatchdog(ctxRecipe, wgWatchdog, end, step)
 
 	err := step.Do(ctxRecipe)
 	if err != nil {
@@ -58,9 +61,12 @@ func (ck *Cookbook) doOneRecipe(ctx context.Context, wgRecipe *sync.WaitGroup, r
 	for _, priority := range priorities {
 		nbSteps := len(ck.Recipes[rname].steps[uint(priority)])
 
-		//Waitgroup for this priority level
+		//Waitgroup for steps of this priority level
 		var wgStep sync.WaitGroup
 
+		//Waitgroup for watchdog of this priority level
+		var wgWatchdog sync.WaitGroup
+
 		//Channel to allow stepExecutor to inform if the step finished in error
 		recipeHadError := make(chan bool, nbSteps)
 		defer close(recipeHadError)
@@ -72,14 +78,19 @@ func (ck *Cookbook) doOneRecipe(ctx context.Context, wgRecipe *sync.WaitGroup, r
 			end := make(chan bool, 1)
 			ends = append(ends, end)
 
-			go stepExecutor(ctxRecipe, step, &wgStep, cancelRecipe, end, recipeHadError)
+			go stepExecutor(ctxRecipe, step, &wgStep, &wgWatchdog, cancelRecipe, end, recipeHadError)
 		}
+
+		//Wait for all step/stepExecutor to finish
 		wgStep.Wait()
 		// All the step of this priority has finished (completed or cancelled), it's time to stop the watchdogs
 		for _, end := range ends {
 			end <- true
 		}
 
+		//Wait for all the watchdog to finish
+		wgWatchdog.Wait()
+
 		//Since there is one end channel by stepExecutor
 		//Each stepExecutor will send only one boolean to the recipeHadError channel
 		for i := 0; i < nbSteps; i++ {
@@ -108,15 +119,19 @@ receive an cancelation that they could use to rollback by example and all the st
 with a priority level not already launched will not be runned.
 */
 func (ck *Cookbook) Do(ctx context.Context) bool {
+	// Waitgroup for the recipes
 	var wgRecipe sync.WaitGroup
+
 	for rname := range ck.Recipes {
 		wgRecipe.Add(1)
 		go ck.doOneRecipe(ctx, &wgRecipe, rname)
 	}
 	wgRecipe.Wait()
+	//We wont treat the event before all recipe goroutine are finished
 	select {
-	case <-ctx.Done(): // the context has been cancelled we want the shell return code to be not ok
-		return true
+	case <-ctx.Done(): // the context has been cancelled before, since all the goroutine has also be notified
+		//  via context inheritance, we can afford to take this event in account after their termination (via the wgRecipe.Wait)
+		return true // we want the shell return code to be not ok
 
 	default: // Make the poll to ctx.Done() non blocking
 		// Do nothing

recipe/do_test.go

@@ -102,14 +102,20 @@ func TestRecipeDoStepError(t *testing.T) {
 		}
 	}
 
+	// Way to verify the status of all steps in synthetic way (only for debug)
+	//debug := make([]string, 0, 5)
+	//debug = append(debug, fmt.Sprintf("\n%11v | %6v | %10v | %6v", "name", "Called", "Cancelled", "Error"))
 	for _, step := range sf.Steps["steperror"] {
 		if step.Priority < 5 && step.Canceled {
 			t.Errorf("A step with priority less than 5 of steperror was cancelled")
 		}
 		if step.Priority < 5 && !step.Called {
 			t.Errorf("A step with priority less than 5 of steperror was not called")
 		}
-		if step.Priority == 5 && !step.Canceled {
+		/*		if step.Priority == 5 {
+					debug = append(debug, fmt.Sprintf("%11v | %6v | %10v | %6v", step.name, step.Called, step.Canceled, step.HasError))
+				}
+		*/if step.Priority == 5 && !step.Canceled {
 			t.Errorf("A step with priority 5 of steperror was not cancelled")
 		}
 		if step.Priority == 5 && !step.Called {
@@ -122,4 +128,6 @@ func TestRecipeDoStepError(t *testing.T) {
 			t.Errorf("A step with priority more than 5 of steperror was cancelled")
 		}
 	}
+
+	//t.Fatal(strings.Join(debug, "\n"))
 }

recipe/recipe.go

@@ -66,14 +66,3 @@ func (ck *Cookbook) Statistics() (map[string][]int, int) {
 	}
 	return result, total
 }
-
-/*TODO:
-
-Gestion des logs se baser sur Packer par exemple:
-https://github.com/hashicorp/packer/blob/3d5af49bf32aca277c573af2e454ee5ed84ef505/log.go#L17
-
-Ou voir a utiliser https://github.com/hashicorp/go-hclog
-
-Pas besoin d'utiliser un channel dans ce cas là, un mutex pour le screen est suffisant (on peut aussi utiliser channel et une goroutine dédiée)
-
-*/

recipe/testdata/good/steperror/steps/step5.yaml

@@ -1,2 +1,2 @@
 priority: 5
-steps: 1
+steps: 3