Add check for resourceVersions on non-ManagedDataabse types

Author: kleesc

controllers/event_filters.go

@@ -18,8 +18,9 @@ var _ predicate.Predicate = ManagedDatabaseVersionChangedPredicate{}
 // the spec field of an object.  This allows a controller to ignore update events where the spec is unchanged,
 // and only the metadata and/or status fields are changed.
 //
-// This predicate will skip update events that have no change in the object's metadata.generation field,
-// unless it is a ManagedDatabase, and its Status.CurrentVersion subresource changes.
+// This predicate will skip update events by a ManagedDatabase where its metadata.generation field is not changed,
+// unless its Status.CurrentVersion subresource changes. For other object types, update are sent on
+// ResourceVersion changes (default).
 type ManagedDatabaseVersionChangedPredicate struct {
 	predicate.Funcs
 }
@@ -42,23 +43,32 @@ func (ManagedDatabaseVersionChangedPredicate) Update(e event.UpdateEvent) bool {
 		log.Error(nil, "Update event has no new metadata", "event", e)
 		return false
 	}
-	// Check that the status subresource is enabled, and for currentVersion changes
-	// in the ManagedDatabase.
-	if e.MetaNew.GetGeneration() > 0 && e.MetaNew.GetGeneration() == e.MetaOld.GetGeneration() {
-		oldMdb, ok := e.ObjectOld.(*dba.ManagedDatabase)
-		if !ok {
-			return false
-		}
 
-		newMdb, ok := e.ObjectNew.(*dba.ManagedDatabase)
-		if !ok {
+	oldMdb, ok := e.ObjectOld.(*dba.ManagedDatabase)
+	if !ok {
+		if e.MetaNew.GetResourceVersion() == e.MetaOld.GetResourceVersion() {
 			return false
 		}
+		return true
+	}
+
+	newMdb, ok := e.ObjectNew.(*dba.ManagedDatabase)
+	if !ok {
+		return false
+	}
 
+	// Check that the status subresource is enabled, and for currentVersion changes
+	// in the ManagedDatabase.
+	if e.MetaNew.GetGeneration() > 0 && e.MetaNew.GetGeneration() == e.MetaOld.GetGeneration() {
 		if oldMdb.Status.CurrentVersion != newMdb.Status.CurrentVersion {
 			return true
 		}
 		return false
 	}
+
+	if e.MetaNew.GetResourceVersion() == e.MetaOld.GetResourceVersion() {
+		return false
+	}
+
 	return true
 }

controllers/event_filters_test.go

@@ -6,14 +6,16 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	dba "github.com/app-sre/dba-operator/api/v1alpha1"
+	batchv1 "k8s.io/api/batch/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 )
 
-func managedDatabase(generation int64, currentVersion string) *dba.ManagedDatabase {
+func managedDatabase(generation int64, resourceVersion, currentVersion string) *dba.ManagedDatabase {
 	managedDatabase := &dba.ManagedDatabase{
 		ObjectMeta: metav1.ObjectMeta{
-			Generation: generation,
+			ResourceVersion: resourceVersion,
+			Generation:      generation,
 		},
 		Status: dba.ManagedDatabaseStatus{
 			CurrentVersion: currentVersion,
@@ -22,27 +24,44 @@ func managedDatabase(generation int64, currentVersion string) *dba.ManagedDataba
 	return managedDatabase
 }
 
+func job(generation int64, resourceVersion string, active int32) *batchv1.Job {
+	job := &batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{
+			ResourceVersion: resourceVersion,
+			Generation:      generation,
+		},
+		Status: batchv1.JobStatus{
+			Active: active,
+		},
+	}
+	return job
+}
+
 func TestManagedDatabaseVersionChangedPredicate(t *testing.T) {
+	mdbPredicate := ManagedDatabaseVersionChangedPredicate{}
+
 	var managedDatabaseUpdatesForPredicateTests = []struct {
-		oldGeneration     int
-		oldCurrentVersion string
-		newGeneration     int
-		newCurrentVersion string
-		expected          bool
+		oldGeneration      int
+		oldResourceVersion string
+		oldCurrentVersion  string
+		newGeneration      int
+		newResourceVersion string
+		newCurrentVersion  string
+		expected           bool
 	}{
-		{0, "v1", 0, "v2", true},  // Status subresource not enabled, currentVersion change
-		{0, "v1", 0, "v1", true},  // Status subresource not enabled, some update other than currentVersion
-		{1, "v1", 1, "v1", false}, // Same generation, same currentVersion (change to some other field)
-		{1, "v1", 1, "v2", true},  // currentVersion change, same generation
-		{1, "v1", 2, "v1", true},  // Generation change, same currentVersion
-		{1, "v1", 2, "v2", true},  // Generation change & version change
+		{0, "1", "v1", 0, "1", "v1", false}, // No-change update
+		{1, "1", "v1", 1, "1", "v1", false}, // No-change update w/ generation field set
+		{0, "1", "v1", 0, "2", "v2", true},  // Generation not set, currentVersion change (resourceVersion change)
+		{0, "1", "v1", 0, "2", "v1", true},  // Generation not set, some update other than currentVersion
+		{1, "1", "v1", 1, "2", "v1", false}, // Same generation, same currentVersion (change to some other field)
+		{1, "1", "v1", 1, "2", "v2", true},  // currentVersion change, same generation
+		{1, "1", "v1", 2, "2", "v1", true},  // Generation change, same currentVersion
+		{1, "1", "v1", 2, "2", "v2", true},  // Generation change & currentVersion change
 	}
 
-	mdbPredicate := ManagedDatabaseVersionChangedPredicate{}
-
 	for _, update := range managedDatabaseUpdatesForPredicateTests {
-		oldManagedDatabase := managedDatabase(int64(update.oldGeneration), update.oldCurrentVersion)
-		newManagedDatabase := managedDatabase(int64(update.newGeneration), update.newCurrentVersion)
+		oldManagedDatabase := managedDatabase(int64(update.oldGeneration), update.oldResourceVersion, update.oldCurrentVersion)
+		newManagedDatabase := managedDatabase(int64(update.newGeneration), update.newResourceVersion, update.newCurrentVersion)
 
 		updateEvent := event.UpdateEvent{
 			MetaOld:   &oldManagedDatabase.ObjectMeta,
@@ -53,4 +72,37 @@ func TestManagedDatabaseVersionChangedPredicate(t *testing.T) {
 
 		assert.Equal(t, mdbPredicate.Update(updateEvent), update.expected)
 	}
+
+	var jobUpdatesForPredicateTests = []struct {
+		oldGeneration      int
+		oldResourceVersion string
+		oldActive          int
+		newGeneration      int
+		newResourceVersion string
+		newActive          int
+		expected           bool
+	}{
+		{0, "1", 0, 0, "1", 0, false}, // No-change update
+		{1, "1", 0, 1, "1", 0, false}, // No-change update w/ generation field set
+		{0, "1", 0, 0, "2", 1, true},  // Status change, no generation, version update
+		{1, "1", 0, 1, "2", 1, true},  // Status change, generation, version update
+	}
+
+	// Any other objects other than ManagedDatabase should reconcile unless there is no changes.
+	//
+	// NOTE: It doesn't look like generation in incremented (or used) for all types by the API server.
+	//       For example, as of 1.15, changing the Job types does not make use of the generation field.
+	for _, update := range jobUpdatesForPredicateTests {
+		oldJob := job(int64(update.oldGeneration), update.oldResourceVersion, int32(update.oldActive))
+		newJob := job(int64(update.newGeneration), update.newResourceVersion, int32(update.newActive))
+
+		updateEvent := event.UpdateEvent{
+			MetaOld:   &oldJob.ObjectMeta,
+			ObjectOld: oldJob,
+			MetaNew:   &newJob.ObjectMeta,
+			ObjectNew: newJob,
+		}
+
+		assert.Equal(t, mdbPredicate.Update(updateEvent), update.expected)
+	}
 }