Remove redundant Instance.Status.AggregatedStatus field (#1474)

Summary: #1442 introduced new `Instance.Spec.PlanExecution.Status` field which duplicates part of the `Instance.Spec.PlanStatus` information . With that, plan execution status is now saved three times: in `planExecution`, `aggregatedStatus` and the corresponding `planStatus` fields e.g.: ```yaml apiVersion: kudo.dev/v1beta1 kind: Instance spec: planExecution: planName: deploy status: IN_PROGRESS status: aggregatedStatus: activePlanName: deploy status: IN_PROGRESS planStatus: deploy: name: deploy phases: - name: deploy status: IN_PROGRESS steps: - name: app status: IN_PROGRESS status: IN_PROGRESS ``` `aggregatedStatus` is completely duplicating `planExecution` while `planStatus` provides extended information on individual phases and steps. This PR removes `aggregatedStatus` field in favor of `planExecution`. The only caveat is that we previously kept `aggregatedStatus.status` value even **after** the plan was finished to signal how _last_ plan execution ended. However, `kudo plan status` already calculates and displays this information from the corresponding `planStatus`. Additionally, having `aggregatedStatus.activePlanName` populated when no plan is active is somewhat broken, to begin with. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
kudobuilder · Apr 30, 2020 · e9cfc2b · e9cfc2b
1 parent b92b50c
commit e9cfc2b
Show file tree

Hide file tree

Showing 48 changed files with 73 additions and 170 deletions.
diff --git a/config/crds/kudo.dev_instances.yaml b/config/crds/kudo.dev_instances.yaml
@@ -108,16 +108,6 @@ spec:
         status:
           description: InstanceStatus defines the observed state of Instance
           properties:
-            aggregatedStatus:
-              description: AggregatedStatus is overview of an instance status derived
-                from the plan status
-              properties:
-                activePlanName:
-                  type: string
-                status:
-                  description: ExecutionStatus captures the state of the rollout.
-                  type: string
-              type: object
             planStatus:
               additionalProperties:
                 description: "PlanStatus is representing status of a plan \n These

diff --git a/pkg/apis/kudo/v1beta1/instance_types.go b/pkg/apis/kudo/v1beta1/instance_types.go
@@ -55,14 +55,7 @@ type PlanExecution struct {
 // InstanceStatus defines the observed state of Instance
 type InstanceStatus struct {
 	// slice would be enough here but we cannot use slice because order of sequence in yaml is considered significant while here it's not
-	PlanStatus       map[string]PlanStatus `json:"planStatus,omitempty"`
-	AggregatedStatus AggregatedStatus      `json:"aggregatedStatus,omitempty"`
-}
-
-// AggregatedStatus is overview of an instance status derived from the plan status
-type AggregatedStatus struct {
-	Status         ExecutionStatus `json:"status,omitempty"`
-	ActivePlanName string          `json:"activePlanName,omitempty"`
+	PlanStatus map[string]PlanStatus `json:"planStatus,omitempty"`
 }
 
 // PlanStatus is representing status of a plan

diff --git a/pkg/apis/kudo/v1beta1/instance_types_helpers.go b/pkg/apis/kudo/v1beta1/instance_types_helpers.go
@@ -67,7 +67,6 @@ func (i *Instance) UpdateInstanceStatus(planStatus *PlanStatus, updatedTimestamp
 			planStatus.LastUpdatedTimestamp = updatedTimestamp
 			i.Status.PlanStatus[k] = *planStatus
 			i.Spec.PlanExecution.Status = planStatus.Status
-			i.Status.AggregatedStatus.Status = planStatus.Status
 		}
 	}
 }

diff --git a/pkg/apis/kudo/v1beta1/instance_types_helpers_test.go b/pkg/apis/kudo/v1beta1/instance_types_helpers_test.go
@@ -115,10 +115,6 @@ func TestInstance_ResetPlanStatus(t *testing.T) {
 					Phases: []PhaseStatus{{Name: "phase", Status: ExecutionNeverRun, Steps: []StepStatus{{Status: ExecutionNeverRun, Name: "step"}}}},
 				},
 			},
