Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix lazy OpenAPI V2 CRD Controller #118808

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Fix lazy OpenAPI V2 CRD Controller #118808

merged 1 commit into from
Jul 18, 2023
+639 −75

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Jun 21, 2023 •
edited by sttts

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Make OpenAPI V2 CRD controller lazy.

Generation is not updated in the fake client so some finessing had to be done to show that CRDs have a diff. An integration test was added to show that generation is updated and updates to CRDs will update the OpenAPI.

This also changes the order of merging. group-version CRDs are first merged based on CR, then with the rest of the specs to get the merged spec. The behavior before was that every group-version CR was merged separately to get the merged spec.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Revised OpenAPI v2 fetching for CustomResourceDefinitions. CRDs are now aggregated lazily,
which improves resource usage during installation of many CRDs. As a result, the first request
to fetch the OpenAPI may be slower.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/assign @apelisse @sttts
/triage accepted

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 21, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jun 21, 2023
@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 21, 2023
@Jefftree Jefftree force-pushed the updated-lazy-crd-controller-v2 branch from 8283f44 to 4b62698 Compare June 21, 2023 21:46
@k8s-ci-robot k8s-ci-robot added area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 21, 2023
@Jefftree
Copy link
Member Author

Test failures look related, looking into them

@apelisse
Copy link
Member

apelisse commented Jul 6, 2023

I wish we had kept the renames separated in another PR for reviewability. :-)

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 6, 2023
@Jefftree Jefftree force-pushed the updated-lazy-crd-controller-v2 branch 2 times, most recently from 406b4b4 to 89ba8d3 Compare July 7, 2023 17:40
apelisse
apelisse reviewed Jul 10, 2023
View reviewed changes
staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/controller.go Outdated
@@ -269,3 +291,7 @@ func (c *Controller) deleteCustomResourceDefinition(obj interface{}) {
func (c *Controller) enqueue(obj *apiextensionsv1.CustomResourceDefinition) {
c.queue.Add(obj.Name)
}

func generateCRDHash(crd *apiextensionsv1.CustomResourceDefinition) string {
return fmt.Sprintf("%s,%d", crd.CreationTimestamp.String(), crd.Generation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use the resource UID?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik, resource UID is changed on updates to the status while we only care about the spec so we may be regenerating more frequently that we want?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, metadata.UID is unique for each resource, and only changes when a resource is recreated. It allows to identify a specific resource beyond it's gvknn.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I mixed that with resourceVersion. This looks cleaner than using the timestamp, thanks!

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/controller.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/controller.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/controller.go Show resolved Hide resolved
@Jefftree Jefftree force-pushed the updated-lazy-crd-controller-v2 branch from b40fc7f to 9cc12c6 Compare July 10, 2023 23:47
@Jefftree
Copy link
Member Author

/test pull-kubernetes-integration
seems to be a flake

@Jefftree
Copy link
Member Author

/retest
(CI issue)

@Jefftree
Copy link
Member Author

/retest
Unrelated flake

@Jefftree Jefftree force-pushed the updated-lazy-crd-controller-v2 branch from 1a1ff04 to 8edd8b7 Compare July 18, 2023 04:49
@Jefftree Jefftree force-pushed the updated-lazy-crd-controller-v2 branch from 8edd8b7 to 735be02 Compare July 18, 2023 04:50
@Jefftree
Copy link
Member Author

Rebased and squashed

@Jefftree Jefftree changed the title Lazy OpenAPI V2 CRD Controller Fix lazy OpenAPI V2 CRD Controller Jul 18, 2023
@apelisse
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 40dbc7a074785b32260179aa41f1f76424f47acc

@sttts
Copy link
Contributor

sttts commented Jul 18, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, Jefftree, sttts

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2023
@sttts sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jul 18, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 18, 2023
@k8s-ci-robot k8s-ci-robot merged commit 7aa4e08 into kubernetes:master Jul 18, 2023
13 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jul 18, 2023
@sftim
Copy link
Contributor

sftim commented Jul 19, 2023

Changelog suggestion

Revised OpenAPI v2 fetching for CustomResourceDefinitions. CRDs are now aggregated lazily,
which improves resource usage during installation of many CRDs. As a result, the first request
to fetch the OpenAPI may be slower.

@sftim
Copy link
Contributor

sftim commented Jul 19, 2023

@stts do you prefer that changelog entry per my suggestion?

@sttts
Copy link
Contributor

sttts commented Jul 19, 2023

@sftim yes, thank you. Updated.

@Jefftree Jefftree deleted the updated-lazy-crd-controller-v2 branch July 31, 2023 14:02
@Jefftree Jefftree mentioned this pull request Nov 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@apelisse apelisse apelisse left review comments

@SataQiu SataQiu Awaiting requested review from SataQiu

@yue9944882 yue9944882 Awaiting requested review from yue9944882

Assignees

@sttts sttts

@apelisse apelisse

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Jefftree @apelisse @sttts @k8s-ci-robot @sftim