From 653339dadfff33836e926811215f3e000992a53e Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Mon, 18 Nov 2024 16:41:02 +0100 Subject: [PATCH] fix(stages): set healthy status for promotions w/o checks Signed-off-by: Hidde Beydals --- internal/controller/stages/regular_stages.go | 40 +++++++-------- .../controller/stages/regular_stages_test.go | 50 ++++++++++++++++--- 2 files changed, 64 insertions(+), 26 deletions(-) diff --git a/internal/controller/stages/regular_stages.go b/internal/controller/stages/regular_stages.go index bbc330bfd..5856a9566 100644 --- a/internal/controller/stages/regular_stages.go +++ b/internal/controller/stages/regular_stages.go @@ -694,23 +694,27 @@ func (r *RegularStageReconciler) assessHealth(ctx context.Context, stage *kargoa return newStatus } - // If there are no health checks to perform, then we can skip the health - // assessment. - healthChecks := lastPromo.Status.HealthChecks - if len(healthChecks) == 0 { - logger.Debug("Stage has no health checks to perform for last Promotion") + // If the last Promotion did not succeed, then we cannot perform any health + // checks because they are only available after a successful Promotion. + // + // TODO(hidde): Long term, this should probably be changed to allow to + // continue to run health checks from the last successful Promotion, + // even if the current Promotion did not succeed (e.g. because it was + // aborted). + if lastPromo.Status.Phase != kargoapi.PromotionPhaseSucceeded { + logger.Debug("Last promotion did not succeed: no health checks to perform") conditions.Set(&newStatus, &metav1.Condition{ Type: kargoapi.ConditionTypeHealthy, Status: metav1.ConditionUnknown, - Reason: "NoHealthChecks", - Message: "Stage has no health checks to perform", + Reason: fmt.Sprintf("LastPromotion%s", lastPromo.Status.Phase), + Message: "No health checks to perform for unsuccessful Promotion", ObservedGeneration: stage.Generation, }) - newStatus.Health = nil return newStatus } // Compose the health check steps. + healthChecks := lastPromo.Status.HealthChecks var steps []directives.HealthCheckStep for _, step := range healthChecks { steps = append(steps, directives.HealthCheckStep{ @@ -733,22 +737,18 @@ func (r *RegularStageReconciler) assessHealth(ctx context.Context, stage *kargoa Type: kargoapi.ConditionTypeHealthy, Status: metav1.ConditionTrue, Reason: string(health.Status), - Message: "Stage is healthy", + Message: fmt.Sprintf("Stage is healthy (performed %d health checks)", len(healthChecks)), ObservedGeneration: stage.Generation, }) case kargoapi.HealthStateUnhealthy: conditions.Set(&newStatus, &metav1.Condition{ - Type: kargoapi.ConditionTypeHealthy, - Status: metav1.ConditionFalse, - Reason: string(health.Status), - Message: "Stage is unhealthy", - ObservedGeneration: stage.Generation, - }) - case kargoapi.HealthStateNotApplicable: - conditions.Set(&newStatus, &metav1.Condition{ - Type: kargoapi.ConditionTypeHealthy, - Status: metav1.ConditionUnknown, - Reason: string(health.Status), + Type: kargoapi.ConditionTypeHealthy, + Status: metav1.ConditionFalse, + Reason: string(health.Status), + Message: fmt.Sprintf( + "Stage is unhealthy (%d issues in %d health checks)", + len(health.Issues), len(healthChecks), + ), ObservedGeneration: stage.Generation, }) default: diff --git a/internal/controller/stages/regular_stages_test.go b/internal/controller/stages/regular_stages_test.go index 18b014c6b..1d8f83ab2 100644 --- a/internal/controller/stages/regular_stages_test.go +++ b/internal/controller/stages/regular_stages_test.go @@ -1204,7 +1204,7 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { }, }, { - name: "no health checks", + name: "unsuccessful last promotion", stage: &kargoapi.Stage{ ObjectMeta: metav1.ObjectMeta{ Namespace: "fake-project", @@ -1213,6 +1213,7 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { Status: kargoapi.StageStatus{ LastPromotion: &kargoapi.PromotionReference{ Status: &kargoapi.PromotionStatus{ + Phase: kargoapi.PromotionPhaseAborted, HealthChecks: nil, }, }, @@ -1224,8 +1225,35 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { healthyCond := conditions.Get(&status, kargoapi.ConditionTypeHealthy) require.NotNil(t, healthyCond) assert.Equal(t, metav1.ConditionUnknown, healthyCond.Status) - assert.Equal(t, "NoHealthChecks", healthyCond.Reason) - assert.Equal(t, "Stage has no health checks to perform", healthyCond.Message) + assert.Equal(t, "LastPromotionAborted", healthyCond.Reason) + assert.Equal(t, "No health checks to perform for unsuccessful Promotion", healthyCond.Message) + }, + }, + { + name: "no health checks", + stage: &kargoapi.Stage{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "fake-project", + Name: "test-stage", + }, + Status: kargoapi.StageStatus{ + LastPromotion: &kargoapi.PromotionReference{ + Status: &kargoapi.PromotionStatus{ + Phase: kargoapi.PromotionPhaseSucceeded, + HealthChecks: nil, + }, + }, + }, + }, + assertions: func(t *testing.T, status kargoapi.StageStatus) { + assert.NotNil(t, status.Health) + assert.Equal(t, kargoapi.HealthStateHealthy, status.Health.Status) + + healthyCond := conditions.Get(&status, kargoapi.ConditionTypeHealthy) + require.NotNil(t, healthyCond) + assert.Equal(t, metav1.ConditionTrue, healthyCond.Status) + assert.Equal(t, kargoapi.ConditionTypeHealthy, healthyCond.Reason) + assert.Contains(t, healthyCond.Message, "Stage is healthy") }, }, { @@ -1238,6 +1266,7 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { Status: kargoapi.StageStatus{ LastPromotion: &kargoapi.PromotionReference{ Status: &kargoapi.PromotionStatus{ + Phase: kargoapi.PromotionPhaseSucceeded, HealthChecks: []kargoapi.HealthCheckStep{ { Uses: "test-check", @@ -1262,7 +1291,7 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { require.NotNil(t, healthyCond) assert.Equal(t, metav1.ConditionTrue, healthyCond.Status) assert.Equal(t, string(kargoapi.HealthStateHealthy), healthyCond.Reason) - assert.Equal(t, "Stage is healthy", healthyCond.Message) + assert.Contains(t, healthyCond.Message, "Stage is healthy") }, }, { @@ -1275,6 +1304,7 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { Status: kargoapi.StageStatus{ LastPromotion: &kargoapi.PromotionReference{ Status: &kargoapi.PromotionStatus{ + Phase: kargoapi.PromotionPhaseSucceeded, HealthChecks: []kargoapi.HealthCheckStep{ { Uses: "test-check", @@ -1289,7 +1319,12 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { directives.HealthCheckContext, []directives.HealthCheckStep, ) kargoapi.Health { - return kargoapi.Health{Status: kargoapi.HealthStateUnhealthy} + return kargoapi.Health{ + Status: kargoapi.HealthStateUnhealthy, + Issues: []string{ + "issue-1", "issue-2", + }, + } }, assertions: func(t *testing.T, status kargoapi.StageStatus) { require.NotNil(t, status.Health) @@ -1299,7 +1334,8 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { require.NotNil(t, healthyCond) assert.Equal(t, metav1.ConditionFalse, healthyCond.Status) assert.Equal(t, string(kargoapi.HealthStateUnhealthy), healthyCond.Reason) - assert.Equal(t, "Stage is unhealthy", healthyCond.Message) + assert.Contains(t, healthyCond.Message, "Stage is unhealthy") + assert.Contains(t, healthyCond.Message, "2 issues in 1 health check") }, }, { @@ -1313,6 +1349,7 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { Conditions: []metav1.Condition{}, LastPromotion: &kargoapi.PromotionReference{ Status: &kargoapi.PromotionStatus{ + Phase: kargoapi.PromotionPhaseSucceeded, HealthChecks: []kargoapi.HealthCheckStep{ { Uses: "test-check", @@ -1348,6 +1385,7 @@ func TestRegularStageReconciler_assessHealth(t *testing.T) { Status: kargoapi.StageStatus{ LastPromotion: &kargoapi.PromotionReference{ Status: &kargoapi.PromotionStatus{ + Phase: kargoapi.PromotionPhaseSucceeded, HealthChecks: []kargoapi.HealthCheckStep{ { Uses: "test-check",