Plan status should show last executed plan, not just active plan (#944)

pkg/apis/kudo/v1alpha1/instance_types.go

@@ -148,6 +148,25 @@ func (i *Instance) GetPlanInProgress() *PlanStatus {
 	return nil
 }
 
+// GetLastExecutedPlanStatus returns status of plan that is currently running, if there is one running
+// if no plan is running it looks for last executed plan based on timestamps
+func (i *Instance) GetLastExecutedPlanStatus() *PlanStatus {
+	if i.NoPlanEverExecuted() {
+		return nil
+	}
+	activePlan := i.GetPlanInProgress()
+	if activePlan != nil {
+		return activePlan
+	}
+	var lastExecutedPlan *PlanStatus
+	for _, p := range i.Status.PlanStatus {
+		if p.Status != ExecutionNeverRun && (lastExecutedPlan == nil || p.LastFinishedRun.Before(&lastExecutedPlan.LastFinishedRun)) {
+			lastExecutedPlan = &p
+		}
+	}
+	return lastExecutedPlan
+}
+
 // NoPlanEverExecuted returns true is this is new instance for which we never executed any plan
 func (i *Instance) NoPlanEverExecuted() bool {
 	for _, p := range i.Status.PlanStatus {

pkg/apis/kudo/v1alpha1/instance_types_test.go

@@ -0,0 +1,74 @@
+/*
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1
+
+import (
+	"testing"
+	"time"
+
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestGetLastExecutedPlanStatus(t *testing.T) {
+	tests := []struct {
+		name             string
+		planStatus       map[string]PlanStatus
+		expectedPlanName string
+	}{
+		{"no plan ever run", map[string]PlanStatus{"test": {
+			Status: ExecutionNeverRun,
+			Name:   "test",
+			Phases: []PhaseStatus{{Name: "phase", Status: ExecutionNeverRun, Steps: []StepStatus{{Status: ExecutionNeverRun, Name: "step"}}}},
+		}}, ""},
+		{"plan in progress", map[string]PlanStatus{
+			"test": {
+				Status: ExecutionInProgress,
+				Name:   "test",
+				Phases: []PhaseStatus{{Name: "phase", Status: ExecutionInProgress, Steps: []StepStatus{{Status: ExecutionInProgress, Name: "step"}}}},
+			},
+			"test2": {
+				Status: ExecutionComplete,
+				Name:   "test2",
+				Phases: []PhaseStatus{{Name: "phase", Status: ExecutionComplete, Steps: []StepStatus{{Status: ExecutionComplete, Name: "step"}}}},
+			}}, "test"},
+		{"last executed plan", map[string]PlanStatus{
+			"test": {
+				Status:          ExecutionComplete,
+				Name:            "test",
+				LastFinishedRun: v1.Time{Time: time.Now()},
+				Phases:          []PhaseStatus{{Name: "phase", Status: ExecutionComplete, Steps: []StepStatus{{Status: ExecutionComplete, Name: "step"}}}},
+			},
+			"test2": {
+				Status:          ExecutionComplete,
+				Name:            "test2",
+				LastFinishedRun: v1.Time{Time: time.Now().Add(time.Hour)},
+				Phases:          []PhaseStatus{{Name: "phase", Status: ExecutionComplete, Steps: []StepStatus{{Status: ExecutionComplete, Name: "step"}}}},
+			}}, "test2"},
+	}
+
+	for _, tt := range tests {
+		i := Instance{}
+		i.Status.PlanStatus = tt.planStatus
+		actual := i.GetLastExecutedPlanStatus()
+		actualName := ""
+		if actual != nil {
+			actualName = actual.Name
+		}
+		if actualName != tt.expectedPlanName {
+			t.Errorf("%s: Expected to get plan %s but got plan status of %v", tt.name, tt.expectedPlanName, actual)
+		}
+	}
+}

pkg/controller/instance/instance_controller.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"log"
 	"strings"
+	"time"
 
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/source"
@@ -166,7 +167,7 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 		return reconcile.Result{}, err
 	}
 	log.Printf("InstanceController: Going to proceed in execution of active plan %s on instance %s/%s", activePlan.Name, instance.Namespace, instance.Name)
-	newStatus, err := executePlan(activePlan, metadata, r.Client, &kustomizeEnhancer{r.Scheme})
+	newStatus, err := executePlan(activePlan, metadata, r.Client, &kustomizeEnhancer{r.Scheme}, time.Now())
 
 	// ---------- 4. Update status of instance after the execution proceeded ----------
 

pkg/controller/instance/plan_execution_engine.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"log"
 	"strconv"
+	"time"
 
 	"k8s.io/apimachinery/pkg/types"
 
@@ -18,6 +19,7 @@ import (
 	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	apijson "k8s.io/apimachinery/pkg/util/json"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -58,7 +60,7 @@ type executionMetadata struct {
 // result of running this function is new state of the execution that is returned to the caller (it can either be completed, or still in progress or errored)
 // in case of error, error is returned along with the state as well (so that it's possible to report which step caused the error)
 // in case of error, method returns ErrorStatus which has property to indicate unrecoverable error meaning if there is no point in retrying that execution
-func executePlan(plan *activePlan, metadata *executionMetadata, c client.Client, renderer kubernetesObjectEnhancer) (*v1alpha1.PlanStatus, error) {
+func executePlan(plan *activePlan, metadata *executionMetadata, c client.Client, renderer kubernetesObjectEnhancer, currentTime time.Time) (*v1alpha1.PlanStatus, error) {
 	if plan.Status.IsTerminal() {
 		log.Printf("PlanExecution: Plan %s for instance %s is terminal, nothing to do", plan.Name, metadata.instanceName)
 		return plan.PlanStatus, nil
@@ -131,6 +133,7 @@ func executePlan(plan *activePlan, metadata *executionMetadata, c client.Client,
 	if allPhasesCompleted {
 		log.Printf("PlanExecution: All phases on plan %s and instance %s are healthy", plan.Name, metadata.instanceName)
 		newState.Status = v1alpha1.ExecutionComplete
+		newState.LastFinishedRun = v1.Time{Time: currentTime}
 	}
 
 	return newState, nil

pkg/controller/instance/plan_execution_engine_test.go

@@ -3,6 +3,7 @@ package instance
 import (
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/kudobuilder/kudo/pkg/util/template"
 	"github.com/pkg/errors"
@@ -19,6 +20,7 @@ import (
 )
 
 func TestExecutePlan(t *testing.T) {
+	timeNow := time.Now()
 	defaultMetadata := &executionMetadata{
 		instanceName:        "Instance",
 		planExecutionID:     "pid",
@@ -79,9 +81,10 @@ func TestExecutePlan(t *testing.T) {
 			Tasks:     map[string]v1alpha1.TaskSpec{"task": {Resources: []string{"pod"}}},
 			Templates: map[string]string{"pod": getResourceAsString(getPod("pod1", "default"))},
 		}, defaultMetadata, &v1alpha1.PlanStatus{
-			Status: v1alpha1.ExecutionComplete,
-			Name:   "test",
-			Phases: []v1alpha1.PhaseStatus{{Name: "phase", Status: v1alpha1.ExecutionComplete, Steps: []v1alpha1.StepStatus{{Status: v1alpha1.ExecutionComplete, Name: "step"}}}},
+			Status:          v1alpha1.ExecutionComplete,
+			Name:            "test",
+			LastFinishedRun: v1.Time{Time: timeNow},
+			Phases:          []v1alpha1.PhaseStatus{{Name: "phase", Status: v1alpha1.ExecutionComplete, Steps: []v1alpha1.StepStatus{{Status: v1alpha1.ExecutionComplete, Name: "step"}}}},
 		}},
 		{"plan in errored state will be retried and completed when no error happens", &activePlan{
 			Name: "test",
@@ -99,15 +102,16 @@ func TestExecutePlan(t *testing.T) {
 			Tasks:     map[string]v1alpha1.TaskSpec{"task": {Resources: []string{"pod"}}},
 			Templates: map[string]string{"pod": getResourceAsString(getPod("pod1", "default"))},
 		}, defaultMetadata, &v1alpha1.PlanStatus{
-			Status: v1alpha1.ExecutionComplete,
-			Name:   "test",
-			Phases: []v1alpha1.PhaseStatus{{Name: "phase", Status: v1alpha1.ExecutionComplete, Steps: []v1alpha1.StepStatus{{Status: v1alpha1.ExecutionComplete, Name: "step"}}}},
+			Status:          v1alpha1.ExecutionComplete,
+			LastFinishedRun: v1.Time{Time: timeNow},
+			Name:            "test",
+			Phases:          []v1alpha1.PhaseStatus{{Name: "phase", Status: v1alpha1.ExecutionComplete, Steps: []v1alpha1.StepStatus{{Status: v1alpha1.ExecutionComplete, Name: "step"}}}},
 		}},
 	}
 
 	for _, tt := range tests {
 		testClient := fake.NewFakeClientWithScheme(scheme.Scheme)
-		newStatus, err := executePlan(tt.activePlan, tt.metadata, testClient, &testKubernetesObjectEnhancer{})
+		newStatus, err := executePlan(tt.activePlan, tt.metadata, testClient, &testKubernetesObjectEnhancer{}, timeNow)
 
 		if err != nil {
 			t.Errorf("%s: Expecting no error but got error %v", tt.name, err)

pkg/kudoctl/cmd/plan/plan_status.go

@@ -97,21 +97,21 @@ func planStatus(options *Options, settings *env.Settings) error {
 		return err
 	}
 
-	activePlanStatus := instance.GetPlanInProgress()
+	lastPlanStatus := instance.GetLastExecutedPlanStatus()
 
-	if activePlanStatus == nil {
-		log.Printf("No active plan exists for instance %s", instance.Name)
+	if lastPlanStatus == nil {
+		log.Printf("No plan ever run for instance - nothing to show for instance %s\n", instance.Name)
 		return nil
 	}
 
-	rootDisplay := fmt.Sprintf("%s (Operator-Version: \"%s\" Active-Plan: \"%s\")", instance.Name, instance.Spec.OperatorVersion.Name, activePlanStatus.Name)
+	rootDisplay := fmt.Sprintf("%s (Operator-Version: \"%s\" Active-Plan: \"%s\")", instance.Name, instance.Spec.OperatorVersion.Name, lastPlanStatus.Name)
 	rootBranchName := tree.AddBranch(rootDisplay)
 
 	for name, plan := range operator.Spec.Plans {
-		if name == activePlanStatus.Name {
-			planDisplay := fmt.Sprintf("Plan %s (%s strategy) [%s]", name, plan.Strategy, activePlanStatus.Status)
+		if name == lastPlanStatus.Name {
+			planDisplay := fmt.Sprintf("Plan %s (%s strategy) [%s]", name, plan.Strategy, lastPlanStatus.Status)
 			planBranchName := rootBranchName.AddBranch(planDisplay)
-			for _, phase := range activePlanStatus.Phases {
+			for _, phase := range lastPlanStatus.Phases {
 				phaseDisplay := fmt.Sprintf("Phase %s [%s]", phase.Name, phase.Status)
 				phaseBranchName := planBranchName.AddBranch(phaseDisplay)
 				for _, steps := range phase.Steps {