From c7395c1ce22164c6b4bdbb1edad1fe41f045e0c0 Mon Sep 17 00:00:00 2001
From: Balazs Gibizer <gibi@redhat.com>
Date: Thu, 13 Jun 2024 15:22:44 +0200
Subject: [PATCH] Restore condition transition time at the right place

As we are resetting the condition list in each reconcile we have the
logic to restore the last transition timestamp of the unchanged
conditions in at the end of the reconcile. However in the nova
controller the restore call was after the instance is was persisted and
therefore it was ineffective. This could lead to extra reconciles in
openstack-operator even if the nova-operator needed to update nothing in
the underlying resources.
---
 controllers/nova_controller.go               |  2 +-
 go.mod                                       |  2 +-
 test/functional/nova_reconfiguration_test.go | 24 ++++++++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/controllers/nova_controller.go b/controllers/nova_controller.go
index 0e247cab8..af45c9179 100644
--- a/controllers/nova_controller.go
+++ b/controllers/nova_controller.go
@@ -159,8 +159,8 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul
 			instance.Status.Conditions.Set(
 				instance.Status.Conditions.Mirror(condition.ReadyCondition))
 		}
-		err := h.PatchInstance(ctx, instance)
 		condition.RestoreLastTransitionTimes(&instance.Status.Conditions, savedConditions)
+		err := h.PatchInstance(ctx, instance)
 		if err != nil {
 			_err = err
 			return
diff --git a/go.mod b/go.mod
index e1bbd1030..542a5c0c0 100644
--- a/go.mod
+++ b/go.mod
@@ -4,6 +4,7 @@ go 1.20
 
 require (
 	github.com/go-logr/logr v1.4.2
+	github.com/google/go-cmp v0.6.0
 	github.com/google/uuid v1.6.0
 	github.com/gophercloud/gophercloud v1.12.0
 	github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.4.0
@@ -42,7 +43,6 @@ require (
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.4 // indirect
 	github.com/google/gnostic-models v0.6.8 // indirect
-	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/pprof v0.0.0-20240424215950-a892ee059fd6 // indirect
 	github.com/imdario/mergo v0.3.16 // indirect
diff --git a/test/functional/nova_reconfiguration_test.go b/test/functional/nova_reconfiguration_test.go
index 36473c0ce..89db43011 100644
--- a/test/functional/nova_reconfiguration_test.go
+++ b/test/functional/nova_reconfiguration_test.go
@@ -17,7 +17,9 @@ package functional_test
 
 import (
 	"fmt"
+	"time"
 
+	"github.com/google/go-cmp/cmp"
 	. "github.com/onsi/ginkgo/v2" //revive:disable:dot-imports
 	. "github.com/onsi/gomega"    //revive:disable:dot-imports
 
@@ -766,4 +768,26 @@ var _ = Describe("Nova reconfiguration", func() {
 			g.Expect(GetEnvVarValue(jobEnv, "PURGE_AGE", "")).To(Equal("99"))
 		}, timeout, interval).Should(Succeed())
 	})
+	It("does not change status if label is added", func() {
+		// ensure that any newly generated timestamp i.e. in the condition list
+		// will result in a different string representation
+		time.Sleep(time.Second)
+
+		var oldStatus *novav1.NovaStatus
+		Eventually(func(g Gomega) {
+			nova := GetNova(novaNames.NovaName)
+			oldStatus = nova.Status.DeepCopy()
+
+			nova.Labels = map[string]string{
+				"foo": "bar",
+			}
+			g.Expect(k8sClient.Update(ctx, nova)).To(Succeed())
+		}, timeout, interval).Should(Succeed())
+
+		Consistently(func(g Gomega) {
+			newStatus := &GetNova(novaNames.NovaName).Status
+			diff := cmp.Diff(oldStatus, newStatus)
+			g.Expect(diff).To(BeEmpty())
+		}, timeout, interval).Should(Succeed())
+	})
 })