Merge pull request #251 from rabbitmq/last-transition-time

ansd · web-flow · commit b3ffeaaa4bd8 · 2021-10-01T10:32:08.000+02:00
Fix endless reconcile issue
diff --git a/api/v1beta1/conditions.go b/api/v1beta1/conditions.go
@@ -14,7 +14,7 @@ type Condition struct {
 	Type ConditionType `json:"type"`
 	// True, False, or Unknown
 	Status corev1.ConditionStatus `json:"status"`
-	// The last time this Condition type changed.
+	// The last time this Condition status changed.
 	LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"`
 	// One word, camel-case reason for current status of the condition.
 	Reason string `json:"reason,omitempty"`
@@ -23,22 +23,33 @@ type Condition struct {
 }
 
 // Ready indicates that the last Create/Update operator on the CR was successful.
-func Ready() Condition {
+func Ready(lastConditions []Condition) Condition {
+	time := lastTransitionTime(corev1.ConditionTrue, lastConditions)
 	return Condition{
 		Type:               ready,
 		Status:             corev1.ConditionTrue,
-		LastTransitionTime: metav1.Now(),
+		LastTransitionTime: time,
 		Reason:             "SuccessfulCreateOrUpdate",
 	}
 }
 
 // NotReady indicates that the last Create/Update operator on the CR failed.
-func NotReady(msg string) Condition {
+func NotReady(msg string, lastConditions []Condition) Condition {
+	time := lastTransitionTime(corev1.ConditionFalse, lastConditions)
 	return Condition{
 		Type:               ready,
 		Status:             corev1.ConditionFalse,
-		LastTransitionTime: metav1.Now(),
+		LastTransitionTime: time,
 		Reason:             "FailedCreateOrUpdate",
 		Message:            msg,
 	}
 }
+
+func lastTransitionTime(newStatus corev1.ConditionStatus, lastConditions []Condition) metav1.Time {
+	for _, lastCondition := range lastConditions {
+		if lastCondition.Type == ready && lastCondition.Status == newStatus {
+			return lastCondition.LastTransitionTime
+		}
+	}
+	return metav1.Now()
+}
diff --git a/api/v1beta1/conditions_test.go b/api/v1beta1/conditions_test.go
@@ -4,29 +4,39 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 var _ = Describe("Conditions", func() {
-	Context("Ready", func() {
+	Describe("Ready", func() {
 		It("returns 'Ready' condition set to true", func() {
-			c := Ready()
+			c := Ready(nil)
 			Expect(string(c.Type)).To(Equal("Ready"))
 			Expect(c.Status).To(Equal(corev1.ConditionTrue))
 			Expect(c.Reason).To(Equal("SuccessfulCreateOrUpdate"))
-			Expect(c.LastTransitionTime).NotTo(Equal(metav1.Time{})) // has been set; not empty
+			Expect(c.LastTransitionTime.IsZero()).To(BeFalse())
 		})
-
 	})
-
-	Context("NotReady", func() {
+	Describe("NotReady", func() {
 		It("returns 'Ready' condition set to false", func() {
-			c := NotReady("fail to declare queue")
+			c := NotReady("fail to declare queue", nil)
 			Expect(string(c.Type)).To(Equal("Ready"))
 			Expect(c.Status).To(Equal(corev1.ConditionFalse))
 			Expect(c.Reason).To(Equal("FailedCreateOrUpdate"))
 			Expect(c.Message).To(Equal("fail to declare queue"))
-			Expect(c.LastTransitionTime).NotTo(Equal(metav1.Time{})) // has been set; not empty
+			Expect(c.LastTransitionTime.IsZero()).To(BeFalse())
+		})
+	})
+	Context("LastTransitionTime", func() {
+		It("changes only if status changes", func() {
+			c1 := Ready(nil)
+			Expect(c1.LastTransitionTime.IsZero()).To(BeFalse())
+			c2 := Ready([]Condition{
+				Condition{Type: "I'm some other type"},
+				c1,
+			})
+			Expect(c2.LastTransitionTime.Time).To(BeTemporally("==", c1.LastTransitionTime.Time))
+			c3 := NotReady("some message", []Condition{c2})
+			Expect(c3.LastTransitionTime.Time).To(BeTemporally(">", c2.LastTransitionTime.Time))
 		})
 	})
 })
diff --git a/config/crd/bases/rabbitmq.com_bindings.yaml b/config/crd/bases/rabbitmq.com_bindings.yaml
@@ -85,7 +85,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/config/crd/bases/rabbitmq.com_exchanges.yaml b/config/crd/bases/rabbitmq.com_exchanges.yaml
@@ -83,7 +83,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/config/crd/bases/rabbitmq.com_federations.yaml b/config/crd/bases/rabbitmq.com_federations.yaml
@@ -104,7 +104,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/config/crd/bases/rabbitmq.com_permissions.yaml b/config/crd/bases/rabbitmq.com_permissions.yaml
@@ -93,7 +93,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/config/crd/bases/rabbitmq.com_policies.yaml b/config/crd/bases/rabbitmq.com_policies.yaml
@@ -95,7 +95,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/config/crd/bases/rabbitmq.com_queues.yaml b/config/crd/bases/rabbitmq.com_queues.yaml
@@ -86,7 +86,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/config/crd/bases/rabbitmq.com_schemareplications.yaml b/config/crd/bases/rabbitmq.com_schemareplications.yaml
@@ -78,7 +78,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/config/crd/bases/rabbitmq.com_shovels.yaml b/config/crd/bases/rabbitmq.com_shovels.yaml
@@ -131,7 +131,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/config/crd/bases/rabbitmq.com_users.yaml b/config/crd/bases/rabbitmq.com_users.yaml
@@ -93,7 +93,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/config/crd/bases/rabbitmq.com_vhosts.yaml b/config/crd/bases/rabbitmq.com_vhosts.yaml
@@ -72,7 +72,7 @@ spec:
                 items:
                   properties:
                     lastTransitionTime:
-                      description: The last time this Condition type changed.
+                      description: The last time this Condition status changed.
                       format: date-time
                       type: string
                     message:
diff --git a/controllers/binding_controller.go b/controllers/binding_controller.go
@@ -70,7 +70,9 @@ func (r *BindingReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	}
 	if errors.Is(err, internal.ResourceNotAllowedError) {
 		logger.Info("Could not create binding resource: " + err.Error())
-		binding.Status.Conditions = []topology.Condition{topology.NotReady(internal.ResourceNotAllowedError.Error())}
+		binding.Status.Conditions = []topology.Condition{
+			topology.NotReady(internal.ResourceNotAllowedError.Error(), binding.Status.Conditions),
+		}
 		if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 			return r.Status().Update(ctx, binding)
 		}); writerErr != nil {
@@ -107,7 +109,9 @@ func (r *BindingReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 
 	if err := r.declareBinding(ctx, rabbitClient, binding); err != nil {
 		// Set Condition 'Ready' to false with message
-		binding.Status.Conditions = []topology.Condition{topology.NotReady(err.Error())}
+		binding.Status.Conditions = []topology.Condition{
+			topology.NotReady(err.Error(), binding.Status.Conditions),
+		}
 		if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 			return r.Status().Update(ctx, binding)
 		}); writerErr != nil {
@@ -116,7 +120,7 @@ func (r *BindingReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		return ctrl.Result{}, err
 	}
 
-	binding.Status.Conditions = []topology.Condition{topology.Ready()}
+	binding.Status.Conditions = []topology.Condition{topology.Ready(binding.Status.Conditions)}
 	binding.Status.ObservedGeneration = binding.GetGeneration()
 	if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 		return r.Status().Update(ctx, binding)
diff --git a/controllers/exchange_controller.go b/controllers/exchange_controller.go
@@ -68,7 +68,9 @@ func (r *ExchangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	}
 	if errors.Is(err, internal.ResourceNotAllowedError) {
 		logger.Info("Could not create exchange resource: " + err.Error())
-		exchange.Status.Conditions = []topology.Condition{topology.NotReady(internal.ResourceNotAllowedError.Error())}
+		exchange.Status.Conditions = []topology.Condition{
+			topology.NotReady(internal.ResourceNotAllowedError.Error(), exchange.Status.Conditions),
+		}
 		if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 			return r.Status().Update(ctx, exchange)
 		}); writerErr != nil {
@@ -106,7 +108,9 @@ func (r *ExchangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 
 	if err := r.declareExchange(ctx, rabbitClient, exchange); err != nil {
 		// Set Condition 'Ready' to false with message
-		exchange.Status.Conditions = []topology.Condition{topology.NotReady(err.Error())}
+		exchange.Status.Conditions = []topology.Condition{
+			topology.NotReady(err.Error(), exchange.Status.Conditions),
+		}
 		if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 			return r.Status().Update(ctx, exchange)
 		}); writerErr != nil {
@@ -115,7 +119,7 @@ func (r *ExchangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return ctrl.Result{}, err
 	}
 
-	exchange.Status.Conditions = []topology.Condition{topology.Ready()}
+	exchange.Status.Conditions = []topology.Condition{topology.Ready(exchange.Status.Conditions)}
 	exchange.Status.ObservedGeneration = exchange.GetGeneration()
 	if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 		return r.Status().Update(ctx, exchange)
diff --git a/controllers/exchange_controller_test.go b/controllers/exchange_controller_test.go
@@ -117,6 +117,74 @@ var _ = Describe("exchange-controller", func() {
 				})
 			})
 		})
+		Context("LastTransitionTime", func() {
+			BeforeEach(func() {
+				exchangeName = "test-last-transition-time"
+				fakeRabbitMQClient.DeclareExchangeReturns(&http.Response{
+					Status:     "201 Created",
+					StatusCode: http.StatusCreated,
+				}, nil)
+			})
+			It("changes only if status changes", func() {
+				By("setting LastTransitionTime when transitioning to status Ready=true")
+				Expect(client.Create(ctx, &exchange)).To(Succeed())
+				EventuallyWithOffset(1, func() []topology.Condition {
+					_ = client.Get(
+						ctx,
+						types.NamespacedName{Namespace: exchange.Namespace, Name: exchange.Name},
+						&exchange,
+					)
+					return exchange.Status.Conditions
+				}, 10*time.Second, 1*time.Second).Should(ConsistOf(MatchFields(IgnoreExtras, Fields{
+					"Type":   Equal(topology.ConditionType("Ready")),
+					"Status": Equal(corev1.ConditionTrue),
+				})))
+				lastTransitionTime := exchange.Status.Conditions[0].LastTransitionTime
+				Expect(lastTransitionTime.IsZero()).To(BeFalse())
+
+				By("not touching LastTransitionTime when staying in status Ready=true")
+				fakeRabbitMQClient.DeclareExchangeReturns(&http.Response{
+					Status:     "204 No Content",
+					StatusCode: http.StatusNoContent,
+				}, nil)
+				exchange.Labels = map[string]string{"k1": "v1"}
+				Expect(client.Update(ctx, &exchange)).To(Succeed())
+				ConsistentlyWithOffset(1, func() []topology.Condition {
+					_ = client.Get(
+						ctx,
+						types.NamespacedName{Namespace: exchange.Namespace, Name: exchange.Name},
+						&exchange,
+					)
+					return exchange.Status.Conditions
+				}, "3s").Should(ConsistOf(MatchFields(IgnoreExtras, Fields{
+					"Type":   Equal(topology.ConditionType("Ready")),
+					"Status": Equal(corev1.ConditionTrue),
+				})))
+				Expect(exchange.Status.Conditions[0].LastTransitionTime.Time).To(BeTemporally("==", lastTransitionTime.Time))
+
+				By("updating LastTransitionTime when transitioning to status Ready=false")
+				fakeRabbitMQClient.DeclareExchangeReturns(&http.Response{
+					Status:     "500 Internal Server Error",
+					StatusCode: http.StatusInternalServerError,
+				}, errors.New("something went wrong"))
+				exchange.Labels = map[string]string{"k1": "v2"}
+				Expect(client.Update(ctx, &exchange)).To(Succeed())
+				EventuallyWithOffset(1, func() []topology.Condition {
+					_ = client.Get(
+						ctx,
+						types.NamespacedName{Namespace: exchange.Namespace, Name: exchange.Name},
+						&exchange,
+					)
+					return exchange.Status.Conditions
+				}, 10*time.Second, 1*time.Second).Should(ConsistOf(MatchFields(IgnoreExtras, Fields{
+					"Type":    Equal(topology.ConditionType("Ready")),
+					"Status":  Equal(corev1.ConditionFalse),
+					"Reason":  Equal("FailedCreateOrUpdate"),
+					"Message": Equal("something went wrong"),
+				})))
+				Expect(exchange.Status.Conditions[0].LastTransitionTime.Time).To(BeTemporally(">", lastTransitionTime.Time))
+			})
+		})
 
 		Context("deletion", func() {
 			JustBeforeEach(func() {
diff --git a/controllers/federation_controller.go b/controllers/federation_controller.go
@@ -61,7 +61,9 @@ func (r *FederationReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 	if errors.Is(err, internal.ResourceNotAllowedError) {
 		logger.Info("Could not create federation resource: " + err.Error())
-		federation.Status.Conditions = []topology.Condition{topology.NotReady(internal.ResourceNotAllowedError.Error())}
+		federation.Status.Conditions = []topology.Condition{
+			topology.NotReady(internal.ResourceNotAllowedError.Error(), federation.Status.Conditions),
+		}
 		if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 			return r.Status().Update(ctx, federation)
 		}); writerErr != nil {
@@ -99,7 +101,9 @@ func (r *FederationReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 	if err := r.setFederation(ctx, rabbitClient, federation); err != nil {
 		// Set Condition 'Ready' to false with message
-		federation.Status.Conditions = []topology.Condition{topology.NotReady(err.Error())}
+		federation.Status.Conditions = []topology.Condition{
+			topology.NotReady(err.Error(), federation.Status.Conditions),
+		}
 		if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 			return r.Status().Update(ctx, federation)
 		}); writerErr != nil {
@@ -108,7 +112,7 @@ func (r *FederationReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, err
 	}
 
-	federation.Status.Conditions = []topology.Condition{topology.Ready()}
+	federation.Status.Conditions = []topology.Condition{topology.Ready(federation.Status.Conditions)}
 	federation.Status.ObservedGeneration = federation.GetGeneration()
 	if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 		return r.Status().Update(ctx, federation)
diff --git a/controllers/permission_controller.go b/controllers/permission_controller.go
@@ -62,7 +62,9 @@ func (r *PermissionReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 	if errors.Is(err, internal.ResourceNotAllowedError) {
 		logger.Info("Could not create permission resource: " + err.Error())
-		permission.Status.Conditions = []topology.Condition{topology.NotReady(internal.ResourceNotAllowedError.Error())}
+		permission.Status.Conditions = []topology.Condition{
+			topology.NotReady(internal.ResourceNotAllowedError.Error(), permission.Status.Conditions),
+		}
 		if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 			return r.Status().Update(ctx, permission)
 		}); writerErr != nil {
@@ -107,7 +109,9 @@ func (r *PermissionReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 	if err := r.updatePermissions(ctx, rabbitClient, permission, user); err != nil {
 		// Set Condition 'Ready' to false with message
-		permission.Status.Conditions = []topology.Condition{topology.NotReady(err.Error())}
+		permission.Status.Conditions = []topology.Condition{
+			topology.NotReady(err.Error(), permission.Status.Conditions),
+		}
 		if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 			return r.Status().Update(ctx, permission)
 		}); writerErr != nil {
@@ -116,7 +120,7 @@ func (r *PermissionReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, err
 	}
 
-	permission.Status.Conditions = []topology.Condition{topology.Ready()}
+	permission.Status.Conditions = []topology.Condition{topology.Ready(permission.Status.Conditions)}
 	permission.Status.ObservedGeneration = permission.GetGeneration()
 	if writerErr := clientretry.RetryOnConflict(clientretry.DefaultRetry, func() error {
 		return r.Status().Update(ctx, permission)
diff --git a/controllers/policy_controller.go b/controllers/policy_controller.go
diff --git a/controllers/queue_controller.go b/controllers/queue_controller.go
diff --git a/controllers/schemareplication_controller.go b/controllers/schemareplication_controller.go
diff --git a/controllers/shovel_controller.go b/controllers/shovel_controller.go
diff --git a/controllers/user_controller.go b/controllers/user_controller.go
diff --git a/controllers/vhost_controller.go b/controllers/vhost_controller.go
diff --git a/docs/api/rabbitmq.com.ref.asciidoc b/docs/api/rabbitmq.com.ref.asciidoc
