Locally store and check resourceVersions

The initial `Get` in a Reconcile, for the object to be reconciled, reads
from the controller-runtime cache. If a resource is updated and then
immediately re-queued, the object retrieved from the cache may not have
that update. Now, when updating a ConfigurationPolicy or OperatorPolicy,
the controller locally saves the resulting resourceVersion, and then
checks against that value when beginning a reconcile.

Previously, ConfigurationPolicy had something similar, but it was using
the `status.lastEvaluated` timestamp. That only has seconds precision,
and so it was not catching every situation where the cache might be
stale - in particular, this caused a status history test to be flakey.

Refs:
 - https://issues.redhat.com/browse/ACM-20803

Signed-off-by: Justin Kulikauskas <[email protected]>

Author: JustinKuli

Makefile

@@ -18,8 +18,6 @@ LOCAL_BIN ?= $(PWD)/bin
 export PATH := $(LOCAL_BIN):$(PATH)
 GOARCH = $(shell go env GOARCH)
 GOOS = $(shell go env GOOS)
-TESTARGS_DEFAULT := -v
-TESTARGS ?= $(TESTARGS_DEFAULT)
 CONTROLLER_NAME = $(shell cat COMPONENT_NAME 2> /dev/null)
 # Handle KinD configuration
 MANAGED_CLUSTER_SUFFIX ?= 

controllers/configurationpolicy_controller.go

@@ -193,7 +193,7 @@ type ConfigurationPolicyReconciler struct {
 	// The number of seconds before a policy is eligible for reevaluation in watch mode (throttles frequently evaluated
 	// policies)
 	EvalBackoffSeconds uint32
-	// lastEvaluatedCache contains the value of ConfigurationPolicyStatus.LastEvaluated per ConfigurationPolicy UID.
+	// lastEvaluatedCache contains the value of the last known ConfigurationPolicy resourceVersion per UID.
 	// This is a workaround to account for race conditions where the status is updated but the controller-runtime cache
 	// has not updated yet.
 	lastEvaluatedCache sync.Map
@@ -404,12 +404,22 @@ func (r *ConfigurationPolicyReconciler) shouldEvaluatePolicy(
 	}
 
 	if cachedLastEval, ok := r.lastEvaluatedCache.Load(policy.UID); ok {
-		if cachedLastEval.(string) != policy.Status.LastEvaluated {
-			log.V(1).Info(
-				"The policy's status.lastEvaluated field is not synced in the controller-runtime cache. Will requeue.",
-			)
+		last, cachedConversionErr := strconv.Atoi(cachedLastEval.(string))
+		current, realConversionErr := strconv.Atoi(policy.GetResourceVersion())
+
+		// Note: resourceVersion should be strictly monotonic *for a specific resource instance*,
+		// but is not necessarily monotonic across different kinds and namespaces.
+		// The comparison here is for a specific resource, and so it should be a valid check.
+		if cachedConversionErr == nil && realConversionErr == nil {
+			if last > current {
+				log.V(1).Info("The policy from the controller-runtime cache is stale. Will requeue.")
 
-			return false, time.Second
+				return false, time.Second
+			}
+		} else {
+			log.Error(nil, "A resourceVersion could not be converted to an integer. Possibly evaluating regardless.",
+				"cache", cachedLastEval, "cachedConversionErr", cachedConversionErr,
+				"real", policy.GetResourceVersion(), "realConversionErr", realConversionErr)
 		}
 	}
 
@@ -3696,7 +3706,7 @@ func (r *ConfigurationPolicyReconciler) addForUpdate(policy *policyv1.Configurat
 		policySystemErrorsCounter.WithLabelValues(parent, policy.GetName(), "status-update-failed").Add(1)
 
 	default:
-		r.lastEvaluatedCache.Store(policy.UID, policy.Status.LastEvaluated)
+		r.lastEvaluatedCache.Store(policy.UID, policy.GetResourceVersion())
 	}
 }
 

controllers/configurationpolicy_controller_test.go

@@ -1118,22 +1118,26 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 
 	// Add a 60 second buffer to avoid race conditions
 	inFuture := time.Now().UTC().Add(60 * time.Second).Format(time.RFC3339)
+	futureResourceVersion := "900000" // 900_000
 	lastEvaluatedInFuture := &sync.Map{}
-	lastEvaluatedInFuture.Store(policy.UID, inFuture)
+	lastEvaluatedInFuture.Store(policy.UID, futureResourceVersion)
 
 	lastEvaluatedInvalid := &sync.Map{}
 	lastEvaluatedInvalid.Store(policy.UID, "Do or do not. There is no try.")
 
 	twelveSecsAgo := time.Now().UTC().Add(-12 * time.Second).Format(time.RFC3339)
+	recentResourceVersion := "700000" // 700_000
 	lastEvaluatedTwelveSecsAgo := &sync.Map{}
-	lastEvaluatedTwelveSecsAgo.Store(policy.UID, twelveSecsAgo)
+	lastEvaluatedTwelveSecsAgo.Store(policy.UID, recentResourceVersion)
 
 	twelveHoursAgo := time.Now().UTC().Add(-12 * time.Hour).Format(time.RFC3339)
+	oldResourceVersion := "20000" // 20_000
 	lastEvaluatedTwelveHoursAgo := &sync.Map{}
-	lastEvaluatedTwelveHoursAgo.Store(policy.UID, twelveHoursAgo)
+	lastEvaluatedTwelveHoursAgo.Store(policy.UID, oldResourceVersion)
 
 	tests := []struct {
 		testDescription          string
+		resourceVersion          string
 		lastEvaluated            string
 		lastEvaluatedGeneration  int64
 		evaluationInterval       policyv1.EvaluationInterval
@@ -1147,6 +1151,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 	}{
 		{
 			"Just evaluated and the generation is unchanged",
+			futureResourceVersion,
 			inFuture,
 			2,
 			policyv1.EvaluationInterval{Compliant: "10s", NonCompliant: "10s"},
@@ -1160,6 +1165,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"The generation has changed",
+			futureResourceVersion,
 			inFuture,
 			1,
 			policyv1.EvaluationInterval{Compliant: "10s", NonCompliant: "10s"},
@@ -1173,6 +1179,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"lastEvaluated not set",
+			futureResourceVersion,
 			"",
 			2,
 			policyv1.EvaluationInterval{Compliant: "10s", NonCompliant: "10s"},
@@ -1186,6 +1193,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Invalid lastEvaluated",
+			futureResourceVersion,
 			"Do or do not. There is no try.",
 			2,
 			policyv1.EvaluationInterval{Compliant: "10s", NonCompliant: "10s"},
@@ -1199,6 +1207,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Unknown compliance state",
+			futureResourceVersion,
 			inFuture,
 			2,
 			policyv1.EvaluationInterval{Compliant: "10s", NonCompliant: "10s"},
@@ -1212,6 +1221,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Default evaluation interval with a past lastEvaluated when compliant",
+			futureResourceVersion,
 			twelveSecsAgo,
 			2,
 			policyv1.EvaluationInterval{Compliant: "10s", NonCompliant: "10s"},
@@ -1225,6 +1235,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Default evaluation interval with a past lastEvaluated when noncompliant",
+			futureResourceVersion,
 			twelveSecsAgo,
 			2,
 			policyv1.EvaluationInterval{Compliant: "10s", NonCompliant: "10s"},
@@ -1238,6 +1249,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Never evaluation interval with past lastEvaluated when compliant",
+			futureResourceVersion,
 			twelveHoursAgo,
 			2,
 			policyv1.EvaluationInterval{Compliant: "never"},
@@ -1251,6 +1263,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Never evaluation interval with past lastEvaluated when noncompliant",
+			futureResourceVersion,
 			twelveHoursAgo,
 			2,
 			policyv1.EvaluationInterval{Compliant: "10s", NonCompliant: "never"},
@@ -1264,6 +1277,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Unset evaluation interval with past lastEvaluated when compliant",
+			futureResourceVersion,
 			twelveHoursAgo,
 			2,
 			policyv1.EvaluationInterval{},
@@ -1277,6 +1291,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Watch evaluation interval with past lastEvaluated when compliant",
+			futureResourceVersion,
 			twelveHoursAgo,
 			2,
 			policyv1.EvaluationInterval{Compliant: "watch", NonCompliant: "watch"},
@@ -1290,6 +1305,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Unset evaluation interval with past lastEvaluated when noncompliant",
+			futureResourceVersion,
 			twelveHoursAgo,
 			2,
 			policyv1.EvaluationInterval{},
@@ -1303,6 +1319,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Watch evaluation interval with past lastEvaluated when noncompliant",
+			futureResourceVersion,
 			twelveHoursAgo,
 			2,
 			policyv1.EvaluationInterval{NonCompliant: "watch"},
@@ -1316,6 +1333,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Invalid evaluation interval when compliant",
+			futureResourceVersion,
 			inFuture,
 			2,
 			policyv1.EvaluationInterval{Compliant: "Do or do not. There is no try."},
@@ -1329,6 +1347,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Invalid evaluation interval when noncompliant",
+			futureResourceVersion,
 			inFuture,
 			2,
 			policyv1.EvaluationInterval{NonCompliant: "Do or do not. There is no try."},
@@ -1342,6 +1361,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Custom evaluation interval that hasn't past yet when compliant",
+			futureResourceVersion,
 			twelveSecsAgo,
 			2,
 			policyv1.EvaluationInterval{Compliant: "12h"},
@@ -1355,6 +1375,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Custom evaluation interval that hasn't past yet when noncompliant",
+			futureResourceVersion,
 			twelveSecsAgo,
 			2,
 			policyv1.EvaluationInterval{NonCompliant: "12h"},
@@ -1368,6 +1389,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Deletion timestamp is non nil",
+			futureResourceVersion,
 			inFuture,
 			2,
 			policyv1.EvaluationInterval{NonCompliant: "12h"},
@@ -1381,6 +1403,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"Finalizer and the controller is being deleted",
+			futureResourceVersion,
 			inFuture,
 			2,
 			policyv1.EvaluationInterval{NonCompliant: "12h"},
@@ -1394,6 +1417,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"No finalizer and the controller is being deleted",
+			futureResourceVersion,
 			inFuture,
 			2,
 			policyv1.EvaluationInterval{NonCompliant: "12h"},
@@ -1407,6 +1431,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 		{
 			"controller-runtime cache not yet synced",
+			recentResourceVersion,
 			twelveHoursAgo,
 			2,
 			policyv1.EvaluationInterval{NonCompliant: "12h"},
@@ -1416,7 +1441,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			nil,
 			false,
 			[]string{},
-			lastEvaluatedTwelveSecsAgo,
+			lastEvaluatedInFuture,
 		},
 	}
 
@@ -1429,6 +1454,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 				policyCopy := policy.DeepCopy()
 
 				policyCopy.Status.LastEvaluated = test.lastEvaluated
+				policyCopy.SetResourceVersion(test.resourceVersion)
 				policyCopy.Status.LastEvaluatedGeneration = test.lastEvaluatedGeneration
 				policyCopy.Spec.EvaluationInterval = test.evaluationInterval
 				policyCopy.Status.ComplianceState = test.complianceState

controllers/operatorpolicy_controller.go

@@ -14,6 +14,7 @@ import (
 	"slices"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	operatorv1 "github.com/operator-framework/api/pkg/operators/v1"
@@ -119,6 +120,10 @@ type OperatorPolicyReconciler struct {
 	HubDynamicWatcher depclient.DynamicWatcher
 	HubClient         *kubernetes.Clientset
 	ClusterName       string
+	// lastEvaluatedCache contains the value of the last known OperatorPolicy resourceVersion per UID.
+	// This is a workaround to account for race conditions where the status is updated but the controller-runtime cache
+	// has not updated yet.
+	lastEvaluatedCache sync.Map
 }
 
 // SetupWithManager sets up the controller with the Manager and will reconcile when the dynamic watcher
@@ -248,6 +253,26 @@ func (r *OperatorPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return reconcile.Result{}, err
 	}
 
+	if cachedLastEval, ok := r.lastEvaluatedCache.Load(policy.UID); ok {
+		last, cachedConversionErr := strconv.Atoi(cachedLastEval.(string))
+		current, realConversionErr := strconv.Atoi(policy.GetResourceVersion())
+
+		// Note: resourceVersion should be strictly monotonic *for a specific resource instance*,
+		// but is not necessarily monotonic across different kinds and namespaces.
+		// The comparison here is for a specific resource, and so it should be a valid check.
+		if cachedConversionErr == nil && realConversionErr == nil {
+			if last > current {
+				opLog.V(1).Info("The policy from the controller-runtime cache is stale. Will requeue.")
+
+				return reconcile.Result{RequeueAfter: time.Second}, nil
+			}
+		} else {
+			opLog.Error(nil, "A resourceVersion could not be converted to an integer. Evaluating anyway.",
+				"cache", cachedLastEval, "cachedConversionErr", cachedConversionErr,
+				"real", policy.GetResourceVersion(), "realConversionErr", realConversionErr)
+		}
+	}
+
 	originalStatus := *policy.Status.DeepCopy()
 
 	// Start query batch for caching and watching related objects
@@ -308,6 +333,8 @@ func (r *OperatorPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	if statusChanged || !reflect.DeepEqual(policy.Status, originalStatus) {
 		if err := r.Status().Update(ctx, policy); err != nil {
 			errs = append(errs, err)
+		} else {
+			r.lastEvaluatedCache.Store(policy.UID, policy.GetResourceVersion())
 		}
 	}