fix(operator): remove broken thread-safety (#2697)

The thread-safety logic is flawed and can prevent Operator resources
from being reconciled when component resources are updated. It also
ensures idempotence when enqueuing a resource for reconciliation; In
other words, enqueuing no-ops if the given resource is already in the queue.

The underlying queue will ensure a reconciler never races itself when
processing a given resource event.

Signed-off-by: Nick Hale <njohnhale@gmail.com>

pkg/controller/operators/operator_controller.go

@@ -3,7 +3,6 @@ package operators
 import (
 	"context"
 	"fmt"
-	"sync"
 
 	"github.com/go-logr/logr"
 	appsv1 "k8s.io/api/apps/v1"
@@ -14,7 +13,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
 	kscheme "k8s.io/client-go/kubernetes/scheme"
 	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -51,10 +49,6 @@ type OperatorReconciler struct {
 
 	log     logr.Logger
 	factory decorators.OperatorFactory
-
-	// last observed resourceVersion for known Operators
-	lastResourceVersion map[types.NamespacedName]string
-	mu                  sync.RWMutex
 }
 
 // +kubebuilder:rbac:groups=operators.coreos.com,resources=operators,verbs=create;update;patch;delete
@@ -106,9 +100,8 @@ func NewOperatorReconciler(cli client.Client, log logr.Logger, scheme *runtime.S
 	return &OperatorReconciler{
 		Client: cli,
 
-		log:                 log,
-		factory:             factory,
-		lastResourceVersion: map[types.NamespacedName]string{},
+		log:     log,
+		factory: factory,
 	}, nil
 }
 
@@ -141,16 +134,6 @@ func (r *OperatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		}
 	}
 
-	rv, ok := r.getLastResourceVersion(req.NamespacedName)
-	if !create && ok && rv == in.ResourceVersion {
-		log.V(1).Info("Operator is already up-to-date")
-		return reconcile.Result{}, nil
-	}
-
-	// Set the cached resource version to 0 so we can handle
-	// the race with requests enqueuing via mapComponentRequests
-	r.setLastResourceVersion(req.NamespacedName, "0")
-
 	// Wrap with convenience decorator
 	operator, err := r.factory.NewOperator(in)
 	if err != nil {
@@ -175,11 +158,6 @@ func (r *OperatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		}
 	}
 
-	// Only set the resource version if it already exists.
-	// If it does not exist, it means mapComponentRequests was called
-	// while we were reconciling and we need to reconcile again
-	r.setLastResourceVersionIfExists(req.NamespacedName, operator.GetResourceVersion())
-
 	return ctrl.Result{}, nil
 }
 
@@ -243,33 +221,6 @@ func (r *OperatorReconciler) hasExistingComponents(ctx context.Context, name str
 	return false, nil
 }
 
-func (r *OperatorReconciler) getLastResourceVersion(name types.NamespacedName) (string, bool) {
-	r.mu.RLock()
-	defer r.mu.RUnlock()
-	rv, ok := r.lastResourceVersion[name]
-	return rv, ok
-}
-
-func (r *OperatorReconciler) setLastResourceVersion(name types.NamespacedName, rv string) {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-	r.lastResourceVersion[name] = rv
-}
-
-func (r *OperatorReconciler) setLastResourceVersionIfExists(name types.NamespacedName, rv string) {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-	if _, ok := r.lastResourceVersion[name]; ok {
-		r.lastResourceVersion[name] = rv
-	}
-}
-
-func (r *OperatorReconciler) unsetLastResourceVersion(name types.NamespacedName) {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-	delete(r.lastResourceVersion, name)
-}
-
 func (r *OperatorReconciler) mapComponentRequests(obj client.Object) []reconcile.Request {
 	var requests []reconcile.Request
 	if obj == nil {
@@ -278,8 +229,6 @@ func (r *OperatorReconciler) mapComponentRequests(obj client.Object) []reconcile
 
 	labels := decorators.OperatorNames(obj.GetLabels())
 	for _, name := range labels {
-		// unset the last recorded resource version so the Operator will reconcile
-		r.unsetLastResourceVersion(name)
 		requests = append(requests, reconcile.Request{NamespacedName: name})
 	}
 

test/e2e/operator_test.go

@@ -71,7 +71,7 @@ var _ = Describe("Operator API", func() {
 	// 15. Delete o
 	// 16. Ensure o is not re-created
 	// issue: https://github.com/operator-framework/operator-lifecycle-manager/issues/2628
-	It("[FLAKE] should surface components in its status", func() {
+	It("should surface components in its status", func() {
 		o := &operatorsv1.Operator{}
 		o.SetName(genName("o-"))