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

Skip building openapi for ignored paths #66512

Merged
merged 1 commit into from Aug 7, 2018

Conversation

@jennybuckley
Contributor

jennybuckley commented Jul 23, 2018

alternative to #66286

/kind bug
Fixes #66285

NONE
@neolit123

This comment has been minimized.

Show comment
Hide comment
Member

neolit123 commented Jul 23, 2018

@tallclair tallclair removed their request for review Jul 25, 2018

@tamalsaha

This comment has been minimized.

Show comment
Hide comment
@tamalsaha

tamalsaha Jul 25, 2018

Member

/lgtm

Member

tamalsaha commented Jul 25, 2018

/lgtm

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Jul 25, 2018

Contributor

/retest

Contributor

jennybuckley commented Jul 25, 2018

/retest

if a.group.OpenAPIConfig == nil {
return nil, nil
}
pathsToIgnore := openapiutil.NewTrie(a.group.OpenAPIConfig.IgnorePrefixes)

This comment has been minimized.

@mbohlool

mbohlool Jul 29, 2018

Member

Creating a Tri and use it only once look like a waste of space.

@mbohlool

mbohlool Jul 29, 2018

Member

Creating a Tri and use it only once look like a waste of space.

This comment has been minimized.

@jennybuckley

jennybuckley Jul 30, 2018

Contributor

I think it would be garbage collected once this function finishes, right?

@jennybuckley

jennybuckley Jul 30, 2018

Contributor

I think it would be garbage collected once this function finishes, right?

This comment has been minimized.

@lavalamp

lavalamp Aug 6, 2018

Member

If the Trie is < 1GB I think it's fine :)

@lavalamp

lavalamp Aug 6, 2018

Member

If the Trie is < 1GB I think it's fine :)

This comment has been minimized.

@lavalamp

lavalamp Aug 6, 2018

Member

Even if not, is there a better way to do this? What else is the CPU going to do with those cycles anyway? :)

@lavalamp

lavalamp Aug 6, 2018

Member

Even if not, is there a better way to do this? What else is the CPU going to do with those cycles anyway? :)

This comment has been minimized.

@lavalamp

lavalamp Aug 6, 2018

Member

It might make sense to build this only once, when APIInstaller is constructed. But I wouldn't bother with that unless a profiler shows this is a hot spot.

@lavalamp

lavalamp Aug 6, 2018

Member

It might make sense to build this only once, when APIInstaller is constructed. But I wouldn't bother with that unless a profiler shows this is a hot spot.

This comment has been minimized.

@jennybuckley

jennybuckley Aug 8, 2018

Contributor

I'm not sure we even use OpenAPIConfig.IgnorePrefixes in the kube apiserver, since things were working fine in kube apiserver without this PR. I think maybe some extension apiservers might use it, but I'm not sure how extensively.

@jennybuckley

jennybuckley Aug 8, 2018

Contributor

I'm not sure we even use OpenAPIConfig.IgnorePrefixes in the kube apiserver, since things were working fine in kube apiserver without this PR. I think maybe some extension apiservers might use it, but I'm not sure how extensively.

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Jul 30, 2018

Contributor

/retest

Contributor

jennybuckley commented Jul 30, 2018

/retest

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Aug 2, 2018

Contributor

@liggitt is this in the area you found that was burning CPU on server start?

Contributor

deads2k commented Aug 2, 2018

@liggitt is this in the area you found that was burning CPU on server start?

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Aug 6, 2018

Contributor

@deads2k yes this is related. It isn't addressing that issue, but since this PR enables skipping openapi generation for some resources, it should only be possible for this PR to reduce the number of objects allocated.

(I think the issue you're talking about is #66368)

Contributor

jennybuckley commented Aug 6, 2018

@deads2k yes this is related. It isn't addressing that issue, but since this PR enables skipping openapi generation for some resources, it should only be possible for this PR to reduce the number of objects allocated.

(I think the issue you're talking about is #66368)

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Aug 6, 2018

Member

/lgtm
/approve

Member

lavalamp commented Aug 6, 2018

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 6, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jennybuckley, lavalamp, tamalsaha

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

Contributor

k8s-ci-robot commented Aug 6, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jennybuckley, lavalamp, tamalsaha

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

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Aug 6, 2018

Contributor

/retest

Contributor

jennybuckley commented Aug 6, 2018

/retest

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 7, 2018

Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

Contributor

k8s-merge-robot commented Aug 7, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 7, 2018

Contributor

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Contributor

k8s-merge-robot commented Aug 7, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 235badb into kubernetes:master Aug 7, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation jennybuckley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment