Skip creating storages for non-stored and non-served versions #130704
Conversation
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@gmail.com>
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Welcome @GrigoriyMikhalkin! |
|
Hi @GrigoriyMikhalkin. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GrigoriyMikhalkin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/triage accepted |
|
cc @jpbetz |
|
/ok-to-test is it possible to add a test demonstrating this fixes the spurious storage issue? |
|
@liggitt Added integration test |
|
@GrigoriyMikhalkin: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| @@ -818,6 +818,11 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd | |||
| return nil, fmt.Errorf("the server could not properly serve the list kind") | |||
| } | |||
|
|
|||
| // Do not construct storage if version is neither served nor stored | |||
There was a problem hiding this comment.
I think short-circuiting at this point in the loop is correct, but it's really hard to reason about which things in this loop have side-effects we need to preserve.
I think it is just the RegisterKindFor calls, but I'm not 100% sure.
I think it would be clearer to add a loop before the storage-constructing loop, which we do not short-circuit, that does whatever validation / equivalent kind registration we want to do for all versions:
// ensure accepted names are valid
if len(crd.Status.AcceptedNames.Plural) == 0 {
utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.plural", crd.Name))
return nil, fmt.Errorf("the server could not properly serve the resource")
}
if len(crd.Status.AcceptedNames.Singular) == 0 {
utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.singular", crd.Name))
return nil, fmt.Errorf("the server could not properly serve the resource")
}
if len(crd.Status.AcceptedNames.Kind) == 0 {
utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.kind", crd.Name))
return nil, fmt.Errorf("the server could not properly serve the kind")
}
if len(crd.Status.AcceptedNames.ListKind) == 0 {
utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.listKind", crd.Name))
return nil, fmt.Errorf("the server could not properly serve the list kind")
}
var (
versionToResource = map[string]schema.GroupVersionResource{}
versionToSingularResource = map[string]schema.GroupVersionResource{}
versionToKind = map[string]schema.GroupVersionKind{}
versionToListKind = map[string]schema.GroupVersionKind{}
versionToSubresources = map[string]*apiextensionsv1.CustomResourceSubresources{}
)
// compute resource, singularResource, kind, subresources, and register equivalent kinds for all versions
for _, v := range crd.Spec.Versions {
versionToResource[v.Name] = schema.GroupVersionResource{Group: crd.Spec.Group, Version: v.Name, Resource: crd.Status.AcceptedNames.Plural}
versionToSingularResource[v.Name] = schema.GroupVersionResource{Group: crd.Spec.Group, Version: v.Name, Resource: crd.Status.AcceptedNames.Singular}
versionToKind[v.Name] = schema.GroupVersionKind{Group: crd.Spec.Group, Version: v.Name, Kind: crd.Status.AcceptedNames.Kind}
versionToListKind[v.Name] = schema.GroupVersionKind{Group: crd.Spec.Group, Version: v.Name, Kind: crd.Status.AcceptedNames.ListKind}
versionToSubresources[v.Name], err = apiextensionshelpers.GetSubresourcesForVersion(crd, v.Name)
if err != nil {
utilruntime.HandleError(err)
return nil, fmt.Errorf("the server could not properly serve the CR subresources")
}
// Register equivalent kinds
equivalentResourceRegistry.RegisterKindFor(versionToResource[v.Name], "", versionToKind[v.Name])
if versionToSubresources[v.Name] != nil {
if versionToSubresources[v.Name].Status != nil {
equivalentResourceRegistry.RegisterKindFor(versionToResource[v.Name], "status", versionToKind[v.Name])
}
if versionToSubresources[v.Name].Scale != nil {
equivalentResourceRegistry.RegisterKindFor(versionToResource[v.Name], "scale", autoscalingv1.SchemeGroupVersion.WithKind("Scale"))
}
}
}and then short-circuit this loop at the very top
+ // construct storage for served / stored versions
for _, v := range crd.Spec.Versions {
+ if !v.Storage && !v.Served {
+ continue
+ }
+
// In addition to Unstructured objects (Custom Resources), we also may sometimes need to
// decode unversioned Options objects, so we delegate to parameterScheme for such types.
parameterScheme := runtime.NewScheme()
@@ -735,22 +787,9 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
)
parameterCodec := runtime.NewParameterCodec(parameterScheme)
- resource := schema.GroupVersionResource{Group: crd.Spec.Group, Version: v.Name, Resource: crd.Status.AcceptedNames.Plural}
- if len(resource.Resource) == 0 {
- utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.plural", crd.Name))
- return nil, fmt.Errorf("the server could not properly serve the resource")
- }
- singularResource := schema.GroupVersionResource{Group: crd.Spec.Group, Version: v.Name, Resource: crd.Status.AcceptedNames.Singular}
- if len(singularResource.Resource) == 0 {
- utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.singular", crd.Name))
- return nil, fmt.Errorf("the server could not properly serve the resource")
- }
- kind := schema.GroupVersionKind{Group: crd.Spec.Group, Version: v.Name, Kind: crd.Status.AcceptedNames.Kind}
- if len(kind.Kind) == 0 {
- utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.kind", crd.Name))
- return nil, fmt.Errorf("the server could not properly serve the kind")
- }
- equivalentResourceRegistry.RegisterKindFor(resource, "", kind)
+ resource := versionToResource[v.Name]
+ singularResource := versionToSingularResource[v.Name]
+ kind := versionToKind[v.Name]
typer := newUnstructuredObjectTyper(parameterScheme)
creator := unstructuredCreator{}
@@ -776,13 +815,8 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
var statusSpec *apiextensionsinternal.CustomResourceSubresourceStatus
var statusValidator apiservervalidation.SchemaValidator
- subresources, err := apiextensionshelpers.GetSubresourcesForVersion(crd, v.Name)
- if err != nil {
- utilruntime.HandleError(err)
- return nil, fmt.Errorf("the server could not properly serve the CR subresources")
- }
+ subresources := versionToSubresources[v.Name]
if subresources != nil && subresources.Status != nil {
- equivalentResourceRegistry.RegisterKindFor(resource, "status", kind)
statusSpec = &apiextensionsinternal.CustomResourceSubresourceStatus{}
if err := apiextensionsv1.Convert_v1_CustomResourceSubresourceStatus_To_apiextensions_CustomResourceSubresourceStatus(subresources.Status, statusSpec, nil); err != nil {
return nil, fmt.Errorf("failed converting CRD status subresource to internal version: %v", err)
@@ -800,7 +834,6 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
var scaleSpec *apiextensionsinternal.CustomResourceSubresourceScale
if subresources != nil && subresources.Scale != nil {
- equivalentResourceRegistry.RegisterKindFor(resource, "scale", autoscalingv1.SchemeGroupVersion.WithKind("Scale"))
scaleSpec = &apiextensionsinternal.CustomResourceSubresourceScale{}
if err := apiextensionsv1.Convert_v1_CustomResourceSubresourceScale_To_apiextensions_CustomResourceSubresourceScale(subresources.Scale, scaleSpec, nil); err != nil {
return nil, fmt.Errorf("failed converting CRD status subresource to internal version: %v", err)
@@ -817,11 +850,7 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
klog.V(2).Infof("The CRD for %v has an invalid printer specification, falling back to default printing: %v", kind, err)
}
- listKind := schema.GroupVersionKind{Group: crd.Spec.Group, Version: v.Name, Kind: crd.Status.AcceptedNames.ListKind}
- if len(listKind.Kind) == 0 {
- utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.listKind", crd.Name))
- return nil, fmt.Errorf("the server could not properly serve the list kind")
- }
+ listKind := versionToListKind[v.Name]
storages[v.Name], err = customresource.NewStorage(
resource.GroupResource(),| // KUBE_APISERVER_SERVE_REMOVED_APIS_FOR_ONE_RELEASE allows for APIs pending removal to not block tests | ||
| t.Setenv("KUBE_APISERVER_SERVE_REMOVED_APIS_FOR_ONE_RELEASE", "true") |
There was a problem hiding this comment.
we shouldn't need this set for this test
| for _, createVersion := range ctc.crd.Spec.Versions { | ||
| if !createVersion.Storage && !createVersion.Served { | ||
| continue | ||
| } |
There was a problem hiding this comment.
| for _, createVersion := range ctc.crd.Spec.Versions { | |
| if !createVersion.Storage && !createVersion.Served { | |
| continue | |
| } | |
| for _, createVersion := range servedVersions(ctc.crd.Spec.Versions) { |
| for _, getVersion := range ctc.crd.Spec.Versions { | ||
| if !getVersion.Storage && !getVersion.Served { | ||
| continue | ||
| } |
There was a problem hiding this comment.
| for _, getVersion := range ctc.crd.Spec.Versions { | |
| if !getVersion.Storage && !getVersion.Served { | |
| continue | |
| } | |
| for _, getVersion := range servedVersions(ctc.crd.Spec.Versions) { |
| for _, createVersion := range ctc.crd.Spec.Versions { | ||
| servedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{} | ||
| for _, version := range ctc.crd.Spec.Versions { | ||
| if !version.Storage && !version.Served { |
There was a problem hiding this comment.
It seems like we should only be checking version.Served when accumulating a list named servedVersions:
| if !version.Storage && !version.Served { | |
| if !version.Served { |
maybe make this a helper function:
func servedVersions(versions []apiextensionsv1.CustomResourceDefinitionVersion) []apiextensionsv1.CustomResourceDefinitionVersionso all the places we're iterating over versions can just wrap with a call to the helper instead of adding custom loop logic (noted a couple below, but I think ~all of them could switch to that style)
| @@ -1224,6 +1348,48 @@ var multiVersionFixture = &apiextensionsv1.CustomResourceDefinition{ | |||
| }, | |||
| }, | |||
| }, | |||
| { | |||
| // same schema as v1beta1 | |||
There was a problem hiding this comment.
| // same schema as v1beta1 | |
| // same schema as v1beta1, but not served |
| } | ||
| // nolint:errcheck | ||
| defer etcdClient.Close() | ||
|
|
There was a problem hiding this comment.
we need to make sure that existing objects in etcd can still be successfully read and converted from a non-served, non-storage version, and can be updated
- create the CRD serving all versions, storing in v1alpha2
- server-side apply an object via the v1alpha2 API (will persist in v1alpha2)
- update the CRD to switch the storage version to a different version, stop serving v1alpha2, and set a conversion webhook
- ensure the conversion webhook is never asked to convert to v1alpha2 (as this test already does)
- ensure the previously created object can still be read via a different API version
- ensure the previously created object can still be updated via a different API version using server-side apply
- ensure the update does not trigger a conversion request to v1alpha2
What type of PR is this?
/kind bug
What this PR does / why we need it:
This is update on #129709.
This PR removes storage creation for non-stored and non-served versions.
Tested locally scenario proposed in #129979:
ListWatchis created only forv1alpha4andv1beta1as expected.Which issue(s) this PR fixes:
Fixes #129979
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: