From 6ec828d02f7e3372a9fd8e9420765d5667ed7dc5 Mon Sep 17 00:00:00 2001 From: Andreas Neumann Date: Tue, 19 Jan 2021 09:08:10 +0100 Subject: [PATCH] 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 --- pkg/apis/kudo/v1beta1/instance_types.go | 20 ++++++++ .../instance/instance_controller.go | 29 +++++++---- .../instance/instance_controller_test.go | 51 +++++++++++++++++++ test/e2e/upgrade/00-assert.yaml | 16 ++++++ test/e2e/upgrade/00-install.yaml | 5 ++ test/e2e/upgrade/01-assert.yaml | 18 +++++++ test/e2e/upgrade/01-upgrade.yaml | 5 ++ .../upgrade/first-operator-v2/operator.yaml | 36 +++++++++++++ .../e2e/upgrade/first-operator-v2/params.yaml | 5 ++ .../templates/configmap.yaml | 6 +++ .../templates/deployment.yaml | 19 +++++++ test/e2e/upgrade/first-operator/operator.yaml | 25 +++++++++ test/e2e/upgrade/first-operator/params.yaml | 5 ++ .../first-operator/templates/deployment.yaml | 19 +++++++ 14 files changed, 248 insertions(+), 11 deletions(-) create mode 100644 test/e2e/upgrade/00-assert.yaml create mode 100644 test/e2e/upgrade/00-install.yaml create mode 100644 test/e2e/upgrade/01-assert.yaml create mode 100644 test/e2e/upgrade/01-upgrade.yaml create mode 100644 test/e2e/upgrade/first-operator-v2/operator.yaml create mode 100644 test/e2e/upgrade/first-operator-v2/params.yaml create mode 100644 test/e2e/upgrade/first-operator-v2/templates/configmap.yaml create mode 100644 test/e2e/upgrade/first-operator-v2/templates/deployment.yaml create mode 100644 test/e2e/upgrade/first-operator/operator.yaml create mode 100644 test/e2e/upgrade/first-operator/params.yaml create mode 100644 test/e2e/upgrade/first-operator/templates/deployment.yaml diff --git a/pkg/apis/kudo/v1beta1/instance_types.go b/pkg/apis/kudo/v1beta1/instance_types.go index 00787142c..5f2da7870 100644 --- a/pkg/apis/kudo/v1beta1/instance_types.go +++ b/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()} diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go index 9bbfa6be0..d9cbe9754 100644 --- a/pkg/controller/instance/instance_controller.go +++ b/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 } } diff --git a/pkg/controller/instance/instance_controller_test.go b/pkg/controller/instance/instance_controller_test.go index 01a4485bd..c694e5f07 100644 --- a/pkg/controller/instance/instance_controller_test.go +++ b/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 { diff --git a/test/e2e/upgrade/00-assert.yaml b/test/e2e/upgrade/00-assert.yaml new file mode 100644 index 000000000..31f3e7b93 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/test/e2e/upgrade/00-install.yaml b/test/e2e/upgrade/00-install.yaml new file mode 100644 index 000000000..5880ac0df --- /dev/null +++ b/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 diff --git a/test/e2e/upgrade/01-assert.yaml b/test/e2e/upgrade/01-assert.yaml new file mode 100644 index 000000000..e040cecd4 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/test/e2e/upgrade/01-upgrade.yaml b/test/e2e/upgrade/01-upgrade.yaml new file mode 100644 index 000000000..1d48619d6 --- /dev/null +++ b/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 diff --git a/test/e2e/upgrade/first-operator-v2/operator.yaml b/test/e2e/upgrade/first-operator-v2/operator.yaml new file mode 100644 index 000000000..eda855422 --- /dev/null +++ b/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: +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 diff --git a/test/e2e/upgrade/first-operator-v2/params.yaml b/test/e2e/upgrade/first-operator-v2/params.yaml new file mode 100644 index 000000000..89c6d7726 --- /dev/null +++ b/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 diff --git a/test/e2e/upgrade/first-operator-v2/templates/configmap.yaml b/test/e2e/upgrade/first-operator-v2/templates/configmap.yaml new file mode 100644 index 000000000..27da78b38 --- /dev/null +++ b/test/e2e/upgrade/first-operator-v2/templates/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test +data: + foo: bar \ No newline at end of file diff --git a/test/e2e/upgrade/first-operator-v2/templates/deployment.yaml b/test/e2e/upgrade/first-operator-v2/templates/deployment.yaml new file mode 100644 index 000000000..f5450122d --- /dev/null +++ b/test/e2e/upgrade/first-operator-v2/templates/deployment.yaml @@ -0,0 +1,19 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + selector: + matchLabels: + app: nginx + replicas: {{ .Params.replicas }} # tells deployment to run 2 pods matching the template + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx + image: nginx:{{ .AppVersion }} + ports: + - containerPort: 80 \ No newline at end of file diff --git a/test/e2e/upgrade/first-operator/operator.yaml b/test/e2e/upgrade/first-operator/operator.yaml new file mode 100644 index 000000000..c87178b0a --- /dev/null +++ b/test/e2e/upgrade/first-operator/operator.yaml @@ -0,0 +1,25 @@ +apiVersion: kudo.dev/v1beta1 +name: "first-operator" +operatorVersion: "0.1.0" +appVersion: "1.7.9" +kubernetesVersion: 1.17.0 +maintainers: + - name: Your name + email: +url: https://kudo.dev +tasks: + - name: app + kind: Apply + spec: + resources: + - deployment.yaml +plans: + deploy: + strategy: serial + phases: + - name: main + strategy: parallel + steps: + - name: everything + tasks: + - app diff --git a/test/e2e/upgrade/first-operator/params.yaml b/test/e2e/upgrade/first-operator/params.yaml new file mode 100644 index 000000000..89c6d7726 --- /dev/null +++ b/test/e2e/upgrade/first-operator/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 diff --git a/test/e2e/upgrade/first-operator/templates/deployment.yaml b/test/e2e/upgrade/first-operator/templates/deployment.yaml new file mode 100644 index 000000000..f5450122d --- /dev/null +++ b/test/e2e/upgrade/first-operator/templates/deployment.yaml @@ -0,0 +1,19 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + selector: + matchLabels: + app: nginx + replicas: {{ .Params.replicas }} # tells deployment to run 2 pods matching the template + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx + image: nginx:{{ .AppVersion }} + ports: + - containerPort: 80 \ No newline at end of file