Get rid of unnecessary DIC reconcile updates (#2294)

* Get rid of unnecessary DIC reconcile updates

Also fixed status.lastExecutionTimestamp to be the last polling time
as intended in the design, and not the last reconcile update time.

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

* Pass DIC last execution time by annotation

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

pkg/controller/dataimportcron-controller.go

@@ -95,6 +95,8 @@ const (
 	AnnImageStreamDockerRef = AnnAPIGroup + "/storage.import.imageStreamDockerRef"
 	// AnnNextCronTime is the next time stamp which satisfies the cron expression
 	AnnNextCronTime = AnnAPIGroup + "/storage.import.nextCronTime"
+	// AnnLastCronTime is the cron last execution time stamp
+	AnnLastCronTime = AnnAPIGroup + "/storage.import.lastCronTime"
 
 	dataImportControllerName    = "dataimportcron-controller"
 	digestPrefix                = "sha256:"
@@ -247,9 +249,7 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
 	log := r.log.WithName("update")
 	res := reconcile.Result{}
 
-	now := metav1.Now()
+	dataImportCronCopy := dataImportCron.DeepCopy()
-	dataImportCron.Status.LastExecutionTimestamp = &now
-
 	importSucceeded := false
 	imports := dataImportCron.Status.CurrentImports
 	dataVolume := &cdiv1.DataVolume{}
@@ -270,7 +270,7 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
 		switch dataVolume.Status.Phase {
 		case cdiv1.Succeeded:
 			importSucceeded = true
-			if err := r.updateDataImportCronOnSuccess(ctx, dataImportCron); err != nil {
+			if err := updateDataImportCronOnSuccess(dataImportCron); err != nil {
 				return res, err
 			}
 			updateDataImportCronCondition(dataImportCron, cdiv1.DataImportCronProgressing, corev1.ConditionFalse, "No current import", noImport)
@@ -324,9 +324,15 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
 		updateDataImportCronCondition(dataImportCron, cdiv1.DataImportCronUpToDate, corev1.ConditionFalse, "No source digest", noDigest)
 	}
 
-	if err := r.client.Update(ctx, dataImportCron); err != nil {
+	if err := updateLastExecutionTimestamp(dataImportCron); err != nil {
 		return res, err
 	}
+
+	if !reflect.DeepEqual(dataImportCron, dataImportCronCopy) {
+		if err := r.client.Update(ctx, dataImportCron); err != nil {
+			return res, err
+		}
+	}
 	return res, nil
 }
 
@@ -366,6 +372,7 @@ func (r *DataImportCronReconciler) updateImageStreamDesiredDigest(ctx context.Co
 	if err != nil {
 		return err
 	}
+	AddAnnotation(dataImportCron, AnnLastCronTime, time.Now().Format(time.RFC3339))
 	if digest != "" && dataImportCron.Annotations[AnnSourceDesiredDigest] != digest {
 		log.Info("Updating DataImportCron", "digest", digest)
 		AddAnnotation(dataImportCron, AnnSourceDesiredDigest, digest)
@@ -408,7 +415,7 @@ func (r *DataImportCronReconciler) updateDataSource(ctx context.Context, dataImp
 	return nil
 }
 
-func (r *DataImportCronReconciler) updateDataImportCronOnSuccess(ctx context.Context, dataImportCron *cdiv1.DataImportCron) error {
+func updateDataImportCronOnSuccess(dataImportCron *cdiv1.DataImportCron) error {
 	if dataImportCron.Status.CurrentImports == nil {
 		return errors.Errorf("No CurrentImports in cron %s", dataImportCron.Name)
 	}
@@ -424,6 +431,21 @@ func (r *DataImportCronReconciler) updateDataImportCronOnSuccess(ctx context.Con
 	return nil
 }
 
+func updateLastExecutionTimestamp(cron *cdiv1.DataImportCron) error {
+	lastTimeStr := cron.Annotations[AnnLastCronTime]
+	if lastTimeStr == "" {
+		return nil
+	}
+	lastTime, err := time.Parse(time.RFC3339, lastTimeStr)
+	if err != nil {
+		return err
+	}
+	if ts := cron.Status.LastExecutionTimestamp; ts == nil || ts.Time != lastTime {
+		cron.Status.LastExecutionTimestamp = &metav1.Time{lastTime}
+	}
+	return nil
+}
+
 func (r *DataImportCronReconciler) createImportDataVolume(ctx context.Context, dataImportCron *cdiv1.DataImportCron) error {
 	log := r.log.WithName("createImportDataVolume")
 	dataSourceName := dataImportCron.Spec.ManagedDataSource

pkg/controller/dataimportcron-controller_test.go

@@ -241,7 +241,6 @@ var _ = Describe("All DataImportCron Tests", func() {
 			err = reconciler.client.Update(context.TODO(), dv)
 			Expect(err).ToNot(HaveOccurred())
 			verifyConditions("Import in progress", true, false, false, inProgress, inProgress, noPvc)
-			Expect(cron.Status.LastExecutionTimestamp).ToNot(BeNil())
 
 			dv.Status.Phase = cdiv1.Succeeded
 			err = reconciler.client.Update(context.TODO(), dv)

tools/cdi-source-update-poller/main.go

@@ -7,6 +7,7 @@ import (
 	"log"
 	"os"
 	"strconv"
+	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/clientcmd"
@@ -64,21 +65,21 @@ func main() {
 
 	dataImportCron, err := cdiClient.CdiV1beta1().DataImportCrons(cronNamespace).Get(context.TODO(), cronName, metav1.GetOptions{})
 	if err != nil {
-		log.Fatalf("Failed getting DataImportCron, cronNamespace %s cronName %s: %v", cronNamespace, cronName, err)
+		log.Fatalf("Failed getting DataImportCron %s/%s: %v", cronNamespace, cronName, err)
 	}
+	controller.AddAnnotation(dataImportCron, controller.AnnLastCronTime, time.Now().Format(time.RFC3339))
+
 	imports := dataImportCron.Status.CurrentImports
 	if digest != "" && (imports == nil || digest != imports[0].Digest) &&
 		digest != dataImportCron.Annotations[controller.AnnSourceDesiredDigest] {
-		if dataImportCron.Annotations == nil {
+		controller.AddAnnotation(dataImportCron, controller.AnnSourceDesiredDigest, digest)
-			dataImportCron.Annotations = make(map[string]string)
+		fmt.Println("Digest updated")
-		}
-		dataImportCron.Annotations[controller.AnnSourceDesiredDigest] = digest
-		dataImportCron, err = cdiClient.CdiV1beta1().DataImportCrons(cronNamespace).Update(context.TODO(), dataImportCron, metav1.UpdateOptions{})
-		if err != nil {
-			log.Fatalf("Failed updating DataImportCron, cronNamespace %s cronName %s: %v", cronNamespace, cronName, err)
-		}
-		fmt.Println("Updated DataImportCron", dataImportCron.Name)
 	} else {
-		fmt.Println("No update to DataImportCron", dataImportCron.Name)
+		fmt.Println("No digest update")
+	}
+
+	_, err = cdiClient.CdiV1beta1().DataImportCrons(cronNamespace).Update(context.TODO(), dataImportCron, metav1.UpdateOptions{})
+	if err != nil {
+		log.Fatalf("Failed updating DataImportCron %s/%s: %v", cronNamespace, cronName, err)
 	}
 }