From 531bc0cf6b452bce7540f8ba858a704f4505d768 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Fri, 31 Jul 2020 07:38:42 -0700 Subject: [PATCH] Address PR --- manifests/crds/rollout-crd.yaml | 2 ++ .../rollouts/v1alpha1/openapi_generated.go | 9 ++++++- pkg/apis/rollouts/v1alpha1/types.go | 14 +++++------ rollout/analysis.go | 2 +- rollout/analysis_test.go | 25 +++++++++++-------- rollout/bluegreen_test.go | 3 ++- rollout/canary_test.go | 6 +++-- rollout/experiment_test.go | 5 ++-- rollout/pause.go | 8 +++--- 9 files changed, 47 insertions(+), 27 deletions(-) diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index ea567f5a81..e3d0ef6337 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -2909,6 +2909,8 @@ spec: type: integer abort: type: boolean + abortedAt: + type: boolean availableReplicas: format: int32 type: integer diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index 5336004ddd..8c87275554 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -787,7 +787,7 @@ func schema_pkg_apis_rollouts_v1alpha1_CanaryStatus(ref common.ReferenceCallback }, "currentBackgroundAnalysisRunStatus": { SchemaProps: spec.SchemaProps{ - Description: "CurrentStepAnalysisRunStatus indicates the status of the current background analysis run", + Description: "CurrentBackgroundAnalysisRunStatus indicates the status of the current background analysis run", Ref: ref("github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutAnalysisRunStatus"), }, }, @@ -2702,6 +2702,13 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutStatus(ref common.ReferenceCallbac Format: "", }, }, + "abortedAt": { + SchemaProps: spec.SchemaProps{ + Description: "AbortedAt indicates the controller reconciled an aborted rollout. The controller uses this to understand if the controller needs to do some specific work when a Rollout is aborted. For example, the reconcileAbort is used to indicate if the Rollout should enter an aborted state when the latest AnalysisRun is a failure, or the controller has already put the Rollout into an aborted and should create a new AnalysisRun.", + Type: []string{"boolean"}, + Format: "", + }, + }, "currentPodHash": { SchemaProps: spec.SchemaProps{ Description: "CurrentPodHash the hash of the current pod template", diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index aaae5659b4..fcf0149848 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -476,11 +476,11 @@ type RolloutStatus struct { PauseConditions []PauseCondition `json:"pauseConditions,omitempty"` //ControllerPause indicates the controller has paused the rollout ControllerPause bool `json:"controllerPause,omitempty"` - // ReconciledAbort indicates the controller reconciled an aborted rollout. The controller uses this to understand if + // AbortedAt indicates the controller reconciled an aborted rollout. The controller uses this to understand if // the controller needs to do some specific work when a Rollout is aborted. For example, the reconcileAbort is used // to indicate if the Rollout should enter an aborted state when the latest AnalysisRun is a failure, or the controller // has already put the Rollout into an aborted and should create a new AnalysisRun. - ReconciledAbort bool `json:"reconciledAbort,omitempty"` + AbortedAt *metav1.Time `json:"abortedAt,omitempty"` // CurrentPodHash the hash of the current pod template // +optional CurrentPodHash string `json:"currentPodHash,omitempty"` @@ -555,12 +555,12 @@ type BlueGreenStatus struct { // +optional ScaleUpPreviewCheckPoint bool `json:"scaleUpPreviewCheckPoint,omitempty"` // PrePromotionAnalysisRun is the current analysis run running before the active service promotion - // TODO(Depreciated): Remove in v0.10 + // TODO(Deprecated): Remove in v0.10 PrePromotionAnalysisRun string `json:"prePromotionAnalysisRun,omitempty"` // PrePromotionAnalysisRunStatus indicates the status of the current prepromotion analysis run PrePromotionAnalysisRunStatus *RolloutAnalysisRunStatus `json:"prePromotionAnalysisRunStatus,omitempty"` // PostPromotionAnalysisRun is the current analysis run running after the active service promotion - // TODO(Depreciated): Remove in v0.10 + // TODO(Deprecated): Remove in v0.10 PostPromotionAnalysisRun string `json:"postPromotionAnalysisRun,omitempty"` // PostPromotionAnalysisRunStatus indicates the status of the current post promotion analysis run PostPromotionAnalysisRunStatus *RolloutAnalysisRunStatus `json:"postPromotionAnalysisRunStatus,omitempty"` @@ -572,14 +572,14 @@ type CanaryStatus struct { // +optional StableRS string `json:"stableRS,omitempty"` // CurrentStepAnalysisRun indicates the analysisRun for the current step index - // TODO(Depreciated): Remove in v0.10 + // TODO(Deprecated): Remove in v0.10 CurrentStepAnalysisRun string `json:"currentStepAnalysisRun,omitempty"` // CurrentStepAnalysisRunStatus indicates the status of the current step analysis run CurrentStepAnalysisRunStatus *RolloutAnalysisRunStatus `json:"currentStepAnalysisRunStatus,omitempty"` // CurrentBackgroundAnalysisRun indicates the analysisRun for the Background step - // TODO(Depreciated): Remove in v0.10 + // TODO(Deprecated): Remove in v0.10 CurrentBackgroundAnalysisRun string `json:"currentBackgroundAnalysisRun,omitempty"` - // CurrentStepAnalysisRunStatus indicates the status of the current background analysis run + // CurrentBackgroundAnalysisRunStatus indicates the status of the current background analysis run CurrentBackgroundAnalysisRunStatus *RolloutAnalysisRunStatus `json:"currentBackgroundAnalysisRunStatus,omitempty"` // CurrentExperiment indicates the running experiment CurrentExperiment string `json:"currentExperiment,omitempty"` diff --git a/rollout/analysis.go b/rollout/analysis.go index 2d99e58893..f126434829 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -130,7 +130,7 @@ func needsNewAnalysisRun(currentAr *v1alpha1.AnalysisRun, rollout *v1alpha1.Roll if rollout.Status.ControllerPause && getPauseCondition(rollout, v1alpha1.PauseReasonBlueGreenPause) == nil { return currentAr.Status.Phase == v1alpha1.AnalysisPhaseInconclusive } - return rollout.Status.ReconciledAbort + return rollout.Status.AbortedAt != nil } // emitAnalysisRunStatusChanges emits a Kubernetes event if the analysis run of that type has changed status diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index a115436bf1..5dfee4d50f 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -1194,12 +1194,13 @@ func TestErrorConditionAfterErrorAnalysisRunStep(t *testing.T) { }, "conditions": %s, "abort": true, - "reconciledAbort": true + "abortedAt": "%s" } }` + now := metav1.Now().UTC().Format(time.RFC3339) condition := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, ar.Status.Message) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ar.Name, condition)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ar.Name, condition, now)), patch) } func TestErrorConditionAfterErrorAnalysisRunBackground(t *testing.T) { @@ -1259,13 +1260,14 @@ func TestErrorConditionAfterErrorAnalysisRunBackground(t *testing.T) { } }, "conditions": %s, - "reconciledAbort": true, + "abortedAt": "%s", "abort": true } }` condition := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "") - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition)), patch) + now := metav1.Now().UTC().Format(time.RFC3339) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, now)), patch) } func TestCancelAnalysisRunsWhenAborted(t *testing.T) { @@ -1316,10 +1318,11 @@ func TestCancelAnalysisRunsWhenAborted(t *testing.T) { expectedPatch := `{ "status": { "conditions": %s, - "reconciledAbort": true + "abortedAt": "%s" } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, newConditions)), patch) + now := metav1.Now().UTC().Format(time.RFC3339) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, newConditions, now)), patch) } func TestCancelBackgroundAnalysisRunWhenRolloutIsCompleted(t *testing.T) { @@ -1879,7 +1882,7 @@ func TestAbortRolloutOnErrorPrePromotionAnalysis(t *testing.T) { expectedPatch := `{ "status": { "abort": true, - "reconciledAbort": true, + "abortedAt": "%s", "pauseConditions": null, "controllerPause":null, "blueGreen": { @@ -1889,7 +1892,8 @@ func TestAbortRolloutOnErrorPrePromotionAnalysis(t *testing.T) { } } }` - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + now := metav1.Now().UTC().Format(time.RFC3339) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now)), patch) } func TestCreatePostPromotionAnalysisRun(t *testing.T) { @@ -2096,7 +2100,7 @@ func TestAbortRolloutOnErrorPostPromotionAnalysis(t *testing.T) { expectedPatch := `{ "status": { "abort": true, - "reconciledAbort": true, + "abortedAt": "%s", "pauseConditions": null, "controllerPause":null, "blueGreen": { @@ -2106,5 +2110,6 @@ func TestAbortRolloutOnErrorPostPromotionAnalysis(t *testing.T) { } } }` - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + now := metav1.Now().UTC().Format(time.RFC3339) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now)), patch) } diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 61b69932d9..f954f60766 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -1196,7 +1196,8 @@ func TestBlueGreenAbort(t *testing.T) { r1 := newBlueGreenRollout("foo", 1, nil, "bar", "") r2 := bumpVersion(r1) r2.Status.Abort = true - r2.Status.ReconciledAbort = true + now := metav1.Now() + r2.Status.AbortedAt = &now rs1 := newReplicaSetWithStatus(r1, 1, 1) rs2 := newReplicaSetWithStatus(r2, 1, 1) diff --git a/rollout/canary_test.go b/rollout/canary_test.go index ae1a7ebfe8..35742b69e1 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -1258,7 +1258,8 @@ func TestHandleCanaryAbort(t *testing.T) { r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 10, 1, 10, false) r2.Status.Abort = true - r2.Status.ReconciledAbort = true + now := metav1.Now() + r2.Status.AbortedAt = &now f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -1291,7 +1292,8 @@ func TestHandleCanaryAbort(t *testing.T) { } r1 := newCanaryRollout("foo", 2, nil, steps, int32Ptr(3), intstr.FromInt(1), intstr.FromInt(0)) r1.Status.Abort = true - r1.Status.ReconciledAbort = true + now := metav1.Now() + r1.Status.AbortedAt = &now rs1 := newReplicaSetWithStatus(r1, 2, 2) rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r1 = updateCanaryRolloutStatus(r1, rs1PodHash, 2, 2, 2, false) diff --git a/rollout/experiment_test.go b/rollout/experiment_test.go index fd3acc4bec..1999a6c1fe 100644 --- a/rollout/experiment_test.go +++ b/rollout/experiment_test.go @@ -299,15 +299,16 @@ func TestAbortRolloutAfterFailedExperiment(t *testing.T) { expectedPatch := `{ "status": { "abort": true, - "reconciledAbort": true, + "abortedAt": "%s", "conditions": %s, "canary": { "currentExperiment": null } } }` + now := metav1.Now().UTC().Format(time.RFC3339) generatedConditons := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "") - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, generatedConditons)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now, generatedConditons)), patch) } func TestPauseRolloutAfterInconclusiveExperiment(t *testing.T) { diff --git a/rollout/pause.go b/rollout/pause.go index 1ed3fd5758..07f4bed5db 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -59,16 +59,18 @@ func (pCtx *pauseContext) ClearPauseConditions() { func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus) { if pCtx.addAbort { newStatus.Abort = true - newStatus.ReconciledAbort = true + now := metav1.Now() + newStatus.AbortedAt = &now return } if !pCtx.removeAbort && pCtx.rollout.Status.Abort { newStatus.Abort = true - newStatus.ReconciledAbort = true + now := metav1.Now() + newStatus.AbortedAt = &now return } newStatus.Abort = false - newStatus.ReconciledAbort = false + newStatus.AbortedAt = nil if pCtx.clearPauseConditions { return