Initialise plan status correctly if an upgraded plan adds new phases or steps (#1755)

* Add e2e-test to confirm missing phase status bug
* Initialize status correctly for newly added phases and steps

Signed-off-by: Andreas Neumann <[email protected]>

Author: ANeumann82

pkg/apis/kudo/v1beta1/instance_types.go

@@ -116,6 +116,16 @@ func (s *StepStatus) SetWithMessage(status ExecutionStatus, message string) {
 	s.Message = message
 }
 
+// Step returns the StepStatus with the given name, or nil if no such step status exists
+func (s *PhaseStatus) Step(name string) *StepStatus {
+	for _, stepStatus := range s.Steps {
+		if stepStatus.Name == name {
+			return &stepStatus
+		}
+	}
+	return nil
+}
+
 func (s *PhaseStatus) Set(status ExecutionStatus) {
 	s.Status = status
 	s.Message = ""
@@ -126,6 +136,16 @@ func (s *PhaseStatus) SetWithMessage(status ExecutionStatus, message string) {
 	s.Message = message
 }
 
+// Step returns the PhaseStatus with the given name, or nil if no such phase status exists
+func (s *PlanStatus) Phase(name string) *PhaseStatus {
+	for _, phaseStatus := range s.Phases {
+		if phaseStatus.Name == name {
+			return &phaseStatus
+		}
+	}
+	return nil
+}
+
 func (s *PlanStatus) Set(status ExecutionStatus) {
 	if s.Status != status {
 		s.LastUpdatedTimestamp = &metav1.Time{Time: time.Now()}

pkg/controller/instance/instance_controller.go

@@ -532,29 +532,36 @@ func ensurePlanStatusInitialized(i *kudoapi.Instance, ov *kudoapi.OperatorVersio
 	}
 
 	for planName, plan := range ov.Spec.Plans {
-		if _, ok := i.Status.PlanStatus[planName]; !ok {
-			planStatus := kudoapi.PlanStatus{
+		planStatus, ok := i.Status.PlanStatus[planName]
+		if !ok {
+			planStatus = kudoapi.PlanStatus{
 				Name:   planName,
 				Status: kudoapi.ExecutionNeverRun,
 				Phases: make([]kudoapi.PhaseStatus, 0),
 			}
-			for _, phase := range plan.Phases {
-				phaseStatus := kudoapi.PhaseStatus{
+		}
+		for _, phase := range plan.Phases {
+			phaseStatus := planStatus.Phase(phase.Name)
+			if phaseStatus == nil {
+				planStatus.Phases = append(planStatus.Phases, kudoapi.PhaseStatus{
 					Name:   phase.Name,
 					Status: kudoapi.ExecutionNeverRun,
 					Steps:  make([]kudoapi.StepStatus, 0),
-				}
-				for _, step := range phase.Steps {
-					stepStatus := kudoapi.StepStatus{
+				})
+				phaseStatus = &planStatus.Phases[len(planStatus.Phases)-1]
+			}
+			for _, step := range phase.Steps {
+				stepStatus := phaseStatus.Step(step.Name)
+				if stepStatus == nil {
+					phaseStatus.Steps = append(phaseStatus.Steps, kudoapi.StepStatus{
 						Name:   step.Name,
 						Status: kudoapi.ExecutionNeverRun,
-					}
-					phaseStatus.Steps = append(phaseStatus.Steps, stepStatus)
+					})
 				}
-				planStatus.Phases = append(planStatus.Phases, phaseStatus)
 			}
-			i.Status.PlanStatus[planName] = planStatus
 		}
+		// We might have updated the plan status, so set it just to make sure
+		i.Status.PlanStatus[planName] = planStatus
 	}
 }
 

pkg/controller/instance/instance_controller_test.go

@@ -575,6 +575,57 @@ func Test_ensurePlanStatusInitialized(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "a new phase is correctly initialized",
+			i:    instance,
+			ov: func() *kudoapi.OperatorVersion {
+				modifiedOv := ov.DeepCopy()
+				pln := modifiedOv.Spec.Plans["deploy"]
+				pln.Phases = append(pln.Phases, kudoapi.Phase{
+					Name: "newphase",
+					Steps: []kudoapi.Step{
+						{
+							Name:  "additionalstep",
+							Tasks: []string{},
+						},
+					},
+				})
+				modifiedOv.Spec.Plans["deploy"] = pln
+				return modifiedOv
+			}(),
+			want: kudoapi.InstanceStatus{
+				PlanStatus: map[string]kudoapi.PlanStatus{
+					"deploy": {
+						Name:   "deploy",
+						Status: kudoapi.ExecutionNeverRun,
+						UID:    "",
+						Phases: []kudoapi.PhaseStatus{
+							{
+								Name:   "phase",
+								Status: kudoapi.ExecutionNeverRun,
+								Steps: []kudoapi.StepStatus{
+									{
+										Name:   "step",
+										Status: kudoapi.ExecutionNeverRun,
+									},
+								},
+							},
+							{
+								Name:   "newphase",
+								Status: kudoapi.ExecutionNeverRun,
+								Steps: []kudoapi.StepStatus{
+									{
+										Name:   "additionalstep",
+										Status: kudoapi.ExecutionNeverRun,
+									},
+								},
+							},
+						},
+					},
+					"backup": backupStatus,
+				},
+			},
+		},
 	}
 
 	for _, tt := range tests {

test/e2e/upgrade/00-assert.yaml

@@ -0,0 +1,16 @@
+apiVersion: kudo.dev/v1beta1
+kind: Instance
+metadata:
+  name: upgrade-operator
+status:
+  conditions:
+    - type: Ready
+      status: "True"
+  planStatus:
+    deploy:
+      status: COMPLETE
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment

test/e2e/upgrade/00-install.yaml

@@ -0,0 +1,5 @@
+apiVersion: kudo.dev/v1beta1
+kind: TestStep
+commands:
+  - command: kubectl kudo install --instance upgrade-operator ./first-operator
+    namespaced: true

test/e2e/upgrade/01-assert.yaml

@@ -0,0 +1,18 @@
+apiVersion: kudo.dev/v1beta1
+kind: Instance
+metadata:
+  name: upgrade-operator
+status:
+  planStatus:
+    deploy:
+      status: COMPLETE
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test

test/e2e/upgrade/01-upgrade.yaml

@@ -0,0 +1,5 @@
+apiVersion: kudo.dev/v1beta1
+kind: TestStep
+commands:
+  - command: kubectl kudo upgrade --instance upgrade-operator ./first-operator-v2
+    namespaced: true

test/e2e/upgrade/first-operator-v2/operator.yaml

@@ -0,0 +1,36 @@
+apiVersion: kudo.dev/v1beta1
+name: "first-operator"
+operatorVersion: "0.2.0"
+appVersion: "1.7.9"
+kubernetesVersion: 1.17.0
+maintainers:
+  - name: Your name
+    email: <[email protected]>
+url: https://kudo.dev
+tasks:
+  - name: app
+    kind: Apply
+    spec:
+      resources:
+        - deployment.yaml
+  - name: config
+    kind: Apply
+    spec:
+      resources:
+        - configmap.yaml
+plans:
+  deploy:
+    strategy: serial
+    phases:
+      - name: newphase
+        strategy: parallel
+        steps:
+          - name: config
+            tasks:
+              - config
+      - name: main
+        strategy: parallel
+        steps:
+          - name: everything
+            tasks:
+              - app

test/e2e/upgrade/first-operator-v2/params.yaml

@@ -0,0 +1,5 @@
+apiVersion: kudo.dev/v1beta1
+parameters:
+  - name: replicas
+    description: Number of replicas that should be run as part of the deployment
+    default: 2

test/e2e/upgrade/first-operator-v2/templates/configmap.yaml

@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  foo: bar