diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go index dfc243609a..a1cf3e9c9d 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{}). diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go index 25e28b9a22..0a550f0ae2 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)) + }) + } +}