From 3807cc1a01619ddf857009c72ca1712746e8fc01 Mon Sep 17 00:00:00 2001 From: Nick Hale Date: Tue, 5 Apr 2022 11:57:55 -0400 Subject: [PATCH] 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 --- .../operators/operator_controller.go | 55 +------------------ test/e2e/operator_test.go | 2 +- 2 files changed, 3 insertions(+), 54 deletions(-) diff --git a/pkg/controller/operators/operator_controller.go b/pkg/controller/operators/operator_controller.go index e39ecb7d9a..a8e6cab804 100644 --- a/pkg/controller/operators/operator_controller.go +++ b/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}) } diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index faa5acac66..cbb29862ae 100644 --- a/test/e2e/operator_test.go +++ b/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-"))