-			AggregatedStatus: AggregatedStatus{
-				Status:         ExecutionInProgress,
-				ActivePlanName: "deploy",
-			},
 		},
 	}
 
@@ -140,7 +136,6 @@ func TestInstance_ResetPlanStatus(t *testing.T) {
 
 	// Expected:
 	// - the status of the 'deploy' plan to be reset: all phases and steps should be PENDING, new UID should be assigned
-	// - AggregatedStatus should be PENDING too and 'deploy' should be the active plan
 	// - 'update' plan status should be unchanged
 	assert.Equal(t, InstanceStatus{
 		PlanStatus: map[string]PlanStatus{
@@ -157,10 +152,6 @@ func TestInstance_ResetPlanStatus(t *testing.T) {
 				Phases: []PhaseStatus{{Name: "phase", Status: ExecutionNeverRun, Steps: []StepStatus{{Status: ExecutionNeverRun, Name: "step"}}}},
 			},
 		},
-		AggregatedStatus: AggregatedStatus{
-			Status:         ExecutionPending,
-			ActivePlanName: "deploy",
-		},
 	}, instance.Status)
 }
 

diff --git a/pkg/apis/kudo/v1beta1/zz_generated.deepcopy.go b/pkg/apis/kudo/v1beta1/zz_generated.deepcopy.go
diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go
@@ -257,8 +257,8 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 	}
 
 	// Publish a PlanFinished event after instance and its status were successfully updated
