Merge pull request #53586 from sttts/sttts-storage-shutdown

Automatic merge from submit-queue (batch tested with PRs 53249, 53586). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

apiextensions-apiserver: stop cacher on CRD update

Potentially fixes #53485

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD

@@ -33,6 +33,7 @@ go_library(
         "//vendor/k8s.io/apiextensions-apiserver/pkg/controller/status:go_default_library",
         "//vendor/k8s.io/apiextensions-apiserver/pkg/registry/customresource:go_default_library",
         "//vendor/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apimachinery/announced:go_default_library",

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -30,6 +30,7 @@ import (
 	"github.com/go-openapi/validate"
 	"github.com/golang/glog"
 
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -79,6 +80,11 @@ type crdHandler struct {
 
 // crdInfo stores enough information to serve the storage for the custom resource
 type crdInfo struct {
+	// spec and acceptedNames are used to compare against if a change is made on a CRD. We only update
+	// the storage if one of these changes.
+	spec          *apiextensions.CustomResourceDefinitionSpec
+	acceptedNames *apiextensions.CustomResourceDefinitionNames
+
 	storage      *customresource.REST
 	requestScope handlers.RequestScope
 }
@@ -108,6 +114,9 @@ func NewCustomResourceDefinitionHandler(
 
 	crdInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
 		UpdateFunc: ret.updateCustomResourceDefinition,
+		DeleteFunc: func(obj interface{}) {
+			ret.removeDeadStorage()
+		},
 	})
 
 	ret.customStorage.Store(crdStorageMap{})
@@ -245,7 +254,7 @@ func (r *crdHandler) removeDeadStorage() {
 		return
 	}
 
-	for uid := range storageMap {
+	for uid, s := range storageMap {
 		found := false
 		for _, crd := range allCustomResourceDefinitions {
 			if crd.UID == uid {
@@ -254,6 +263,8 @@ 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)
 		}
 	}
@@ -301,7 +312,7 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti
 	parameterScheme.AddGeneratedDeepCopyFuncs(metav1.GetGeneratedDeepCopyFuncs()...)
 	parameterCodec := runtime.NewParameterCodec(parameterScheme)
 
-	kind := schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Spec.Names.Kind}
+	kind := schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Status.AcceptedNames.Kind}
 	typer := unstructuredObjectTyper{
 		delegate:          parameterScheme,
 		unstructuredTyper: discovery.NewUnstructuredObjectTyper(nil),
@@ -319,8 +330,8 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti
 	validator := validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default)
 
 	storage := customresource.NewREST(
-		schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Spec.Names.Plural},
+		schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Status.AcceptedNames.Plural},
-		schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Spec.Names.ListKind},
+		schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Status.AcceptedNames.ListKind},
 		customresource.NewStrategy(
 			typer,
 			crd.Spec.Scope == apiextensions.NamespaceScoped,
@@ -367,7 +378,7 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti
 		Typer:           typer,
 		UnsafeConvertor: unstructured.UnstructuredObjectConverter{},
 
-		Resource:    schema.GroupVersionResource{Group: crd.Spec.Group, Version: crd.Spec.Version, Resource: crd.Spec.Names.Plural},
+		Resource:    schema.GroupVersionResource{Group: crd.Spec.Group, Version: crd.Spec.Version, Resource: crd.Status.AcceptedNames.Plural},
 		Kind:        kind,
 		Subresource: "",
 
@@ -375,6 +386,9 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti
 	}
 
 	ret = &crdInfo{
+		spec:          &crd.Spec,
+		acceptedNames: &crd.Status.AcceptedNames,
+
 		storage:      storage,
 		requestScope: requestScope,
 	}
@@ -410,23 +424,36 @@ func (c crdObjectConverter) ConvertFieldLabel(version, kind, label, value string
 	}
 }
 
-func (c *crdHandler) updateCustomResourceDefinition(oldObj, _ interface{}) {
+func (c *crdHandler) updateCustomResourceDefinition(oldObj, newObj interface{}) {
 	oldCRD := oldObj.(*apiextensions.CustomResourceDefinition)
-	glog.V(4).Infof("Updating customresourcedefinition %s", oldCRD.Name)
+	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
 	}
 
-	delete(storageMap2, oldCRD.UID)
 	c.customStorage.Store(storageMap2)
 }
 

staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 	"time"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -379,15 +380,18 @@ func TestCRValidationOnCRDUpdate(t *testing.T) {
 
 	// update the CRD to a less stricter schema
 	gottenCRD.Spec.Validation.OpenAPIV3Schema.Required = []string{"alpha", "beta"}
-
+	if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(gottenCRD); err != nil {
-	updatedCRD, err := apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(gottenCRD)
-	if err != nil {
 		t.Fatal(err)
 	}
 
 	// CR is now accepted
 	err = wait.Poll(500*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {
-		_, err = instantiateCustomResource(t, newNoxuValidationInstance(ns, "foo"), noxuResourceClient, updatedCRD)
+		_, err := noxuResourceClient.Create(newNoxuValidationInstance(ns, "foo"))
+		if statusError, isStatus := err.(*apierrors.StatusError); isStatus {
+			if strings.Contains(statusError.Error(), "is invalid") {
+				return false, nil
+			}
+		}
 		if err != nil {
 			return false, err
 		}