Merge pull request #643 from ystia/bugfix/GH-630-cancelation_during_d…

…eploy Fix error on canceling a running deployment
ystia · May 12, 2020 · 4f127ac · 4f127ac
2 parents e2d9a8a + acf9f7a
commit 4f127ac
Show file tree

Hide file tree

Showing 5 changed files with 103 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@
 
 * Bootstrap may failed with a nil pointer error if download of a component fails ([GH-634](https://github.com/ystia/yorc/issues/634))
 * Missing concurrency limit during data migration for logs and events file storage ([GH-640](https://github.com/ystia/yorc/issues/640))
+* Unable to undeploy a deployment in progress from Alien4Cloud ([GH-630](https://github.com/ystia/yorc/issues/630))
 
 ## 4.0.0 (April 17, 2020)
 

diff --git a/tasks/consul_test.go b/tasks/consul_test.go
@@ -91,5 +91,8 @@ func TestRunConsulTasksPackageTests(t *testing.T) {
 		t.Run("TestIsStepRegistrationInProgress", func(t *testing.T) {
 			testIsStepRegistrationInProgress(t)
 		})
+		t.Run("TestCheckAndSetTaskErrorMessage", func(t *testing.T) {
+			testCheckAndSetTaskErrorMessage(t)
+		})
 	})
 }
diff --git a/tasks/tasks.go b/tasks/tasks.go
@@ -137,13 +137,42 @@ func GetTaskErrorMessage(taskID string) (string, error) {
 	return "", nil
 }
 
-// SetTaskErrorMessage retrieves the task related error message if any.
+// SetTaskErrorMessage sets a task related error message.
 //
-// If no error message is found, an empty string is returned instead
+// Set task error message even if it already contains a value.
+// For a better control on gracefully setting this error message use CheckAndSetTaskErrorMessage
 func SetTaskErrorMessage(taskID, errorMessage string) error {
 	return consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "errorMessage"), errorMessage)
 }
 
+// CheckAndSetTaskErrorMessage sets a task related error message.
+//
+// This function check for an existing message and overwrite it only if requested.
+func CheckAndSetTaskErrorMessage(taskID, errorMessage string, overwriteExisting bool) error {
+	keyPath := path.Join(consulutil.TasksPrefix, taskID, "errorMessage")
+	kvp := &api.KVPair{Key: keyPath, Value: []byte(errorMessage)}
+	kv := consulutil.GetKV()
+	for {
+		existingKey, meta, err := kv.Get(keyPath, nil)
+		if err != nil {
+			return errors.Wrap(err, consulutil.ConsulGenericErrMsg)
+		}
+		if existingKey != nil && len(existingKey.Value) > 0 && !overwriteExisting {
+			return nil
+		}
+		if existingKey != nil {
+			kvp.ModifyIndex = meta.LastIndex
+		}
+		set, _, err := consulutil.GetKV().CAS(kvp, nil)
+		if err != nil {
+			return errors.Wrap(err, consulutil.ConsulGenericErrMsg)
+		}
+		if set {
+			return nil
+		}
+	}
+}
+
 // GetTaskResultSet retrieves the task related resultSet in json string format
 //
 // If no resultSet is found, nil is returned instead

diff --git a/tasks/tasks_test.go b/tasks/tasks_test.go
@@ -18,16 +18,18 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"github.com/stretchr/testify/require"
-	"github.com/ystia/yorc/v4/storage"
-	"github.com/ystia/yorc/v4/storage/types"
-	"github.com/ystia/yorc/v4/tosca"
+	"net/url"
 	"path"
 	"reflect"
 	"testing"
 
 	"github.com/hashicorp/consul/testutil"
+	"github.com/stretchr/testify/require"
+
 	"github.com/ystia/yorc/v4/helper/consulutil"
+	"github.com/ystia/yorc/v4/storage"
+	"github.com/ystia/yorc/v4/storage/types"
+	"github.com/ystia/yorc/v4/tosca"
 )
 
 func populateKV(t *testing.T, srv *testutil.TestServer) {
@@ -708,3 +710,61 @@ func testIsStepRegistrationInProgress(t *testing.T) {
 		})
 	}
 }
