New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove warning log for crd merging #109880
Conversation
I still would not have expected duplicates... are we sure we want to ignore this and stomp schemas? Is this related to #109275? |
@liggitt: This is for CRD merging, not merging between all api resources. Between CRDs, things like ObjectMeta, etc are shared and should be safe for merging since they'll all be the same version served from the same apiextensions server. I don't think we'll run into any conflicts here? The v2 code assumes conflicts are ignored as well. https://github.com/Jefftree/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go#L63 The issue you linked is around merging between built-ins and CRDs in which conflicts may occur and should be a separate issue |
Why would merging built-ins and CRDs would conflict?! |
I'm not sure what merging is happening, but shouldn't we successfully identify metadata types in the same process as identical and combine them rather than see them as duplicates? |
Yes. That's for solving #109275. This PR only tackles the merging between CRDs which should not have any conflicts (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go#L63).
Sorry I mean merging between built-ins, CRDs and aggregated apiservers. I think we may have conflicts between merging between aggregated apiservers if there is version skew AND a resource that both apiservers depend on changes between versions. We should check if the resources are identical before creating a duplicate _v2 type in the merging process for that (OpenAPI V2 specific), so +1 to Jordan's comment. That's outside the scope of this PR though |
if we have 2 CRDs defined in the same group/version, are we merging two identical sets of metadata type definitions? if so, comments like this are either wrong or confusing:
rather than simply skipping or stomping, this should be more principled... some possibilities are:
this also needs to add tests for scenarios that could lead to duplicate types being encountered:
|
This is sort of the approach I have gone with. I'm not sure if we need to list every single type that could conflict and opted to just look at the group version for meta.v1 and autoscaling.v1. I've added unit tests and am still looking into integration tests to properly create CRDs with Scale and Status subresources. Once #109275 is fixed, we could probably look to change the CRD merging to also use the proper merge code implemented by the fix |
Updated, PTAL @liggitt |
/lgtm |
thanks, please pick to release-1.24 as well |
Thanks! |
// conflicts between user-defined CRDs | ||
mergeSpecV3(crdSpec, s) | ||
err := mergeSpecV3(crdSpec, s) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
erroring out rather than warn seems great, as we would catch these things earlier.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jefftree, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…9880-upstream-release-1.24 Automated cherry pick of #109880: Remove warning log for merging meta and scale type
What type of PR is this?
/kind bug
What this PR does / why we need it:
Remove warning log for merging in OpenAPI V3. There should be no conflicts when merging CRDs since the naming controller would prevent it. (https://github.com/Jefftree/kubernetes/blob/86da34b54a6678a404098569a424c1932f427411/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go#L104) Referenced resources like ObjectMeta should be on the same version since the apiextensions server serves all CRDs. (https://github.com/Jefftree/kubernetes/blob/86da34b54a6678a404098569a424c1932f427411/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go#L95-L96)
Which issue(s) this PR fixes:
Fixes #109871
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: