From 867fa925224cbb4b6a3e14a258a40bf35b1d320f Mon Sep 17 00:00:00 2001 From: odubajDT Date: Fri, 18 Nov 2022 14:35:58 +0100 Subject: [PATCH 01/12] first draft Signed-off-by: odubajDT --- operator/api/v1alpha1/common/phases.go | 26 ++++++++ operator/controllers/common/phasehandler.go | 71 +++++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/operator/api/v1alpha1/common/phases.go b/operator/api/v1alpha1/common/phases.go index b453a93318..06f403d101 100644 --- a/operator/api/v1alpha1/common/phases.go +++ b/operator/api/v1alpha1/common/phases.go @@ -1,5 +1,7 @@ package common +import "strings" + type KeptnPhase KeptnPhaseType type KeptnPhaseType struct { @@ -7,6 +9,30 @@ type KeptnPhaseType struct { ShortName string } +func (p KeptnPhaseType) IsEvaluation() bool { + return strings.Contains(p.ShortName, "DeployEvaluations") +} + +func (p KeptnPhaseType) IsPreEvaluation() bool { + return strings.Contains(p.ShortName, "PreDeployEvaluations") +} + +func (p KeptnPhaseType) IsPostEvaluation() bool { + return strings.Contains(p.ShortName, "PostDeployEvaluations") +} + +func (p KeptnPhaseType) IsTask() bool { + return strings.Contains(p.ShortName, "DeployTasks") +} + +func (p KeptnPhaseType) IsPreTask() bool { + return strings.Contains(p.ShortName, "PreDeployTasks") +} + +func (p KeptnPhaseType) IsPostTask() bool { + return strings.Contains(p.ShortName, "PostDeployTasks") +} + var ( PhaseWorkloadPreDeployment = KeptnPhaseType{LongName: "Workload Pre-Deployment Tasks", ShortName: "WorkloadPreDeployTasks"} PhaseWorkloadPostDeployment = KeptnPhaseType{LongName: "Workload Post-Deployment Tasks", ShortName: "WorkloadPostDeployTasks"} diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index a59fc021c5..984b226e45 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -6,9 +6,11 @@ import ( "time" "github.com/go-logr/logr" + klcv1alpha1 "github.com/keptn/lifecycle-toolkit/operator/api/v1alpha1" "github.com/keptn/lifecycle-toolkit/operator/api/v1alpha1/common" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/trace" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -79,6 +81,12 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, piWrapper.Complete() piWrapper.SetState(common.StateFailed) spanAppTrace.AddEvent(phase.LongName + " has failed") + msg, err := r.CreateFailureReasonMessages(ctx, phase, piWrapper) + if err != nil { + r.Log.Error(err, "cannot create failure spans") + } else { + spanAppTrace.AddEvent(msg) + } spanAppTrace.SetStatus(codes.Error, "Failed") spanAppTrace.End() if err := r.SpanHandler.UnbindSpan(reconcileObject, phase.ShortName); err != nil { @@ -106,3 +114,66 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, return &PhaseResult{Continue: false, Result: requeueResult}, nil } + +func (r PhaseHandler) CreateFailureReasonMessages(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { + if phase.IsEvaluation() { + return r.GetEvaluationFailureReasons(ctx, phase, object) + } else if phase.IsTask() { + return r.GetTaskFailureReasons(ctx, phase, object) + } + return "", nil +} + +func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { + resultMsg := "" + var evaluations []klcv1alpha1.KeptnEvaluation + var status []klcv1alpha1.EvaluationStatus + if phase.IsPreEvaluation() { + status = object.GetPreDeploymentEvaluationTaskStatus() + } else { + status = object.GetPostDeploymentEvaluationTaskStatus() + } + + for _, item := range status { + evaluation := &klcv1alpha1.KeptnEvaluation{} + if err := r.Client.Get(ctx, types.NamespacedName{Name: item.EvaluationName, Namespace: object.GetNamespace()}, evaluation); err != nil { + return "", err + } + evaluations = append(evaluations, *evaluation) + } + + for _, eval := range evaluations { + for k, v := range eval.Status.EvaluationStatus { + if v.Status == common.StateFailed { + resultMsg = resultMsg + fmt.Sprintf("\n evaluation of '%s' failed with value: '%s': %s", k, v.Value, v.Message) + } + } + } + + return resultMsg, nil +} + +func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { + resultMsg := "" + var tasks []klcv1alpha1.KeptnTask + var status []klcv1alpha1.TaskStatus + if phase.IsPreTask() { + status = object.GetPreDeploymentTaskStatus() + } else { + status = object.GetPostDeploymentTaskStatus() + } + + for _, item := range status { + task := &klcv1alpha1.KeptnTask{} + if err := r.Client.Get(ctx, types.NamespacedName{Name: item.TaskName, Namespace: object.GetNamespace()}, task); err != nil { + return "", err + } + tasks = append(tasks, *task) + } + + for _, task := range tasks { + //get job + } + + return resultMsg, nil +} From af6f8693973475e8e382d5dbe7cdb38bc18d96db Mon Sep 17 00:00:00 2001 From: odubajDT Date: Sun, 20 Nov 2022 11:06:39 +0100 Subject: [PATCH 02/12] not possible to fetch failed job data Signed-off-by: odubajDT --- operator/controllers/common/phasehandler.go | 42 +++++++++++---------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index 984b226e45..5ba949a306 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -126,7 +126,6 @@ func (r PhaseHandler) CreateFailureReasonMessages(ctx context.Context, phase com func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { resultMsg := "" - var evaluations []klcv1alpha1.KeptnEvaluation var status []klcv1alpha1.EvaluationStatus if phase.IsPreEvaluation() { status = object.GetPreDeploymentEvaluationTaskStatus() @@ -134,19 +133,20 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com status = object.GetPostDeploymentEvaluationTaskStatus() } - for _, item := range status { - evaluation := &klcv1alpha1.KeptnEvaluation{} - if err := r.Client.Get(ctx, types.NamespacedName{Name: item.EvaluationName, Namespace: object.GetNamespace()}, evaluation); err != nil { - return "", err - } - evaluations = append(evaluations, *evaluation) + // there can be only one evaluation and in this section of the code, it can only be failed + // checking length of the status only for safety reasons + if len(status) != 1 { + return "", fmt.Errorf("evaluation status not found") } - for _, eval := range evaluations { - for k, v := range eval.Status.EvaluationStatus { - if v.Status == common.StateFailed { - resultMsg = resultMsg + fmt.Sprintf("\n evaluation of '%s' failed with value: '%s': %s", k, v.Value, v.Message) - } + evaluation := &klcv1alpha1.KeptnEvaluation{} + if err := r.Client.Get(ctx, types.NamespacedName{Name: status[0].EvaluationName, Namespace: object.GetNamespace()}, evaluation); err != nil { + return "", err + } + + for k, v := range evaluation.Status.EvaluationStatus { + if v.Status == common.StateFailed { + resultMsg = resultMsg + fmt.Sprintf("\n evaluation of '%s' failed with value: '%s' and reason: '%s'", k, v.Value, v.Message) } } @@ -155,7 +155,7 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { resultMsg := "" - var tasks []klcv1alpha1.KeptnTask + var failedTasks []klcv1alpha1.KeptnTask var status []klcv1alpha1.TaskStatus if phase.IsPreTask() { status = object.GetPreDeploymentTaskStatus() @@ -164,16 +164,18 @@ func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.Ke } for _, item := range status { - task := &klcv1alpha1.KeptnTask{} - if err := r.Client.Get(ctx, types.NamespacedName{Name: item.TaskName, Namespace: object.GetNamespace()}, task); err != nil { - return "", err + if item.Status == common.StateFailed { + task := &klcv1alpha1.KeptnTask{} + if err := r.Client.Get(ctx, types.NamespacedName{Name: item.TaskName, Namespace: object.GetNamespace()}, task); err != nil { + return "", err + } + failedTasks = append(failedTasks, *task) } - tasks = append(tasks, *task) } - for _, task := range tasks { - //get job - } + // for _, task := range failedTasks { + // //get job failure details + // } return resultMsg, nil } From 0e587624f6e9b4ebf685d9b5195060e17cb7991e Mon Sep 17 00:00:00 2001 From: odubajDT Date: Sun, 20 Nov 2022 11:13:32 +0100 Subject: [PATCH 03/12] first version Signed-off-by: odubajDT --- operator/api/v1alpha1/common/phases.go | 12 ------- operator/controllers/common/phasehandler.go | 36 --------------------- 2 files changed, 48 deletions(-) diff --git a/operator/api/v1alpha1/common/phases.go b/operator/api/v1alpha1/common/phases.go index 06f403d101..fde2deefd4 100644 --- a/operator/api/v1alpha1/common/phases.go +++ b/operator/api/v1alpha1/common/phases.go @@ -21,18 +21,6 @@ func (p KeptnPhaseType) IsPostEvaluation() bool { return strings.Contains(p.ShortName, "PostDeployEvaluations") } -func (p KeptnPhaseType) IsTask() bool { - return strings.Contains(p.ShortName, "DeployTasks") -} - -func (p KeptnPhaseType) IsPreTask() bool { - return strings.Contains(p.ShortName, "PreDeployTasks") -} - -func (p KeptnPhaseType) IsPostTask() bool { - return strings.Contains(p.ShortName, "PostDeployTasks") -} - var ( PhaseWorkloadPreDeployment = KeptnPhaseType{LongName: "Workload Pre-Deployment Tasks", ShortName: "WorkloadPreDeployTasks"} PhaseWorkloadPostDeployment = KeptnPhaseType{LongName: "Workload Post-Deployment Tasks", ShortName: "WorkloadPostDeployTasks"} diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index 5ba949a306..8474f3a235 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -115,15 +115,6 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, return &PhaseResult{Continue: false, Result: requeueResult}, nil } -func (r PhaseHandler) CreateFailureReasonMessages(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { - if phase.IsEvaluation() { - return r.GetEvaluationFailureReasons(ctx, phase, object) - } else if phase.IsTask() { - return r.GetTaskFailureReasons(ctx, phase, object) - } - return "", nil -} - func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { resultMsg := "" var status []klcv1alpha1.EvaluationStatus @@ -152,30 +143,3 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com return resultMsg, nil } - -func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { - resultMsg := "" - var failedTasks []klcv1alpha1.KeptnTask - var status []klcv1alpha1.TaskStatus - if phase.IsPreTask() { - status = object.GetPreDeploymentTaskStatus() - } else { - status = object.GetPostDeploymentTaskStatus() - } - - for _, item := range status { - if item.Status == common.StateFailed { - task := &klcv1alpha1.KeptnTask{} - if err := r.Client.Get(ctx, types.NamespacedName{Name: item.TaskName, Namespace: object.GetNamespace()}, task); err != nil { - return "", err - } - failedTasks = append(failedTasks, *task) - } - } - - // for _, task := range failedTasks { - // //get job failure details - // } - - return resultMsg, nil -} From 6e682bb0d8957cb1483072a99f0ea417d90074f1 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Sun, 20 Nov 2022 11:22:45 +0100 Subject: [PATCH 04/12] added tests for phases Signed-off-by: odubajDT --- operator/api/v1alpha1/common/common_test.go | 8 +- operator/api/v1alpha1/common/phases_test.go | 118 ++++++++++++++++++++ 2 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 operator/api/v1alpha1/common/phases_test.go diff --git a/operator/api/v1alpha1/common/common_test.go b/operator/api/v1alpha1/common/common_test.go index 7b20c9826d..1bb60e0921 100644 --- a/operator/api/v1alpha1/common/common_test.go +++ b/operator/api/v1alpha1/common/common_test.go @@ -7,7 +7,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestKeptnKeptnState_IsCompleted(t *testing.T) { +func TestKeptnState_IsCompleted(t *testing.T) { tests := []struct { State KeptnState Want bool @@ -36,7 +36,7 @@ func TestKeptnKeptnState_IsCompleted(t *testing.T) { } } -func TestKeptnKeptnState_IsSucceeded(t *testing.T) { +func TestKeptnState_IsSucceeded(t *testing.T) { tests := []struct { State KeptnState Want bool @@ -57,7 +57,7 @@ func TestKeptnKeptnState_IsSucceeded(t *testing.T) { } } -func TestKeptnKeptnState_IsFailed(t *testing.T) { +func TestKeptnState_IsFailed(t *testing.T) { tests := []struct { State KeptnState Want bool @@ -78,7 +78,7 @@ func TestKeptnKeptnState_IsFailed(t *testing.T) { } } -func TestKeptnKeptnState_IsCancelled(t *testing.T) { +func TestKeptnState_IsCancelled(t *testing.T) { tests := []struct { State KeptnState Want bool diff --git a/operator/api/v1alpha1/common/phases_test.go b/operator/api/v1alpha1/common/phases_test.go new file mode 100644 index 0000000000..41c43a10f3 --- /dev/null +++ b/operator/api/v1alpha1/common/phases_test.go @@ -0,0 +1,118 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestKeptnPhaseType_IsEvaluation(t *testing.T) { + tests := []struct { + State KeptnPhaseType + Want bool + }{ + { + State: PhaseWorkloadDeployment, + Want: false, + }, + { + State: PhaseWorkloadPostEvaluation, + Want: true, + }, + { + State: PhaseWorkloadPreEvaluation, + Want: true, + }, + { + State: PhaseAppPostEvaluation, + Want: true, + }, + { + State: PhaseAppPreEvaluation, + Want: true, + }, + { + State: PhaseAppPreDeployment, + Want: false, + }, + } + for _, tt := range tests { + t.Run("", func(t *testing.T) { + require.Equal(t, tt.State.IsEvaluation(), tt.Want) + }) + } +} + +func TestKeptnPhaseType_IsPreEvaluation(t *testing.T) { + tests := []struct { + State KeptnPhaseType + Want bool + }{ + { + State: PhaseWorkloadDeployment, + Want: false, + }, + { + State: PhaseWorkloadPostEvaluation, + Want: false, + }, + { + State: PhaseWorkloadPreEvaluation, + Want: true, + }, + { + State: PhaseAppPostEvaluation, + Want: false, + }, + { + State: PhaseAppPreEvaluation, + Want: true, + }, + { + State: PhaseAppPreDeployment, + Want: false, + }, + } + for _, tt := range tests { + t.Run("", func(t *testing.T) { + require.Equal(t, tt.State.IsPreEvaluation(), tt.Want) + }) + } +} + +func TestKeptnPhaseType_IsPostEvaluation(t *testing.T) { + tests := []struct { + State KeptnPhaseType + Want bool + }{ + { + State: PhaseWorkloadDeployment, + Want: false, + }, + { + State: PhaseWorkloadPostEvaluation, + Want: true, + }, + { + State: PhaseWorkloadPreEvaluation, + Want: false, + }, + { + State: PhaseAppPostEvaluation, + Want: true, + }, + { + State: PhaseAppPreEvaluation, + Want: false, + }, + { + State: PhaseAppPreDeployment, + Want: false, + }, + } + for _, tt := range tests { + t.Run("", func(t *testing.T) { + require.Equal(t, tt.State.IsPostEvaluation(), tt.Want) + }) + } +} From 9430976a4dc3ed9a10996ee6bdc87aea65b382e8 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 21 Nov 2022 09:38:39 +0100 Subject: [PATCH 05/12] support task failure spans Signed-off-by: odubajDT --- operator/api/v1alpha1/common/phases.go | 12 ++ operator/api/v1alpha1/common/phases_test.go | 111 ++++++++++++++++++ operator/api/v1alpha1/keptntask_types.go | 1 + .../bases/lifecycle.keptn.sh_keptntasks.yaml | 2 + operator/controllers/common/phasehandler.go | 38 +++++- 5 files changed, 163 insertions(+), 1 deletion(-) diff --git a/operator/api/v1alpha1/common/phases.go b/operator/api/v1alpha1/common/phases.go index fde2deefd4..06f403d101 100644 --- a/operator/api/v1alpha1/common/phases.go +++ b/operator/api/v1alpha1/common/phases.go @@ -21,6 +21,18 @@ func (p KeptnPhaseType) IsPostEvaluation() bool { return strings.Contains(p.ShortName, "PostDeployEvaluations") } +func (p KeptnPhaseType) IsTask() bool { + return strings.Contains(p.ShortName, "DeployTasks") +} + +func (p KeptnPhaseType) IsPreTask() bool { + return strings.Contains(p.ShortName, "PreDeployTasks") +} + +func (p KeptnPhaseType) IsPostTask() bool { + return strings.Contains(p.ShortName, "PostDeployTasks") +} + var ( PhaseWorkloadPreDeployment = KeptnPhaseType{LongName: "Workload Pre-Deployment Tasks", ShortName: "WorkloadPreDeployTasks"} PhaseWorkloadPostDeployment = KeptnPhaseType{LongName: "Workload Post-Deployment Tasks", ShortName: "WorkloadPostDeployTasks"} diff --git a/operator/api/v1alpha1/common/phases_test.go b/operator/api/v1alpha1/common/phases_test.go index 41c43a10f3..e200da4fbe 100644 --- a/operator/api/v1alpha1/common/phases_test.go +++ b/operator/api/v1alpha1/common/phases_test.go @@ -116,3 +116,114 @@ func TestKeptnPhaseType_IsPostEvaluation(t *testing.T) { }) } } + +func TestKeptnPhaseType_IsTask(t *testing.T) { + tests := []struct { + State KeptnPhaseType + Want bool + }{ + { + State: PhaseWorkloadDeployment, + Want: false, + }, + { + State: PhaseWorkloadPostDeployment, + Want: true, + }, + { + State: PhaseWorkloadPreDeployment, + Want: true, + }, + { + State: PhaseAppPostDeployment, + Want: true, + }, + { + State: PhaseAppPreDeployment, + Want: true, + }, + { + State: PhaseAppPreEvaluation, + Want: false, + }, + } + for _, tt := range tests { + t.Run("", func(t *testing.T) { + require.Equal(t, tt.State.IsTask(), tt.Want) + }) + } +} + +func TestKeptnPhaseType_IsPreTask(t *testing.T) { + tests := []struct { + State KeptnPhaseType + Want bool + }{ + { + State: PhaseWorkloadDeployment, + Want: false, + }, + { + State: PhaseWorkloadPostDeployment, + Want: false, + }, + { + State: PhaseWorkloadPreDeployment, + Want: true, + }, + { + State: PhaseAppPostDeployment, + Want: false, + }, + { + State: PhaseAppPreDeployment, + Want: true, + }, + { + State: PhaseAppPreEvaluation, + Want: false, + }, + } + for _, tt := range tests { + t.Run("", func(t *testing.T) { + require.Equal(t, tt.State.IsPreTask(), tt.Want) + }) + } +} + +func TestKeptnPhaseType_IsPostTask(t *testing.T) { + tests := []struct { + State KeptnPhaseType + Want bool + }{ + { + State: PhaseWorkloadDeployment, + Want: false, + }, + { + State: PhaseWorkloadPostDeployment, + Want: true, + }, + { + State: PhaseWorkloadPreDeployment, + Want: false, + }, + { + State: PhaseAppPostDeployment, + Want: true, + }, + { + State: PhaseAppPreDeployment, + Want: false, + }, + { + State: PhaseAppPreEvaluation, + Want: false, + }, + } + for _, tt := range tests { + t.Run("", func(t *testing.T) { + require.Equal(t, tt.State.IsPostTask(), tt.Want) + }) + } +} diff --git a/operator/api/v1alpha1/keptntask_types.go b/operator/api/v1alpha1/keptntask_types.go index c89e2b8a5d..dfda41757c 100644 --- a/operator/api/v1alpha1/keptntask_types.go +++ b/operator/api/v1alpha1/keptntask_types.go @@ -64,6 +64,7 @@ type KeptnTaskStatus struct { JobName string `json:"jobName,omitempty"` // +kubebuilder:default:=Pending Status common.KeptnState `json:"status,omitempty"` + Message string `json:"message,omitempty"` StartTime metav1.Time `json:"startTime,omitempty"` EndTime metav1.Time `json:"endTime,omitempty"` // INSERT ADDITIONAL STATUS FIELD - define observed state of cluster diff --git a/operator/config/crd/bases/lifecycle.keptn.sh_keptntasks.yaml b/operator/config/crd/bases/lifecycle.keptn.sh_keptntasks.yaml index 0f930b630d..8fee50eeb9 100644 --- a/operator/config/crd/bases/lifecycle.keptn.sh_keptntasks.yaml +++ b/operator/config/crd/bases/lifecycle.keptn.sh_keptntasks.yaml @@ -116,6 +116,8 @@ spec: type: string jobName: type: string + message: + type: string startTime: format: date-time type: string diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index 8474f3a235..21f8e59751 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -115,6 +115,15 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, return &PhaseResult{Continue: false, Result: requeueResult}, nil } +func (r PhaseHandler) CreateFailureReasonMessages(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { + if phase.IsEvaluation() { + return r.GetEvaluationFailureReasons(ctx, phase, object) + } else if phase.IsTask() { + return r.GetTaskFailureReasons(ctx, phase, object) + } + return "", nil +} + func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { resultMsg := "" var status []klcv1alpha1.EvaluationStatus @@ -137,9 +146,36 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com for k, v := range evaluation.Status.EvaluationStatus { if v.Status == common.StateFailed { - resultMsg = resultMsg + fmt.Sprintf("\n evaluation of '%s' failed with value: '%s' and reason: '%s'", k, v.Value, v.Message) + resultMsg = resultMsg + fmt.Sprintf("\nevaluation of '%s' failed with value: '%s' and reason: '%s'", k, v.Value, v.Message) + } + } + + return resultMsg, nil +} + +func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { + resultMsg := "" + var failedTasks []klcv1alpha1.KeptnTask + var status []klcv1alpha1.TaskStatus + if phase.IsPreTask() { + status = object.GetPreDeploymentTaskStatus() + } else { + status = object.GetPostDeploymentTaskStatus() + } + + for _, item := range status { + if item.Status == common.StateFailed { + task := &klcv1alpha1.KeptnTask{} + if err := r.Client.Get(ctx, types.NamespacedName{Name: item.TaskName, Namespace: object.GetNamespace()}, task); err != nil { + return "", err + } + failedTasks = append(failedTasks, *task) } } + for _, task := range failedTasks { + resultMsg = resultMsg + fmt.Sprintf("\ntask '%s' failed with reason: '%s'", task.Name, task.Status.Message) + } + return resultMsg, nil } From 10bf4cea8f42a722bd449a4950c5b849b095d477 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 21 Nov 2022 10:09:17 +0100 Subject: [PATCH 06/12] renaming Signed-off-by: odubajDT --- operator/controllers/common/phasehandler.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index 21f8e59751..f067b81eb8 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -115,7 +115,7 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, return &PhaseResult{Continue: false, Result: requeueResult}, nil } -func (r PhaseHandler) CreateFailureReasonMessages(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { +func (r PhaseHandler) CreateFailureReasonSpanEvents(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { if phase.IsEvaluation() { return r.GetEvaluationFailureReasons(ctx, phase, object) } else if phase.IsTask() { @@ -146,7 +146,7 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com for k, v := range evaluation.Status.EvaluationStatus { if v.Status == common.StateFailed { - resultMsg = resultMsg + fmt.Sprintf("\nevaluation of '%s' failed with value: '%s' and reason: '%s'", k, v.Value, v.Message) + resultMsg = resultMsg + fmt.Sprintf("evaluation of '%s' failed with value: '%s' and reason: '%s'\n", k, v.Value, v.Message) } } @@ -174,7 +174,7 @@ func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.Ke } for _, task := range failedTasks { - resultMsg = resultMsg + fmt.Sprintf("\ntask '%s' failed with reason: '%s'", task.Name, task.Status.Message) + resultMsg = resultMsg + fmt.Sprintf("task '%s' failed with reason: '%s'\n", task.Name, task.Status.Message) } return resultMsg, nil From 91f31f061e82b0371d4cf07508391168586ccdc8 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 21 Nov 2022 11:11:41 +0100 Subject: [PATCH 07/12] added unit tests Signed-off-by: odubajDT --- operator/controllers/common/phasehandler.go | 10 +- .../controllers/common/phasehandler_test.go | 232 ++++++++++++++++++ 2 files changed, 237 insertions(+), 5 deletions(-) diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index f067b81eb8..127f6260ee 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -115,7 +115,7 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, return &PhaseResult{Continue: false, Result: requeueResult}, nil } -func (r PhaseHandler) CreateFailureReasonSpanEvents(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { +func (r PhaseHandler) createFailureReasonSpanEvents(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { if phase.IsEvaluation() { return r.GetEvaluationFailureReasons(ctx, phase, object) } else if phase.IsTask() { @@ -124,7 +124,7 @@ func (r PhaseHandler) CreateFailureReasonSpanEvents(ctx context.Context, phase c return "", nil } -func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { +func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) (string, error) { resultMsg := "" var status []klcv1alpha1.EvaluationStatus if phase.IsPreEvaluation() { @@ -141,7 +141,7 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com evaluation := &klcv1alpha1.KeptnEvaluation{} if err := r.Client.Get(ctx, types.NamespacedName{Name: status[0].EvaluationName, Namespace: object.GetNamespace()}, evaluation); err != nil { - return "", err + return "", fmt.Errorf("evaluation %s not found", status[0].EvaluationName) } for k, v := range evaluation.Status.EvaluationStatus { @@ -153,7 +153,7 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com return resultMsg, nil } -func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { +func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) (string, error) { resultMsg := "" var failedTasks []klcv1alpha1.KeptnTask var status []klcv1alpha1.TaskStatus @@ -167,7 +167,7 @@ func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.Ke if item.Status == common.StateFailed { task := &klcv1alpha1.KeptnTask{} if err := r.Client.Get(ctx, types.NamespacedName{Name: item.TaskName, Namespace: object.GetNamespace()}, task); err != nil { - return "", err + return "", fmt.Errorf("task %s not found", item.TaskName) } failedTasks = append(failedTasks, *task) } diff --git a/operator/controllers/common/phasehandler_test.go b/operator/controllers/common/phasehandler_test.go index 3c627e7af7..4dd8f07e49 100644 --- a/operator/controllers/common/phasehandler_test.go +++ b/operator/controllers/common/phasehandler_test.go @@ -225,3 +225,235 @@ func TestPhaseHandler(t *testing.T) { }) } } + +func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { + tests := []struct { + name string + handler PhaseHandler + object *v1alpha1.KeptnAppVersion + clientObject *v1alpha1.KeptnEvaluation + phase common.KeptnPhaseType + want string + wantErr error + }{ + { + name: "status len is 0", + handler: PhaseHandler{ + SpanHandler: &SpanHandler{}, + Log: ctrl.Log.WithName("controller"), + Recorder: record.NewFakeRecorder(100), + }, + object: &v1alpha1.KeptnAppVersion{ + Status: v1alpha1.KeptnAppVersionStatus{ + Status: common.StateFailed, + CurrentPhase: common.PhaseAppPreEvaluation.LongName, + }, + }, + clientObject: &v1alpha1.KeptnEvaluation{}, + phase: common.PhaseAppPreEvaluation, + want: "", + wantErr: fmt.Errorf("evaluation status not found"), + }, + { + name: "cannot get evaluation", + handler: PhaseHandler{ + SpanHandler: &SpanHandler{}, + Log: ctrl.Log.WithName("controller"), + Recorder: record.NewFakeRecorder(100), + }, + object: &v1alpha1.KeptnAppVersion{ + ObjectMeta: v1.ObjectMeta{ + Name: "appversion", + Namespace: "namespace", + }, + Status: v1alpha1.KeptnAppVersionStatus{ + Status: common.StateFailed, + CurrentPhase: common.PhaseAppPreEvaluation.LongName, + PreDeploymentEvaluationTaskStatus: []v1alpha1.EvaluationStatus{ + { + EvaluationName: "eval-name", + }, + }, + }, + }, + clientObject: &v1alpha1.KeptnEvaluation{}, + phase: common.PhaseAppPreEvaluation, + want: "", + wantErr: fmt.Errorf("evaluation eval-name not found"), + }, + { + name: "evaluation failed", + handler: PhaseHandler{ + SpanHandler: &SpanHandler{}, + Log: ctrl.Log.WithName("controller"), + Recorder: record.NewFakeRecorder(100), + }, + object: &v1alpha1.KeptnAppVersion{ + ObjectMeta: v1.ObjectMeta{ + Name: "appversion", + Namespace: "namespace", + }, + Status: v1alpha1.KeptnAppVersionStatus{ + Status: common.StateFailed, + CurrentPhase: common.PhaseAppPreEvaluation.LongName, + PreDeploymentEvaluationTaskStatus: []v1alpha1.EvaluationStatus{ + { + EvaluationName: "eval-name", + }, + }, + }, + }, + clientObject: &v1alpha1.KeptnEvaluation{ + ObjectMeta: v1.ObjectMeta{ + Name: "eval-name", + Namespace: "namespace", + }, + Status: v1alpha1.KeptnEvaluationStatus{ + EvaluationStatus: map[string]v1alpha1.EvaluationStatusItem{ + "cpu": { + Value: "10", + Status: common.StateFailed, + Message: "cpu failed", + }, + "mem": { + Value: "10", + Status: common.StateSucceeded, + Message: "mem passed", + }, + }, + }, + }, + phase: common.PhaseAppPreEvaluation, + want: "evaluation of 'cpu' failed with value: '10' and reason: 'cpu failed'\n", + wantErr: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v1alpha1.AddToScheme(scheme.Scheme) + client := fake.NewClientBuilder().WithObjects(tt.clientObject).Build() + tt.handler.Client = client + result, err := tt.handler.GetEvaluationFailureReasons(context.TODO(), tt.phase, tt.object) + require.Equal(t, tt.want, result) + require.Equal(t, tt.wantErr, err) + }) + } +} + +func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { + tests := []struct { + name string + handler PhaseHandler + object *v1alpha1.KeptnAppVersion + clientObject *v1alpha1.KeptnTask + phase common.KeptnPhaseType + want string + wantErr error + }{ + { + name: "no state failed", + handler: PhaseHandler{ + SpanHandler: &SpanHandler{}, + Log: ctrl.Log.WithName("controller"), + Recorder: record.NewFakeRecorder(100), + }, + object: &v1alpha1.KeptnAppVersion{ + ObjectMeta: v1.ObjectMeta{ + Name: "appversion", + Namespace: "namespace", + }, + Status: v1alpha1.KeptnAppVersionStatus{ + Status: common.StateSucceeded, + CurrentPhase: common.PhaseAppPreDeployment.LongName, + PreDeploymentTaskStatus: []v1alpha1.TaskStatus{ + { + TaskName: "task-name", + Status: common.StateSucceeded, + }, + }, + }, + }, + clientObject: &v1alpha1.KeptnTask{}, + phase: common.PhaseAppPreDeployment, + want: "", + wantErr: nil, + }, + { + name: "cannot get task", + handler: PhaseHandler{ + SpanHandler: &SpanHandler{}, + Log: ctrl.Log.WithName("controller"), + Recorder: record.NewFakeRecorder(100), + }, + object: &v1alpha1.KeptnAppVersion{ + ObjectMeta: v1.ObjectMeta{ + Name: "appversion", + Namespace: "namespace", + }, + Status: v1alpha1.KeptnAppVersionStatus{ + Status: common.StateFailed, + CurrentPhase: common.PhaseAppPreDeployment.LongName, + PreDeploymentTaskStatus: []v1alpha1.TaskStatus{ + { + TaskName: "task-name", + Status: common.StateFailed, + }, + }, + }, + }, + clientObject: &v1alpha1.KeptnTask{}, + phase: common.PhaseAppPreDeployment, + want: "", + wantErr: fmt.Errorf("task task-name not found"), + }, + { + name: "task failed", + handler: PhaseHandler{ + SpanHandler: &SpanHandler{}, + Log: ctrl.Log.WithName("controller"), + Recorder: record.NewFakeRecorder(100), + }, + object: &v1alpha1.KeptnAppVersion{ + ObjectMeta: v1.ObjectMeta{ + Name: "appversion", + Namespace: "namespace", + }, + Status: v1alpha1.KeptnAppVersionStatus{ + Status: common.StateFailed, + CurrentPhase: common.PhaseAppPreDeployment.LongName, + PreDeploymentTaskStatus: []v1alpha1.TaskStatus{ + { + TaskName: "task-name", + Status: common.StateFailed, + }, + }, + }, + }, + clientObject: &v1alpha1.KeptnTask{ + ObjectMeta: v1.ObjectMeta{ + Name: "task-name", + Namespace: "namespace", + }, + Status: v1alpha1.KeptnTaskStatus{ + Status: common.StateFailed, + Message: "task failed", + }, + }, + phase: common.PhaseAppPreDeployment, + want: "task 'task-name' failed with reason: 'task failed'\n", + wantErr: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v1alpha1.AddToScheme(scheme.Scheme) + client := fake.NewClientBuilder().WithObjects(tt.clientObject).Build() + tt.handler.Client = client + result, err := tt.handler.GetTaskFailureReasons(context.TODO(), tt.phase, tt.object) + require.Equal(t, tt.want, result) + require.Equal(t, tt.wantErr, err) + }) + } +} From afa701a213f5750cbfa4bd6fe219dc94d5635584 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 21 Nov 2022 13:13:19 +0100 Subject: [PATCH 08/12] pr review Signed-off-by: odubajDT --- operator/controllers/common/phasehandler.go | 43 ++++++++++++------- .../controllers/common/phasehandler_test.go | 22 +++++----- 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index 127f6260ee..09aad6ca20 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -115,17 +115,28 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, return &PhaseResult{Continue: false, Result: requeueResult}, nil } -func (r PhaseHandler) createFailureReasonSpanEvents(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper) (string, error) { +func (r PhaseHandler) createFailureReasonSpanEvents(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper, spanTrace trace.Span) error { + var messages []string + var err error if phase.IsEvaluation() { - return r.GetEvaluationFailureReasons(ctx, phase, object) + messages, err = r.GetEvaluationFailureReasons(ctx, phase, object) } else if phase.IsTask() { - return r.GetTaskFailureReasons(ctx, phase, object) + messages, err = r.GetTaskFailureReasons(ctx, phase, object) } - return "", nil + + if err != nil { + return err + } + + for _, msg := range messages { + spanTrace.AddEvent(msg) + } + + return nil } -func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) (string, error) { - resultMsg := "" +func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) ([]string, error) { + resultMsgs := []string{} var status []klcv1alpha1.EvaluationStatus if phase.IsPreEvaluation() { status = object.GetPreDeploymentEvaluationTaskStatus() @@ -136,25 +147,26 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com // there can be only one evaluation and in this section of the code, it can only be failed // checking length of the status only for safety reasons if len(status) != 1 { - return "", fmt.Errorf("evaluation status not found") + return []string{}, fmt.Errorf("evaluation status not found for %s/%s", object.GetAppName(), object.GetParentName()) } evaluation := &klcv1alpha1.KeptnEvaluation{} if err := r.Client.Get(ctx, types.NamespacedName{Name: status[0].EvaluationName, Namespace: object.GetNamespace()}, evaluation); err != nil { - return "", fmt.Errorf("evaluation %s not found", status[0].EvaluationName) + return []string{}, fmt.Errorf("evaluation %s not found for %s/%s", status[0].EvaluationName, object.GetAppName(), object.GetParentName()) } for k, v := range evaluation.Status.EvaluationStatus { if v.Status == common.StateFailed { - resultMsg = resultMsg + fmt.Sprintf("evaluation of '%s' failed with value: '%s' and reason: '%s'\n", k, v.Value, v.Message) + msg := fmt.Sprintf("evaluation of '%s' failed with value: '%s' and reason: '%s'\n", k, v.Value, v.Message) + resultMsgs = append(resultMsgs, msg) } } - return resultMsg, nil + return resultMsgs, nil } -func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) (string, error) { - resultMsg := "" +func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) ([]string, error) { + resultMsgs := []string{} var failedTasks []klcv1alpha1.KeptnTask var status []klcv1alpha1.TaskStatus if phase.IsPreTask() { @@ -167,15 +179,16 @@ func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.Ke if item.Status == common.StateFailed { task := &klcv1alpha1.KeptnTask{} if err := r.Client.Get(ctx, types.NamespacedName{Name: item.TaskName, Namespace: object.GetNamespace()}, task); err != nil { - return "", fmt.Errorf("task %s not found", item.TaskName) + return []string{}, fmt.Errorf("task %s not found for %s/%s", item.TaskName, object.GetAppName(), object.GetParentName()) } failedTasks = append(failedTasks, *task) } } for _, task := range failedTasks { - resultMsg = resultMsg + fmt.Sprintf("task '%s' failed with reason: '%s'\n", task.Name, task.Status.Message) + msg := fmt.Sprintf("task '%s' failed with reason: '%s'\n", task.Name, task.Status.Message) + resultMsgs = append(resultMsgs, msg) } - return resultMsg, nil + return resultMsgs, nil } diff --git a/operator/controllers/common/phasehandler_test.go b/operator/controllers/common/phasehandler_test.go index 4dd8f07e49..c4ea3bf0f3 100644 --- a/operator/controllers/common/phasehandler_test.go +++ b/operator/controllers/common/phasehandler_test.go @@ -233,7 +233,7 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { object *v1alpha1.KeptnAppVersion clientObject *v1alpha1.KeptnEvaluation phase common.KeptnPhaseType - want string + want []string wantErr error }{ { @@ -251,8 +251,8 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { }, clientObject: &v1alpha1.KeptnEvaluation{}, phase: common.PhaseAppPreEvaluation, - want: "", - wantErr: fmt.Errorf("evaluation status not found"), + want: []string{}, + wantErr: fmt.Errorf("evaluation status not found for /"), }, { name: "cannot get evaluation", @@ -278,8 +278,8 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { }, clientObject: &v1alpha1.KeptnEvaluation{}, phase: common.PhaseAppPreEvaluation, - want: "", - wantErr: fmt.Errorf("evaluation eval-name not found"), + want: []string{}, + wantErr: fmt.Errorf("evaluation eval-name not found for /"), }, { name: "evaluation failed", @@ -324,7 +324,7 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { }, }, phase: common.PhaseAppPreEvaluation, - want: "evaluation of 'cpu' failed with value: '10' and reason: 'cpu failed'\n", + want: []string{"evaluation of 'cpu' failed with value: '10' and reason: 'cpu failed'\n"}, wantErr: nil, }, } @@ -348,7 +348,7 @@ func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { object *v1alpha1.KeptnAppVersion clientObject *v1alpha1.KeptnTask phase common.KeptnPhaseType - want string + want []string wantErr error }{ { @@ -376,7 +376,7 @@ func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { }, clientObject: &v1alpha1.KeptnTask{}, phase: common.PhaseAppPreDeployment, - want: "", + want: []string{}, wantErr: nil, }, { @@ -404,8 +404,8 @@ func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { }, clientObject: &v1alpha1.KeptnTask{}, phase: common.PhaseAppPreDeployment, - want: "", - wantErr: fmt.Errorf("task task-name not found"), + want: []string{}, + wantErr: fmt.Errorf("task task-name not found for /"), }, { name: "task failed", @@ -441,7 +441,7 @@ func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { }, }, phase: common.PhaseAppPreDeployment, - want: "task 'task-name' failed with reason: 'task failed'\n", + want: []string{"task 'task-name' failed with reason: 'task failed'\n"}, wantErr: nil, }, } From ee7920c6e9bd6262aeee6cac14392b0b12f3375e Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 21 Nov 2022 13:17:42 +0100 Subject: [PATCH 09/12] rebase Signed-off-by: odubajDT --- operator/controllers/common/phasehandler.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index 09aad6ca20..38a276f144 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -81,11 +81,8 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, piWrapper.Complete() piWrapper.SetState(common.StateFailed) spanAppTrace.AddEvent(phase.LongName + " has failed") - msg, err := r.CreateFailureReasonMessages(ctx, phase, piWrapper) - if err != nil { + if err := r.createFailureReasonSpanEvents(ctx, phase, piWrapper, spanAppTrace); err != nil { r.Log.Error(err, "cannot create failure spans") - } else { - spanAppTrace.AddEvent(msg) } spanAppTrace.SetStatus(codes.Error, "Failed") spanAppTrace.End() From 7bf48178216fcac8a2f94e3a87c6b2d2f13db562 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 21 Nov 2022 13:20:07 +0100 Subject: [PATCH 10/12] fixed error msg Signed-off-by: odubajDT --- operator/controllers/common/phasehandler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index 38a276f144..04b1d9eadc 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -82,7 +82,7 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, piWrapper.SetState(common.StateFailed) spanAppTrace.AddEvent(phase.LongName + " has failed") if err := r.createFailureReasonSpanEvents(ctx, phase, piWrapper, spanAppTrace); err != nil { - r.Log.Error(err, "cannot create failure spans") + r.Log.Error(err, "cannot add failure events to spans") } spanAppTrace.SetStatus(codes.Error, "Failed") spanAppTrace.End() From dedd2dc59659398c759d6b96c4afc39995362033 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 21 Nov 2022 14:24:06 +0100 Subject: [PATCH 11/12] pr review Signed-off-by: odubajDT --- operator/controllers/common/phasehandler.go | 47 ++++++++++++------- .../controllers/common/phasehandler_test.go | 32 +++++++++---- 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index 04b1d9eadc..b17a66374a 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -28,6 +28,11 @@ type PhaseResult struct { ctrl.Result } +type failedCheckReason struct { + Message string + Time time.Time +} + func RecordEvent(recorder record.EventRecorder, phase common.KeptnPhaseType, eventType string, reconcileObject client.Object, shortReason string, longReason string, version string) { recorder.Event(reconcileObject, eventType, fmt.Sprintf("%s%s", phase.ShortName, shortReason), fmt.Sprintf("%s %s / Namespace: %s, Name: %s, Version: %s ", phase.LongName, longReason, reconcileObject.GetNamespace(), reconcileObject.GetName(), version)) } @@ -113,27 +118,27 @@ func (r PhaseHandler) HandlePhase(ctx context.Context, ctxTrace context.Context, } func (r PhaseHandler) createFailureReasonSpanEvents(ctx context.Context, phase common.KeptnPhaseType, object *PhaseItemWrapper, spanTrace trace.Span) error { - var messages []string + var messageObjects []failedCheckReason var err error if phase.IsEvaluation() { - messages, err = r.GetEvaluationFailureReasons(ctx, phase, object) + messageObjects, err = r.GetEvaluationFailureReasons(ctx, phase, object) } else if phase.IsTask() { - messages, err = r.GetTaskFailureReasons(ctx, phase, object) + messageObjects, err = r.GetTaskFailureReasons(ctx, phase, object) } if err != nil { return err } - for _, msg := range messages { - spanTrace.AddEvent(msg) + for _, msgObject := range messageObjects { + spanTrace.AddEvent(msgObject.Message, trace.WithTimestamp(msgObject.Time)) } return nil } -func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) ([]string, error) { - resultMsgs := []string{} +func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) ([]failedCheckReason, error) { + resultEvents := []failedCheckReason{} var status []klcv1alpha1.EvaluationStatus if phase.IsPreEvaluation() { status = object.GetPreDeploymentEvaluationTaskStatus() @@ -144,26 +149,29 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com // there can be only one evaluation and in this section of the code, it can only be failed // checking length of the status only for safety reasons if len(status) != 1 { - return []string{}, fmt.Errorf("evaluation status not found for %s/%s", object.GetAppName(), object.GetParentName()) + return nil, fmt.Errorf("evaluation status not found for %s/%s", object.GetAppName(), object.GetParentName()) } evaluation := &klcv1alpha1.KeptnEvaluation{} if err := r.Client.Get(ctx, types.NamespacedName{Name: status[0].EvaluationName, Namespace: object.GetNamespace()}, evaluation); err != nil { - return []string{}, fmt.Errorf("evaluation %s not found for %s/%s", status[0].EvaluationName, object.GetAppName(), object.GetParentName()) + return nil, fmt.Errorf("evaluation %s not found for %s/%s", status[0].EvaluationName, object.GetAppName(), object.GetParentName()) } for k, v := range evaluation.Status.EvaluationStatus { if v.Status == common.StateFailed { - msg := fmt.Sprintf("evaluation of '%s' failed with value: '%s' and reason: '%s'\n", k, v.Value, v.Message) - resultMsgs = append(resultMsgs, msg) + obj := failedCheckReason{ + Time: evaluation.Status.EndTime.Time, + Message: fmt.Sprintf("evaluation of '%s' failed with value: '%s' and reason: '%s'\n", k, v.Value, v.Message), + } + resultEvents = append(resultEvents, obj) } } - return resultMsgs, nil + return resultEvents, nil } -func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) ([]string, error) { - resultMsgs := []string{} +func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.KeptnPhaseType, object PhaseItem) ([]failedCheckReason, error) { + resultEvents := []failedCheckReason{} var failedTasks []klcv1alpha1.KeptnTask var status []klcv1alpha1.TaskStatus if phase.IsPreTask() { @@ -176,16 +184,19 @@ func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.Ke if item.Status == common.StateFailed { task := &klcv1alpha1.KeptnTask{} if err := r.Client.Get(ctx, types.NamespacedName{Name: item.TaskName, Namespace: object.GetNamespace()}, task); err != nil { - return []string{}, fmt.Errorf("task %s not found for %s/%s", item.TaskName, object.GetAppName(), object.GetParentName()) + return nil, fmt.Errorf("task %s not found for %s/%s", item.TaskName, object.GetAppName(), object.GetParentName()) } failedTasks = append(failedTasks, *task) } } for _, task := range failedTasks { - msg := fmt.Sprintf("task '%s' failed with reason: '%s'\n", task.Name, task.Status.Message) - resultMsgs = append(resultMsgs, msg) + obj := failedCheckReason{ + Time: task.Status.EndTime.Time, + Message: fmt.Sprintf("task '%s' failed with reason: '%s'\n", task.Name, task.Status.Message), + } + resultEvents = append(resultEvents, obj) } - return resultMsgs, nil + return resultEvents, nil } diff --git a/operator/controllers/common/phasehandler_test.go b/operator/controllers/common/phasehandler_test.go index c4ea3bf0f3..7a8e231d50 100644 --- a/operator/controllers/common/phasehandler_test.go +++ b/operator/controllers/common/phasehandler_test.go @@ -233,7 +233,7 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { object *v1alpha1.KeptnAppVersion clientObject *v1alpha1.KeptnEvaluation phase common.KeptnPhaseType - want []string + want []failedCheckReason wantErr error }{ { @@ -251,7 +251,7 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { }, clientObject: &v1alpha1.KeptnEvaluation{}, phase: common.PhaseAppPreEvaluation, - want: []string{}, + want: nil, wantErr: fmt.Errorf("evaluation status not found for /"), }, { @@ -278,7 +278,7 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { }, clientObject: &v1alpha1.KeptnEvaluation{}, phase: common.PhaseAppPreEvaluation, - want: []string{}, + want: nil, wantErr: fmt.Errorf("evaluation eval-name not found for /"), }, { @@ -309,6 +309,7 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { Namespace: "namespace", }, Status: v1alpha1.KeptnEvaluationStatus{ + EndTime: v1.Time{Time: time.Date(1, 1, 1, 1, 1, 1, 0, time.Local)}, EvaluationStatus: map[string]v1alpha1.EvaluationStatusItem{ "cpu": { Value: "10", @@ -323,8 +324,13 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { }, }, }, - phase: common.PhaseAppPreEvaluation, - want: []string{"evaluation of 'cpu' failed with value: '10' and reason: 'cpu failed'\n"}, + phase: common.PhaseAppPreEvaluation, + want: []failedCheckReason{ + { + Message: "evaluation of 'cpu' failed with value: '10' and reason: 'cpu failed'\n", + Time: time.Date(1, 1, 1, 1, 1, 1, 0, time.Local), + }, + }, wantErr: nil, }, } @@ -348,7 +354,7 @@ func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { object *v1alpha1.KeptnAppVersion clientObject *v1alpha1.KeptnTask phase common.KeptnPhaseType - want []string + want []failedCheckReason wantErr error }{ { @@ -376,7 +382,7 @@ func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { }, clientObject: &v1alpha1.KeptnTask{}, phase: common.PhaseAppPreDeployment, - want: []string{}, + want: []failedCheckReason{}, wantErr: nil, }, { @@ -404,7 +410,7 @@ func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { }, clientObject: &v1alpha1.KeptnTask{}, phase: common.PhaseAppPreDeployment, - want: []string{}, + want: nil, wantErr: fmt.Errorf("task task-name not found for /"), }, { @@ -438,10 +444,16 @@ func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { Status: v1alpha1.KeptnTaskStatus{ Status: common.StateFailed, Message: "task failed", + EndTime: v1.Time{Time: time.Date(1, 1, 1, 1, 1, 1, 0, time.Local)}, + }, + }, + phase: common.PhaseAppPreDeployment, + want: []failedCheckReason{ + { + Message: "task 'task-name' failed with reason: 'task failed'\n", + Time: time.Date(1, 1, 1, 1, 1, 1, 0, time.Local), }, }, - phase: common.PhaseAppPreDeployment, - want: []string{"task 'task-name' failed with reason: 'task failed'\n"}, wantErr: nil, }, } From 834c127520bf24b4fdaf4b6f74668505aeff7579 Mon Sep 17 00:00:00 2001 From: odubajDT Date: Mon, 21 Nov 2022 14:58:17 +0100 Subject: [PATCH 12/12] minor fix Signed-off-by: odubajDT --- operator/controllers/common/phasehandler.go | 4 ++-- operator/controllers/common/phasehandler_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/operator/controllers/common/phasehandler.go b/operator/controllers/common/phasehandler.go index b17a66374a..f784a2458b 100644 --- a/operator/controllers/common/phasehandler.go +++ b/operator/controllers/common/phasehandler.go @@ -161,7 +161,7 @@ func (r PhaseHandler) GetEvaluationFailureReasons(ctx context.Context, phase com if v.Status == common.StateFailed { obj := failedCheckReason{ Time: evaluation.Status.EndTime.Time, - Message: fmt.Sprintf("evaluation of '%s' failed with value: '%s' and reason: '%s'\n", k, v.Value, v.Message), + Message: fmt.Sprintf("evaluation of '%s' failed with value: '%s' and reason: '%s'", k, v.Value, v.Message), } resultEvents = append(resultEvents, obj) } @@ -193,7 +193,7 @@ func (r PhaseHandler) GetTaskFailureReasons(ctx context.Context, phase common.Ke for _, task := range failedTasks { obj := failedCheckReason{ Time: task.Status.EndTime.Time, - Message: fmt.Sprintf("task '%s' failed with reason: '%s'\n", task.Name, task.Status.Message), + Message: fmt.Sprintf("task '%s' failed with reason: '%s'", task.Name, task.Status.Message), } resultEvents = append(resultEvents, obj) } diff --git a/operator/controllers/common/phasehandler_test.go b/operator/controllers/common/phasehandler_test.go index 7a8e231d50..81e9690509 100644 --- a/operator/controllers/common/phasehandler_test.go +++ b/operator/controllers/common/phasehandler_test.go @@ -327,7 +327,7 @@ func TestPhaseHandler_GetEvaluationFailureReasons(t *testing.T) { phase: common.PhaseAppPreEvaluation, want: []failedCheckReason{ { - Message: "evaluation of 'cpu' failed with value: '10' and reason: 'cpu failed'\n", + Message: "evaluation of 'cpu' failed with value: '10' and reason: 'cpu failed'", Time: time.Date(1, 1, 1, 1, 1, 1, 0, time.Local), }, }, @@ -450,7 +450,7 @@ func TestPhaseHandler_GetTaskFailureReasons(t *testing.T) { phase: common.PhaseAppPreDeployment, want: []failedCheckReason{ { - Message: "task 'task-name' failed with reason: 'task failed'\n", + Message: "task 'task-name' failed with reason: 'task failed'", Time: time.Date(1, 1, 1, 1, 1, 1, 0, time.Local), }, },