From 86ffa59d340c7b8d31d7ee3655d8a83053dcd95b Mon Sep 17 00:00:00 2001 From: hzxuzhonghu Date: Fri, 5 Jan 2018 16:13:51 +0800 Subject: [PATCH 1/2] refactor customresource handler --- .../pkg/apiserver/apiserver.go | 1 - .../pkg/apiserver/customresource_handler.go | 140 ++++++++++-------- 2 files changed, 75 insertions(+), 66 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go index de90cef9068..b8af7d55595 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go @@ -177,7 +177,6 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget) versionDiscoveryHandler, groupDiscoveryHandler, s.GenericAPIServer.RequestContextMapper(), - s.Informers.Apiextensions().InternalVersion().CustomResourceDefinitions().Lister(), s.Informers.Apiextensions().InternalVersion().CustomResourceDefinitions(), delegateHandler, c.ExtraConfig.CRDRESTOptionsGetter, diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index 773a0657726..3112b9353fd 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -44,12 +44,13 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/endpoints/handlers" + "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" apirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" "k8s.io/apiserver/pkg/storage/storagebackend" "k8s.io/client-go/discovery" - cache "k8s.io/client-go/tools/cache" + "k8s.io/client-go/tools/cache" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiservervalidation "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" @@ -67,6 +68,9 @@ type crdHandler struct { customStorageLock sync.Mutex // customStorage contains a crdStorageMap + // atomic.Value has a very good read performance compared to sync.RWMutex + // see https://gist.github.com/dim/152e6bf80e1384ea72e17ac717a5000a + // which is suited for most read and rarely write cases customStorage atomic.Value requestContextMapper apirequest.RequestContextMapper @@ -96,7 +100,6 @@ func NewCustomResourceDefinitionHandler( versionDiscoveryHandler *versionDiscoveryHandler, groupDiscoveryHandler *groupDiscoveryHandler, requestContextMapper apirequest.RequestContextMapper, - crdLister listers.CustomResourceDefinitionLister, crdInformer informers.CustomResourceDefinitionInformer, delegate http.Handler, restOptionsGetter generic.RESTOptionsGetter, @@ -106,7 +109,7 @@ func NewCustomResourceDefinitionHandler( groupDiscoveryHandler: groupDiscoveryHandler, customStorage: atomic.Value{}, requestContextMapper: requestContextMapper, - crdLister: crdLister, + crdLister: crdInformer.Lister(), delegate: delegate, restOptionsGetter: restOptionsGetter, admission: admission, @@ -120,19 +123,20 @@ func NewCustomResourceDefinitionHandler( }) ret.customStorage.Store(crdStorageMap{}) + return ret } func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { ctx, ok := r.requestContextMapper.Get(req) if !ok { - // programmer error - panic("missing context") + responsewriters.InternalError(w, req, fmt.Errorf("no context found for request")) + return } requestInfo, ok := apirequest.RequestInfoFrom(ctx) if !ok { - // programmer error - panic("missing requestInfo") + responsewriters.InternalError(w, req, fmt.Errorf("no RequestInfo found in the context")) + return } if !requestInfo.IsResourceRequest { pathParts := splitPath(requestInfo.Path) @@ -168,6 +172,7 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { } if !apiextensions.IsCRDConditionTrue(crd, apiextensions.Established) { r.delegate.ServeHTTP(w, req) + return } if len(requestInfo.Subresource) > 0 { http.NotFound(w, req) @@ -176,7 +181,7 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { terminating := apiextensions.IsCRDConditionTrue(crd, apiextensions.Terminating) - crdInfo, err := r.getServingInfoFor(crd) + crdInfo, err := r.getOrCreateServingInfoFor(crd) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -242,19 +247,52 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { } } +func (r *crdHandler) updateCustomResourceDefinition(oldObj, newObj interface{}) { + oldCRD := oldObj.(*apiextensions.CustomResourceDefinition) + newCRD := newObj.(*apiextensions.CustomResourceDefinition) + + r.customStorageLock.Lock() + defer r.customStorageLock.Unlock() + + storageMap := r.customStorage.Load().(crdStorageMap) + oldInfo, found := storageMap[newCRD.UID] + if !found { + return + } + if apiequality.Semantic.DeepEqual(&newCRD.Spec, oldInfo.spec) && apiequality.Semantic.DeepEqual(&newCRD.Status.AcceptedNames, oldInfo.acceptedNames) { + glog.V(6).Infof("Ignoring customresourcedefinition %s update because neither spec, nor accepted names changed", oldCRD.Name) + return + } + + glog.V(4).Infof("Updating customresourcedefinition %s", oldCRD.Name) + + // Copy because we cannot write to storageMap without a race + // as it is used without locking elsewhere. + storageMap2 := storageMap.clone() + if oldInfo, ok := storageMap2[types.UID(oldCRD.UID)]; ok { + oldInfo.storage.DestroyFunc() + delete(storageMap2, types.UID(oldCRD.UID)) + } + + r.customStorage.Store(storageMap2) +} + // removeDeadStorage removes REST storage that isn't being used func (r *crdHandler) removeDeadStorage() { - // these don't have to be live. A snapshot is fine - // if we wrongly delete, that's ok. The rest storage will be recreated on the next request - // if we wrongly miss one, that's ok. We'll get it next time - storageMap := r.customStorage.Load().(crdStorageMap) allCustomResourceDefinitions, err := r.crdLister.List(labels.Everything()) if err != nil { utilruntime.HandleError(err) return } - for uid, s := range storageMap { + r.customStorageLock.Lock() + defer r.customStorageLock.Unlock() + + storageMap := r.customStorage.Load().(crdStorageMap) + // Copy because we cannot write to storageMap without a race + // as it is used without locking elsewhere + storageMap2 := storageMap.clone() + for uid, s := range storageMap2 { found := false for _, crd := range allCustomResourceDefinitions { if crd.UID == uid { @@ -265,38 +303,33 @@ func (r *crdHandler) removeDeadStorage() { if !found { glog.V(4).Infof("Removing dead CRD storage for %v", s.requestScope.Resource) s.storage.DestroyFunc() - delete(storageMap, uid) + delete(storageMap2, uid) } } - - r.customStorageLock.Lock() - defer r.customStorageLock.Unlock() - - r.customStorage.Store(storageMap) + r.customStorage.Store(storageMap2) } // GetCustomResourceListerCollectionDeleter returns the ListerCollectionDeleter for // the given uid, or nil if one does not exist. func (r *crdHandler) GetCustomResourceListerCollectionDeleter(crd *apiextensions.CustomResourceDefinition) finalizer.ListerCollectionDeleter { - info, err := r.getServingInfoFor(crd) + info, err := r.getOrCreateServingInfoFor(crd) if err != nil { utilruntime.HandleError(err) } return info.storage } -func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefinition) (*crdInfo, error) { +func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResourceDefinition) (*crdInfo, error) { storageMap := r.customStorage.Load().(crdStorageMap) - ret, ok := storageMap[crd.UID] - if ok { + if ret, ok := storageMap[crd.UID]; ok { return ret, nil } r.customStorageLock.Lock() defer r.customStorageLock.Unlock() - ret, ok = storageMap[crd.UID] - if ok { + storageMap = r.customStorage.Load().(crdStorageMap) + if ret, ok := storageMap[crd.UID]; ok { return ret, nil } @@ -384,7 +417,7 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti MetaGroupVersion: metav1.SchemeGroupVersion, } - ret = &crdInfo{ + ret := &crdInfo{ spec: &crd.Spec, acceptedNames: &crd.Status.AcceptedNames, @@ -392,16 +425,13 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti requestScope: requestScope, } - storageMap2 := make(crdStorageMap, len(storageMap)) - // Copy because we cannot write to storageMap without a race - // as it is used without locking elsewhere - for k, v := range storageMap { - storageMap2[k] = v - } + // as it is used without locking elsewhere. + storageMap2 := storageMap.clone() storageMap2[crd.UID] = ret r.customStorage.Store(storageMap2) + return ret, nil } @@ -423,39 +453,6 @@ func (c crdObjectConverter) ConvertFieldLabel(version, kind, label, value string } } -func (c *crdHandler) updateCustomResourceDefinition(oldObj, newObj interface{}) { - oldCRD := oldObj.(*apiextensions.CustomResourceDefinition) - newCRD := newObj.(*apiextensions.CustomResourceDefinition) - - c.customStorageLock.Lock() - defer c.customStorageLock.Unlock() - storageMap := c.customStorage.Load().(crdStorageMap) - - oldInfo, found := storageMap[newCRD.UID] - if !found { - return - } - if apiequality.Semantic.DeepEqual(&newCRD.Spec, oldInfo.spec) && apiequality.Semantic.DeepEqual(&newCRD.Status.AcceptedNames, oldInfo.acceptedNames) { - glog.V(6).Infof("Ignoring customresourcedefinition %s update because neither spec, nor accepted names changed", oldCRD.Name) - return - } - - glog.V(4).Infof("Updating customresourcedefinition %s", oldCRD.Name) - storageMap2 := make(crdStorageMap, len(storageMap)) - - // Copy because we cannot write to storageMap without a race - // as it is used without locking elsewhere - for k, v := range storageMap { - if k == oldCRD.UID { - v.storage.DestroyFunc() - continue - } - storageMap2[k] = v - } - - c.customStorage.Store(storageMap2) -} - type unstructuredNegotiatedSerializer struct { typer runtime.ObjectTyper creator runtime.ObjectCreater @@ -578,3 +575,16 @@ func (t CRDRESTOptionsGetter) GetRESTOptions(resource schema.GroupResource) (gen } return ret, nil } + +// clone returns a clone of the provided crdStorageMap. +// The clone is a shallow copy of the map. +func (in crdStorageMap) clone() crdStorageMap { + if in == nil { + return nil + } + out := make(crdStorageMap, len(in)) + for key, value := range in { + out[key] = value + } + return out +} From e7530405456daafd014cb8e7702d5ce177dbf9e7 Mon Sep 17 00:00:00 2001 From: hzxuzhonghu Date: Fri, 5 Jan 2018 16:14:00 +0800 Subject: [PATCH 2/2] run update bazel and staging-godep --- staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json | 4 ++++ .../src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD | 1 + 2 files changed, 5 insertions(+) diff --git a/staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json b/staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json index 08607c1ddcf..b00cf3779c8 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json +++ b/staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json @@ -1790,6 +1790,10 @@ "ImportPath": "k8s.io/apiserver/pkg/endpoints/handlers", "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" }, + { + "ImportPath": "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters", + "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + }, { "ImportPath": "k8s.io/apiserver/pkg/endpoints/request", "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD index a3842388097..dbed4134690 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD @@ -54,6 +54,7 @@ go_library( "//vendor/k8s.io/apiserver/pkg/admission:go_default_library", "//vendor/k8s.io/apiserver/pkg/endpoints/discovery:go_default_library", "//vendor/k8s.io/apiserver/pkg/endpoints/handlers:go_default_library", + "//vendor/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters:go_default_library", "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//vendor/k8s.io/apiserver/pkg/registry/generic:go_default_library", "//vendor/k8s.io/apiserver/pkg/registry/generic/registry:go_default_library",