Repository: metal3-io/baremetal-operator
Pull request #747 (2020-12-09 13:51:28)

Filter out status updates from the reconcile loop
Everytime the BMH status is saved the `LastUpdated` field is updated with the current time, and a new update is generated if the marshalled string results different from the previous version (up to the seconds), producing more reconcile loops than needed.
This PR filters out all the updates on the Status by ignoring all those ones that do not increase the Generation field (except for the finalizers and annotations) [1]

[1] https://www.openshift.com/blog/kubernetes-operators-best-practices


metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/747#" title="Author self-approved">andfasano</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **maelk**
You can assign the PR to them by writing `/assign @maelk` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["maelk"]} -->
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/747#" title="Author self-approved">andfasano</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/747#pullrequestreview-548206081" title="Approved">dhellmann</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [dhellmann]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dhellmann: /test-integration

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index dfc24360..a1cf3e9c 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"reflect"
 	"strconv"
 	"strings"
 	"time"
@@ -909,6 +910,30 @@ func hostHasFinalizer(host *metal3v1alpha1.BareMetalHost) bool {
 	return utils.StringInList(host.Finalizers, metal3v1alpha1.BareMetalHostFinalizer)
 }
 
+func (r *BareMetalHostReconciler) updateEventHandler(e event.UpdateEvent) bool {
+	_, oldOK := e.ObjectOld.(*metal3v1alpha1.BareMetalHost)
+	_, newOK := e.ObjectNew.(*metal3v1alpha1.BareMetalHost)
+	if !(oldOK && newOK) {
+		// The thing that changed wasn't a host, so we
+		// need to assume that we must update. This
+		// happens when, for example, an owned Secret
+		// changes.
+		return true
+	}
+
+	//If the update increased the resource Generation then let's process it
+	if e.MetaNew.GetGeneration() != e.MetaOld.GetGeneration() {
+		return true
+	}
+
+	//Discard updates that did not increase the resource Generation (such as on Status.LastUpdated), except for the finalizers or annotations
+	if reflect.DeepEqual(e.MetaNew.GetFinalizers(), e.MetaOld.GetFinalizers()) && reflect.DeepEqual(e.MetaNew.GetAnnotations(), e.MetaOld.GetAnnotations()) {
+		return false
+	}
+
+	return true
+}
+
 // SetupWithManager reigsters the reconciler to be run by the manager
 func (r *BareMetalHostReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
@@ -936,23 +961,7 @@ func (r *BareMetalHostReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		For(&metal3v1alpha1.BareMetalHost{}).
 		WithEventFilter(
 			predicate.Funcs{
-				UpdateFunc: func(e event.UpdateEvent) bool {
-					oldHost, oldOK := e.ObjectOld.(*metal3v1alpha1.BareMetalHost)
-					newHost, newOK := e.ObjectNew.(*metal3v1alpha1.BareMetalHost)
-					if !(oldOK && newOK) {
-						// The thing that changed wasn't a host, so we
-						// need to assume that we must update. This
-						// happens when, for example, an owned Secret
-						// changes.
-						return true
-					}
-
-					if oldHost.Status.ErrorCount != newHost.Status.ErrorCount {
-						//skip reconcile loop
-						return false
-					}
-					return true
-				},
+				UpdateFunc: r.updateEventHandler,
 			}).
 		WithOptions(opts).
 		Owns(&corev1.Secret{}).


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 25e28b9a..0a550f0a 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -19,6 +19,7 @@ import (
 
 	ctrl "sigs.k8s.io/controller-runtime"
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
@@ -1174,3 +1175,126 @@ func TestProvisionerIsReady(t *testing.T) {
 		},
 	)
 }
+
+func TestUpdateEventHandler(t *testing.T) {
+	cases := []struct {
+		name            string
+		event           event.UpdateEvent
+		expectedProcess bool
+	}{
+		{
+			name: "process-non-bmh-events",
+			event: event.UpdateEvent{
+				ObjectOld: &corev1.Secret{},
+				ObjectNew: &corev1.Secret{},
+			},
+			expectedProcess: true,
+		},
+		{
+			name: "process-generation-change",
+			event: event.UpdateEvent{
+				ObjectOld: &metal3v1alpha1.BareMetalHost{},
+				ObjectNew: &metal3v1alpha1.BareMetalHost{},
+				MetaOld:   &metav1.ObjectMeta{Generation: 0},
+				MetaNew:   &metav1.ObjectMeta{Generation: 1},
+			},
+
+			expectedProcess: true,
+		},
+		{
+			name: "skip-if-same-generation-finalizers-and-annotations",
+			event: event.UpdateEvent{
+				ObjectOld: &metal3v1alpha1.BareMetalHost{},
+				ObjectNew: &metal3v1alpha1.BareMetalHost{},
+				MetaOld: &metav1.ObjectMeta{
+					Generation: 0,
+					Finalizers: []string{metal3v1alpha1.BareMetalHostFinalizer},
+					Annotations: map[string]string{
+						metal3v1alpha1.PausedAnnotation: "true",
+					},
+				},
+				MetaNew: &metav1.ObjectMeta{
+					Generation: 0,
+					Finalizers: []string{metal3v1alpha1.BareMetalHostFinalizer},
+					Annotations: map[string]string{
+						metal3v1alpha1.PausedAnnotation: "true",
+					},
+				},
+			},
+
+			expectedProcess: false,
+		},
+		{
+			name: "process-same-generation-annotations-change",
+			event: event.UpdateEvent{
+				ObjectOld: &metal3v1alpha1.BareMetalHost{},
+				ObjectNew: &metal3v1alpha1.BareMetalHost{},
+				MetaOld: &metav1.ObjectMeta{
+					Generation:  0,
+					Finalizers:  []string{metal3v1alpha1.BareMetalHostFinalizer},
+					Annotations: map[string]string{},
+				},
+				MetaNew: &metav1.ObjectMeta{
+					Generation: 0,
+					Finalizers: []string{metal3v1alpha1.BareMetalHostFinalizer},
+					Annotations: map[string]string{
+						metal3v1alpha1.PausedAnnotation: "true",
+					},
+				},
+			},
+
+			expectedProcess: true,
+		},
+		{
+			name: "process-same-generation-finalizers-change",
+			event: event.UpdateEvent{
+				ObjectOld: &metal3v1alpha1.BareMetalHost{},
+				ObjectNew: &metal3v1alpha1.BareMetalHost{},
+				MetaOld: &metav1.ObjectMeta{
+					Generation: 0,
+					Finalizers: []string{},
+					Annotations: map[string]string{
+						metal3v1alpha1.PausedAnnotation: "true",
+					},
+				},
+				MetaNew: &metav1.ObjectMeta{
+					Generation: 0,
+					Finalizers: []string{metal3v1alpha1.BareMetalHostFinalizer},
+					Annotations: map[string]string{
+						metal3v1alpha1.PausedAnnotation: "true",
+					},
+				},
+			},
+
+			expectedProcess: true,
+		},
+		{
+			name: "process-same-generation-finalizers-and-annotation-change",
+			event: event.UpdateEvent{
+				ObjectOld: &metal3v1alpha1.BareMetalHost{},
+				ObjectNew: &metal3v1alpha1.BareMetalHost{},
+				MetaOld: &metav1.ObjectMeta{
+					Generation:  0,
+					Finalizers:  []string{},
+					Annotations: map[string]string{},
+				},
+				MetaNew: &metav1.ObjectMeta{
+					Generation: 0,
+					Finalizers: []string{metal3v1alpha1.BareMetalHostFinalizer},
+					Annotations: map[string]string{
+						metal3v1alpha1.PausedAnnotation: "true",
+					},
+				},
+			},
+
+			expectedProcess: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			r := newTestReconciler()
+			assert.Equal(t, tc.expectedProcess, r.updateEventHandler(tc.event))
+		})
+	}
+}