From 17e48e7cfcf60fe008ba2638edaea47608267af9 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 Upstream-repository: operator-lifecycle-manager Upstream-commit: 3807cc1a01619ddf857009c72ca1712746e8fc01 --- .../operators/operator_controller.go | 55 +------------------ .../test/e2e/operator_test.go | 1 + .../operators/operator_controller.go | 55 +------------------ 3 files changed, 5 insertions(+), 106 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go b/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go index 62aa4e2d06..d20761b8cc 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go +++ b/staging/operator-lifecycle-manager/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" @@ -50,10 +48,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 @@ -105,9 +99,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 } @@ -139,16 +132,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 { @@ -174,11 +157,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 } @@ -242,33 +220,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 { @@ -277,8 +228,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/staging/operator-lifecycle-manager/test/e2e/operator_test.go b/staging/operator-lifecycle-manager/test/e2e/operator_test.go index 219b5f9f3e..bd943bc947 100644 --- a/staging/operator-lifecycle-manager/test/e2e/operator_test.go +++ b/staging/operator-lifecycle-manager/test/e2e/operator_test.go @@ -70,6 +70,7 @@ var _ = Describe("Operator API", func() { // 14. Ensure the reference to ns-a is eventually removed from o's status.components.refs field // 15. Delete o // 16. Ensure o is not re-created + // issue: https://github.com/operator-framework/operator-lifecycle-manager/issues/2628 It("should surface components in its status", func() { o := &operatorsv1.Operator{} o.SetName(genName("o-")) diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go index 62aa4e2d06..d20761b8cc 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/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" @@ -50,10 +48,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 @@ -105,9 +99,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 } @@ -139,16 +132,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 { @@ -174,11 +157,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 } @@ -242,33 +220,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 { @@ -277,8 +228,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}) }