+
+func testCheckAndSetTaskErrorMessage(t *testing.T) {
+	type setupFn func(t *testing.T, taskID string)
+	type args struct {
+		errorMessage      string
+		overwriteExisting bool
+	}
+	tests := []struct {
+		name                 string
+		args                 args
+		setupFn              setupFn
+		wantErr              bool
+		expectedErrorMessage string
+	}{
+		{"TaskExistsErrorMessageDoesNotExistNoOverwrite", args{"MyMessage", false}, func(t *testing.T, taskID string) {
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "targetId"), "id")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "status"), "1")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "type"), "1")
+		}, false, "MyMessage"},
+		{"TaskExistsErrorMessageDoesNotExistOverwrite", args{"MyMessage", true}, func(t *testing.T, taskID string) {
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "targetId"), "id")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "status"), "1")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "type"), "1")
+		}, false, "MyMessage"},
+		{"TaskExistsErrorMessageExistNoOverwrite", args{"MyMessage", false}, func(t *testing.T, taskID string) {
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "targetId"), "id")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "status"), "1")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "type"), "1")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "errorMessage"), "PreviousErrorMessage")
+		}, false, "PreviousErrorMessage"},
+		{"TaskExistsErrorMessageExistOverwrite", args{"MyMessage", true}, func(t *testing.T, taskID string) {
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "targetId"), "id")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "status"), "1")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "type"), "1")
+			consulutil.StoreConsulKeyAsString(path.Join(consulutil.TasksPrefix, taskID, "errorMessage"), "PreviousErrorMessage")
+		}, false, "MyMessage"},
+		{"TaskDoesNotExistNoOverwrite", args{"MyMessage", false}, nil, false, "MyMessage"},
+		{"TaskDoesNotExistOverwrite", args{"MyMessage", true}, nil, false, "MyMessage"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			taskID := url.PathEscape(t.Name())
+			defer consulutil.Delete(path.Join(consulutil.TasksPrefix, taskID), true)
+			if tt.setupFn != nil {
+				tt.setupFn(t, taskID)
+			}
+			if err := CheckAndSetTaskErrorMessage(taskID, tt.args.errorMessage, tt.args.overwriteExisting); (err != nil) != tt.wantErr {
+				t.Errorf("CheckAndSetTaskErrorMessage() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			actualErrorMessage, err := GetTaskErrorMessage(taskID)
+			require.NoError(t, err)
+			if actualErrorMessage != tt.expectedErrorMessage {
+				t.Errorf("CheckAndSetTaskErrorMessage() actualErrorMessage = %v, expectedErrorMessage %v", actualErrorMessage, tt.expectedErrorMessage)
+			}
+
+		})
+	}
+}
diff --git a/tasks/workflow/step.go b/tasks/workflow/step.go
@@ -216,13 +216,11 @@ func (s *step) run(ctx context.Context, cfg config.Configuration, deploymentID s
 				s.setStatus(tasks.TaskStepStatusERROR)
 				if !bypassErrors {
 					tasks.NotifyErrorOnTask(s.t.taskID)
-					// set task status and generic error message
-					err2 := checkAndSetTaskStatus(ctx, s.t.targetID, s.t.taskID, tasks.TaskStatusFAILED, errors.Errorf("Workflow %q step%q failed.", workflowName, s.Name))
-					if err2 != nil {
-						events.WithContextOptionalFields(ctx).NewLogEntry(events.LogLevelERROR, deploymentID).Registerf("failed to set task status to error for step:%q: %v", s.Name, err2)
-					}
+					// only set generic error message here.
+					// Task status is handled in task execution final function
+					tasks.CheckAndSetTaskErrorMessage(s.t.taskID, fmt.Sprintf("Workflow %q step %q failed.", workflowName, s.Name), false)
 
-					err2 = s.registerOnCancelOrFailureSteps(ctx, workflowName, s.OnFailure)
+					err2 := s.registerOnCancelOrFailureSteps(ctx, workflowName, s.OnFailure)
 					if err2 != nil {
 						events.WithContextOptionalFields(ctx).NewLogEntry(events.LogLevelERROR, deploymentID).Registerf("failed to register on failure steps: %v", err2)
 					}