From 4855917bc3b4c763722d16b2b962f5469a5fbe61 Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Fri, 4 Nov 2016 13:49:17 -0400 Subject: [PATCH] Fix possible race in operationNotSupportedCache Because we can run multiple workers to delete namespaces simultaneously, the operationNotSupportedCache needs to be guarded with a mutex to avoid concurrent map read/write errors. --- .../namespace/namespace_controller.go | 10 +++--- .../namespace/namespace_controller_test.go | 4 +-- .../namespace/namespace_controller_utils.go | 34 +++++++++++++------ 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/pkg/controller/namespace/namespace_controller.go b/pkg/controller/namespace/namespace_controller.go index f1fe663add1..d5111190535 100644 --- a/pkg/controller/namespace/namespace_controller.go +++ b/pkg/controller/namespace/namespace_controller.go @@ -50,7 +50,7 @@ type NamespaceController struct { // list of preferred group versions and their corresponding resource set for namespace deletion groupVersionResources []unversioned.GroupVersionResource // opCache is a cache to remember if a particular operation is not supported to aid dynamic client. - opCache operationNotSupportedCache + opCache *operationNotSupportedCache // finalizerToken is the finalizer token managed by this controller finalizerToken api.FinalizerName } @@ -70,13 +70,15 @@ func NewNamespaceController( // we found in practice though that some auth engines when encountering paths they don't know about may return a 50x. // until we have verbs, we pre-populate resources that do not support list or delete for well-known apis rather than // probing the server once in order to be told no. - opCache := operationNotSupportedCache{} + opCache := &operationNotSupportedCache{ + m: make(map[operationKey]bool), + } ignoredGroupVersionResources := []unversioned.GroupVersionResource{ {Group: "", Version: "v1", Resource: "bindings"}, } for _, ignoredGroupVersionResource := range ignoredGroupVersionResources { - opCache[operationKey{op: operationDeleteCollection, gvr: ignoredGroupVersionResource}] = true - opCache[operationKey{op: operationList, gvr: ignoredGroupVersionResource}] = true + opCache.setNotSupported(operationKey{op: operationDeleteCollection, gvr: ignoredGroupVersionResource}) + opCache.setNotSupported(operationKey{op: operationList, gvr: ignoredGroupVersionResource}) } // create the controller so we can inject the enqueue function diff --git a/pkg/controller/namespace/namespace_controller_test.go b/pkg/controller/namespace/namespace_controller_test.go index 0bfb071eacd..a278fba5c0d 100644 --- a/pkg/controller/namespace/namespace_controller_test.go +++ b/pkg/controller/namespace/namespace_controller_test.go @@ -158,7 +158,7 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *unversioned.APIV mockClient := fake.NewSimpleClientset(testInput.testNamespace) clientPool := dynamic.NewClientPool(clientConfig, registered.RESTMapper(), dynamic.LegacyAPIPathResolverFunc) - err := syncNamespace(mockClient, clientPool, operationNotSupportedCache{}, groupVersionResources, testInput.testNamespace, api.FinalizerKubernetes) + err := syncNamespace(mockClient, clientPool, &operationNotSupportedCache{m: make(map[operationKey]bool)}, groupVersionResources, testInput.testNamespace, api.FinalizerKubernetes) if err != nil { t.Errorf("scenario %s - Unexpected error when synching namespace %v", scenario, err) } @@ -227,7 +227,7 @@ func TestSyncNamespaceThatIsActive(t *testing.T) { Phase: api.NamespaceActive, }, } - err := syncNamespace(mockClient, nil, operationNotSupportedCache{}, testGroupVersionResources(), testNamespace, api.FinalizerKubernetes) + err := syncNamespace(mockClient, nil, &operationNotSupportedCache{m: make(map[operationKey]bool)}, testGroupVersionResources(), testNamespace, api.FinalizerKubernetes) if err != nil { t.Errorf("Unexpected error when synching namespace %v", err) } diff --git a/pkg/controller/namespace/namespace_controller_utils.go b/pkg/controller/namespace/namespace_controller_utils.go index 97063dc2d24..f6bffd58bde 100644 --- a/pkg/controller/namespace/namespace_controller_utils.go +++ b/pkg/controller/namespace/namespace_controller_utils.go @@ -19,6 +19,7 @@ package namespace import ( "fmt" "sort" + "sync" "time" "k8s.io/kubernetes/pkg/api" @@ -60,11 +61,22 @@ type operationKey struct { // operationNotSupportedCache is a simple cache to remember if an operation is not supported for a resource. // if the operationKey maps to true, it means the operation is not supported. -type operationNotSupportedCache map[operationKey]bool +type operationNotSupportedCache struct { + lock sync.RWMutex + m map[operationKey]bool +} // isSupported returns true if the operation is supported -func (o operationNotSupportedCache) isSupported(key operationKey) bool { - return !o[key] +func (o *operationNotSupportedCache) isSupported(key operationKey) bool { + o.lock.RLock() + defer o.lock.RUnlock() + return !o.m[key] +} + +func (o *operationNotSupportedCache) setNotSupported(key operationKey) { + o.lock.Lock() + defer o.lock.Unlock() + o.m[key] = true } // updateNamespaceFunc is a function that makes an update to a namespace @@ -148,7 +160,7 @@ func finalizeNamespace(kubeClient clientset.Interface, namespace *api.Namespace, // it returns an error if the operation was supported on the server but was unable to complete. func deleteCollection( dynamicClient *dynamic.Client, - opCache operationNotSupportedCache, + opCache *operationNotSupportedCache, gvr unversioned.GroupVersionResource, namespace string, ) (bool, error) { @@ -180,7 +192,7 @@ func deleteCollection( // remember next time that this resource does not support delete collection... if errors.IsMethodNotSupported(err) || errors.IsNotFound(err) { glog.V(5).Infof("namespace controller - deleteCollection not supported - namespace: %s, gvr: %v", namespace, gvr) - opCache[key] = true + opCache.setNotSupported(key) return false, nil } @@ -195,7 +207,7 @@ func deleteCollection( // an error if the operation is supported but could not be completed. func listCollection( dynamicClient *dynamic.Client, - opCache operationNotSupportedCache, + opCache *operationNotSupportedCache, gvr unversioned.GroupVersionResource, namespace string, ) (*runtime.UnstructuredList, bool, error) { @@ -225,7 +237,7 @@ func listCollection( // remember next time that this resource does not support delete collection... if errors.IsMethodNotSupported(err) || errors.IsNotFound(err) { glog.V(5).Infof("namespace controller - listCollection not supported - namespace: %s, gvr: %v", namespace, gvr) - opCache[key] = true + opCache.setNotSupported(key) return nil, false, nil } @@ -235,7 +247,7 @@ func listCollection( // deleteEachItem is a helper function that will list the collection of resources and delete each item 1 by 1. func deleteEachItem( dynamicClient *dynamic.Client, - opCache operationNotSupportedCache, + opCache *operationNotSupportedCache, gvr unversioned.GroupVersionResource, namespace string, ) error { @@ -263,7 +275,7 @@ func deleteEachItem( func deleteAllContentForGroupVersionResource( kubeClient clientset.Interface, clientPool dynamic.ClientPool, - opCache operationNotSupportedCache, + opCache *operationNotSupportedCache, gvr unversioned.GroupVersionResource, namespace string, namespaceDeletedAt unversioned.Time, @@ -331,7 +343,7 @@ func deleteAllContentForGroupVersionResource( func deleteAllContent( kubeClient clientset.Interface, clientPool dynamic.ClientPool, - opCache operationNotSupportedCache, + opCache *operationNotSupportedCache, groupVersionResources []unversioned.GroupVersionResource, namespace string, namespaceDeletedAt unversioned.Time, @@ -358,7 +370,7 @@ func deleteAllContent( func syncNamespace( kubeClient clientset.Interface, clientPool dynamic.ClientPool, - opCache operationNotSupportedCache, + opCache *operationNotSupportedCache, groupVersionResources []unversioned.GroupVersionResource, namespace *api.Namespace, finalizerToken api.FinalizerName,