Simplify instance controller reconciliation (#1551)

Summary: current instance controller reconciliation logic with regard to detecting currently active plan can be much simpler now that we have IAW (instance admission webhook) controlling the scheduled plan.
kudobuilder · Jun 8, 2020 · e228e0d · e228e0d
1 parent 5a54251
commit e228e0d
Show file tree

Hide file tree

Showing 5 changed files with 313 additions and 362 deletions.
diff --git a/pkg/apis/kudo/v1beta1/instance_types_helpers.go b/pkg/apis/kudo/v1beta1/instance_types_helpers.go
@@ -2,20 +2,17 @@ package v1beta1
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"log"
 
 	"github.com/thoas/go-funk"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/uuid"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
 	instanceCleanupFinalizerName = "kudo.dev.instance.cleanup"
-	snapshotAnnotation           = "kudo.dev/last-applied-instance-state"
 )
 
 // ScheduledPlan returns plan status of currently active plan or nil if no plan is running. In most cases this method
@@ -78,44 +75,31 @@ func (i *Instance) NoPlanEverExecuted() bool {
 }
 
 // UpdateInstanceStatus updates `Status.PlanStatus` and `Status.AggregatedStatus` property based on the given plan
-func (i *Instance) UpdateInstanceStatus(planStatus *PlanStatus, updatedTimestamp *metav1.Time) {
+func (i *Instance) UpdateInstanceStatus(ps *PlanStatus, updatedTimestamp *metav1.Time) {
 	for k, v := range i.Status.PlanStatus {
-		if v.Name == planStatus.Name {
-			planStatus.LastUpdatedTimestamp = updatedTimestamp
-			i.Status.PlanStatus[k] = *planStatus
-			i.Spec.PlanExecution.Status = planStatus.Status
+		if v.Name == ps.Name {
+			ps.LastUpdatedTimestamp = updatedTimestamp
+			i.Status.PlanStatus[k] = *ps
+			i.Spec.PlanExecution.Status = ps.Status
 		}
 	}
 }
 
 // ResetPlanStatus method resets a PlanStatus for a passed plan name and instance. Plan/phase/step statuses
 // are set to ExecutionPending meaning that the controller will restart plan execution.
-func (i *Instance) ResetPlanStatus(plan string, updatedTimestamp *metav1.Time) error {
-	planStatus := i.PlanStatus(plan)
-	if planStatus == nil {
-		return fmt.Errorf("failed to find planStatus for the plan '%s'", plan)
-	}
-
-	// reset plan's phases and steps by setting them to ExecutionPending
-	planStatus.Set(ExecutionPending)
-	// when using webhooks, instance admission webhook already generates an UID for current plan, otherwise, we generate a new one.
-	if i.Spec.PlanExecution.UID != "" {
-		planStatus.UID = i.Spec.PlanExecution.UID
-	} else {
-		planStatus.UID = uuid.NewUUID()
-	}
-
-	for i, ph := range planStatus.Phases {
-		planStatus.Phases[i].Set(ExecutionPending)
-
-		for j := range ph.Steps {
-			planStatus.Phases[i].Steps[j].Set(ExecutionPending)
+func (i *Instance) ResetPlanStatus(ps *PlanStatus, uid types.UID, updatedTimestamp *metav1.Time) {
+	ps.UID = uid
+	ps.Status = ExecutionPending
+	for i := range ps.Phases {
+		ps.Phases[i].Set(ExecutionPending)
+
+		for j := range ps.Phases[i].Steps {
+			ps.Phases[i].Steps[j].Set(ExecutionPending)
 		}
 	}
 
 	// update plan status and instance aggregated status
-	i.UpdateInstanceStatus(planStatus, updatedTimestamp)
-	return nil
+	i.UpdateInstanceStatus(ps, updatedTimestamp)
 }
 
 // IsDeleting returns true is the instance is being deleted.
@@ -145,35 +129,6 @@ func (i *Instance) PlanStatus(plan string) *PlanStatus {
 	return nil
 }
 
-// annotateSnapshot stores the current spec of Instance into the snapshot annotation
-// this information is used when executing update/upgrade plans, this overrides any snapshot that existed before
-func (i *Instance) AnnotateSnapshot() error {
-	jsonBytes, err := json.Marshal(i.Spec)
-	if err != nil {
-		return err
-	}
-	if i.Annotations == nil {
-		i.Annotations = make(map[string]string)
-	}
-	i.Annotations[snapshotAnnotation] = string(jsonBytes)
-	return nil
-}
-
-func (i *Instance) SnapshotSpec() (*InstanceSpec, error) {
-	if i.Annotations != nil {
-		snapshot, ok := i.Annotations[snapshotAnnotation]
-		if ok {
-			var spec *InstanceSpec
-			err := json.Unmarshal([]byte(snapshot), &spec)
-			if err != nil {
-				return nil, err
-			}
-			return spec, nil
-		}
-	}
-	return nil, nil
-}
-
 func (i *Instance) HasCleanupFinalizer() bool {
 	return funk.ContainsString(i.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
 }

diff --git a/pkg/apis/kudo/v1beta1/instance_types_helpers_test.go b/pkg/apis/kudo/v1beta1/instance_types_helpers_test.go
@@ -20,14 +20,11 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
-	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/uuid"
 )
 
 var (
 	testTime = time.Date(2019, 10, 17, 1, 1, 1, 1, time.UTC)
-	testUUID = uuid.NewUUID()
 )
 
 func TestGetLastExecutedPlanStatus(t *testing.T) {
@@ -81,80 +78,6 @@ func TestGetLastExecutedPlanStatus(t *testing.T) {
 	}
 }
 
-func TestInstance_ResetPlanStatus(t *testing.T) {
-	// a test instance with 'deploy' plan IN_PROGRESS and 'update' that NEVER_RUN
-	instance := &Instance{
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: "kudo.dev/v1beta1",
-			Kind:       "Instance",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "test",
-			Namespace: "test",
-		},
-		Spec: InstanceSpec{
-			OperatorVersion: v1.ObjectReference{
-				Name: "foo-operator",
-			},
-			Parameters: map[string]string{
-				"foo": "foo",
-			},
-		},
-		Status: InstanceStatus{
-			PlanStatus: map[string]PlanStatus{
-				"deploy": {
-					Status:               ExecutionInProgress,
-					Name:                 "deploy",
-					LastUpdatedTimestamp: &metav1.Time{Time: testTime},
-					UID:                  testUUID,
-					Phases:               []PhaseStatus{{Name: "phase", Status: ExecutionInProgress, Steps: []StepStatus{{Status: ExecutionInProgress, Name: "step"}}}},
-				},
-				"update": {
-					Status: ExecutionNeverRun,
-					Name:   "update",
-					Phases: []PhaseStatus{{Name: "phase", Status: ExecutionNeverRun, Steps: []StepStatus{{Status: ExecutionNeverRun, Name: "step"}}}},
-				},
-			},
-		},
-	}
-
-	oldUID := instance.Status.PlanStatus["deploy"].UID
-
-	err := instance.ResetPlanStatus("deploy", &metav1.Time{Time: time.Now()})
-	assert.NoError(t, err)
-
-	// we test that UID has changed. afterwards, we replace it with the old one and compare new
-	// plan status with the desired state
-	assert.NotEqual(t, instance.Status.PlanStatus["deploy"].UID, oldUID)
-	assert.Equal(t, instance.Spec.PlanExecution.Status, ExecutionPending)
-
-	oldPlanStatus := instance.Status.PlanStatus["deploy"]
-	statusCopy := oldPlanStatus.DeepCopy()
-	statusCopy.UID = testUUID
-	statusCopy.LastUpdatedTimestamp = &metav1.Time{Time: testTime}
-	instance.Status.PlanStatus["deploy"] = *statusCopy
-
-	// Expected:
-	// - the status of the 'deploy' plan to be reset: all phases and steps should be PENDING, new UID should be assigned
-	// - 'update' plan status should be unchanged
-	assert.Equal(t, InstanceStatus{
-		PlanStatus: map[string]PlanStatus{
-			"deploy": {
-				Status:               ExecutionPending,
-				Name:                 "deploy",
-				LastUpdatedTimestamp: &metav1.Time{Time: testTime},
-				UID:                  testUUID,
-				Phases:               []PhaseStatus{{Name: "phase", Status: ExecutionPending, Steps: []StepStatus{{Status: ExecutionPending, Name: "step"}}}},
-			},
-			"update": {
-				Status: ExecutionNeverRun,
-				Name:   "update",
-				Phases: []PhaseStatus{{Name: "phase", Status: ExecutionNeverRun, Steps: []StepStatus{{Status: ExecutionNeverRun, Name: "step"}}}},
-			},
-		},
-	}, instance.Status)
-}
-
 func TestGetParamDefinitions(t *testing.T) {
 	ov := &OperatorVersion{
 		ObjectMeta: metav1.ObjectMeta{Name: "foo-operator", Namespace: "default"},