-	if instance.Status.AggregatedStatus.Status.IsTerminal() {
-		r.Recorder.Event(instance, "Normal", "PlanFinished", fmt.Sprintf("Execution of plan %s finished with status %s", activePlanStatus.Name, instance.Status.AggregatedStatus.Status))
+	if instance.Spec.PlanExecution.Status.IsTerminal() {
+		r.Recorder.Event(instance, "Normal", "PlanFinished", fmt.Sprintf("Execution of plan %s finished with status %s", activePlanStatus.Name, instance.Spec.PlanExecution.Status))
 	}
 
 	return reconcile.Result{}, nil

diff --git a/pkg/engine/health/health.go b/pkg/engine/health/health.go
@@ -76,12 +76,15 @@ func IsHealthy(obj runtime.Object) error {
 
 		return fmt.Errorf("job %q still running or failed", obj.Name)
 	case *kudov1beta1.Instance:
-		log.Printf("HealthUtil: Instance %v is in state %v", obj.Name, obj.Status.AggregatedStatus.Status)
+		ps := obj.GetLastExecutedPlanStatus()
+		if ps == nil {
+			return fmt.Errorf("no plan has been executed for Instance %v", obj.Name)
+		}
 
-		if obj.Status.AggregatedStatus.Status.IsFinished() {
+		if ps.Status.IsFinished() {
 			return nil
 		}
-		return fmt.Errorf("instance's active plan is in state %v", obj.Status.AggregatedStatus.Status)
+		return fmt.Errorf("instance's active plan is in state %v", ps.Status)
 
 	case *corev1.Pod:
 		if obj.Status.Phase == corev1.PodRunning {

diff --git a/pkg/kudoctl/cmd/testdata/deploy-kudo-ns.yaml.golden b/pkg/kudoctl/cmd/testdata/deploy-kudo-ns.yaml.golden
@@ -504,16 +504,6 @@ spec:
         status:
           description: InstanceStatus defines the observed state of Instance
           properties:
-            aggregatedStatus:
-              description: AggregatedStatus is overview of an instance status derived
-                from the plan status
-              properties:
-                activePlanName:
-                  type: string
-                status:
-                  description: ExecutionStatus captures the state of the rollout.
-                  type: string
-              type: object
             planStatus:
               additionalProperties:
                 description: "PlanStatus is representing status of a plan \n These

diff --git a/pkg/kudoctl/cmd/testdata/deploy-kudo-sa.yaml.golden b/pkg/kudoctl/cmd/testdata/deploy-kudo-sa.yaml.golden
@@ -504,16 +504,6 @@ spec:
         status:
           description: InstanceStatus defines the observed state of Instance
           properties:
-            aggregatedStatus:
-              description: AggregatedStatus is overview of an instance status derived
-                from the plan status
-              properties:
-                activePlanName:
-                  type: string
-                status:
-                  description: ExecutionStatus captures the state of the rollout.
-                  type: string
-              type: object
             planStatus:
               additionalProperties:
                 description: "PlanStatus is representing status of a plan \n These

diff --git a/pkg/kudoctl/cmd/testdata/deploy-kudo-webhook.yaml.golden b/pkg/kudoctl/cmd/testdata/deploy-kudo-webhook.yaml.golden
@@ -504,16 +504,6 @@ spec:
         status:
           description: InstanceStatus defines the observed state of Instance
           properties:
-            aggregatedStatus:
-              description: AggregatedStatus is overview of an instance status derived
-                from the plan status
-              properties:
-                activePlanName:
-                  type: string
-                status:
-                  description: ExecutionStatus captures the state of the rollout.
-                  type: string
-              type: object
             planStatus:
               additionalProperties:
                 description: "PlanStatus is representing status of a plan \n These

diff --git a/pkg/kudoctl/cmd/testdata/deploy-kudo.yaml.golden b/pkg/kudoctl/cmd/testdata/deploy-kudo.yaml.golden
@@ -504,16 +504,6 @@ spec:
         status:
           description: InstanceStatus defines the observed state of Instance
           properties:
-            aggregatedStatus:
-              description: AggregatedStatus is overview of an instance status derived
-                from the plan status
-              properties:
-                activePlanName:
-                  type: string
-                status:
-                  description: ExecutionStatus captures the state of the rollout.
-                  type: string
-              type: object
             planStatus:
               additionalProperties:
                 description: "PlanStatus is representing status of a plan \n These

diff --git a/pkg/kudoctl/kudoinit/crd/bindata.go b/pkg/kudoctl/kudoinit/crd/bindata.go
diff --git a/test/e2e/cli-install-uninstall/01-assert.yaml b/test/e2e/cli-install-uninstall/01-assert.yaml
@@ -3,10 +3,11 @@ kind: Instance
 metadata:
   name: first-operator
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
 ---
 apiVersion: apps/v1
 kind: Deployment
 metadata:
-  name: nginx-deployment
+  name: nginx-deployment
diff --git a/test/e2e/terminal-failed-job/01-assert.yaml b/test/e2e/terminal-failed-job/01-assert.yaml
@@ -7,5 +7,6 @@ spec:
   operatorVersion:
     name: failjob-operator-0.1.0
 status:
-  aggregatedStatus:
-    status: FATAL_ERROR
+  planStatus:
+    deploy:
+      status: FATAL_ERROR
diff --git a/test/e2e/terminal-failed-job/02-assert.yaml b/test/e2e/terminal-failed-job/02-assert.yaml
@@ -7,5 +7,6 @@ spec:
   operatorVersion:
     name: job-timeout-operator-0.1.0
 status:
-  aggregatedStatus:
-    status: FATAL_ERROR
+  planStatus:
+    deploy:
+      status: FATAL_ERROR
diff --git a/test/e2e/toggle-task/01-assert.yaml b/test/e2e/toggle-task/01-assert.yaml
@@ -7,12 +7,13 @@ spec:
   operatorVersion:
     name: feature-operator-0.1.0
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
 ---
 apiVersion: v1
 kind: ConfigMap
 metadata:
   name: feature-operator-cm
 data:
-  foo: "1234"
+  foo: "1234"
diff --git a/test/e2e/toggle-task/02-assert.yaml b/test/e2e/toggle-task/02-assert.yaml
@@ -7,5 +7,6 @@ spec:
   operatorVersion:
     name: feature-operator-0.1.0
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
diff --git a/test/integration/cli-install/00-assert.yaml b/test/integration/cli-install/00-assert.yaml
@@ -7,8 +7,9 @@ spec:
   operatorVersion:
     name: cli-install-operator-0.1.0
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
 ---
 apiVersion: v1
 kind: Service

diff --git a/test/integration/cli-install/01-assert.yaml b/test/integration/cli-install/01-assert.yaml
@@ -8,8 +8,9 @@ spec:
   operatorVersion:
     name: cli-install-operator-0.1.0
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
 ---
 apiVersion: v1
 kind: Service

diff --git a/test/integration/create-operator-in-operator/01-assert.yaml b/test/integration/create-operator-in-operator/01-assert.yaml
@@ -3,16 +3,18 @@ kind: Instance
 metadata:
   name: oio-instance
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
 ---
 apiVersion: kudo.dev/v1beta1
 kind: Instance
 metadata:
   name: inner-instance
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
 ---
 apiVersion: v1
 kind: ConfigMap

diff --git a/test/integration/immutable-client-ip/00-assert.yaml b/test/integration/immutable-client-ip/00-assert.yaml
@@ -3,8 +3,9 @@ kind: Instance
 metadata:
   name: immutable-client-ip-instance
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
 ---
 apiVersion: v1
 kind: Service

diff --git a/test/integration/immutable-client-ip/01-assert.yaml b/test/integration/immutable-client-ip/01-assert.yaml
@@ -3,8 +3,9 @@ kind: Instance
 metadata:
   name: immutable-client-ip-instance
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
 ---
 apiVersion: v1
 kind: Service

diff --git a/test/integration/install-existing-crds-in-operator/00-assert.yaml b/test/integration/install-existing-crds-in-operator/00-assert.yaml
@@ -3,5 +3,6 @@ kind: Instance
 metadata:
   name: crd-instance
 status:
-  aggregatedStatus:
-    status: COMPLETE
+  planStatus:
+    deploy:
+      status: COMPLETE
diff --git a/test/integration/instance-controller-param-change-custom-trigger/00-assert.yaml b/test/integration/instance-controller-param-change-custom-trigger/00-assert.yaml
@@ -3,8 +3,6 @@ kind: Instance
 metadata:
   name: icto-custom-trigger
 status:
-  aggregatedStatus:
-    status: COMPLETE
   planStatus:
     deploy:
       status: COMPLETE
diff --git a/test/integration/instance-controller-param-change-custom-trigger/01-assert.yaml b/test/integration/instance-controller-param-change-custom-trigger/01-assert.yaml
@@ -3,8 +3,6 @@ kind: Instance
 metadata:
   name: icto-custom-trigger
 status:
-  aggregatedStatus:
-    status: COMPLETE
   planStatus:
     foo-changed:
       status: COMPLETE
diff --git a/test/integration/instance-controller-param-change-fallback-to-deploy/00-assert.yaml b/test/integration/instance-controller-param-change-fallback-to-deploy/00-assert.yaml
@@ -3,8 +3,6 @@ kind: Instance
 metadata:
   name: icto-fallback-to-deploy
 status:
-  aggregatedStatus:
-    status: COMPLETE
   planStatus:
     deploy:
       status: COMPLETE
diff --git a/test/integration/instance-controller-param-change-fallback-to-deploy/01-assert.yaml b/test/integration/instance-controller-param-change-fallback-to-deploy/01-assert.yaml
@@ -3,8 +3,6 @@ kind: Instance
 metadata:
   name: icto-fallback-to-deploy
 status:
-  aggregatedStatus:
-    status: COMPLETE
   planStatus:
     deploy:
       status: COMPLETE
diff --git a/test/integration/instance-controller-param-change-no-trigger/00-assert.yaml b/test/integration/instance-controller-param-change-no-trigger/00-assert.yaml
@@ -3,8 +3,6 @@ kind: Instance
 metadata:
   name: icto-no-trigger
 status:
-  aggregatedStatus:
-    status: COMPLETE
   planStatus:
     deploy:
       status: COMPLETE