Move ResourceQuota admission to k8s.io/apiserver lib #93537

timuthy · 2020-07-29T15:18:27Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR moves the generic parts of the ResourceQuota admission plugin to /staging/src/k8s.io/apiserver, so that Extension API server can use the admission plugin without the necessity of having a dependency to k8s.io/kubernetes. It follows the same principle which is already in place for the webhook admission plugin and is a consequent enhancement of enabling custom or extended resource, tackled by #72384.

Special notes for your reviewer:

Test file plugin/pkg/admission/resourcequota/admission_test.go remains as it contains core related test cases, only generic parts were moved to staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/admission_test.go.
Test file staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/resource_access_test.go was added to cover the LRUCacheLookup use case.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2020-07-29T15:18:30Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2020-07-29T15:18:35Z

Welcome @timuthy!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-07-29T15:18:35Z

Hi @timuthy. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

yliaog · 2020-07-30T21:43:18Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/resource_access_test.go

+limitations under the License.
+*/
+
+package resourcequota


is this file moved from other place? it shows as an add here, which makes it hard to see the diff

This file was indeed added because previously, the LRU lookup was tested in /kubernetes/plugin/pkg/admission/resourcequota/admission_test.go but due to the move out and the inaccessibility of the struct quotaAccessor, I added the LRU lookup test case to this new test file.

in the admission_test.go, the test case is TestAdmitPodInNamespaceWithoutQuota? what about adding the access to quotaAccessor just for the testing purpose? then you may keep the test case there, instead of adding a new one here.

ok. so this is testing the cache hit case, probably better to add a cache miss case too.

staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/admission_test.go

yliaog · 2020-07-30T22:05:04Z

it's fine to move to staging. trying to understand how is the library going to be used? in a standalone extension apiserver?

timuthy · 2020-07-31T06:14:37Z

it's fine to move to staging. trying to understand how is the library going to be used? in a standalone extension apiserver?

Yes, in an extension apiserver. You can have a look at this RP if you are interested in more details. After the move out, this PR can only refer to common parts in k8s.io/apiserver instead of having a dependency to k8s.io/kubernetes.

timuthy · 2020-07-31T11:18:42Z

/retest

pkg/controller/resourcequota/resource_quota_monitor.go

plugin/pkg/admission/resourcequota/BUILD

test/integration/quota/quota_test.go

staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/admission_test.go

yliaog · 2020-08-01T00:01:47Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/resource_access_test.go

+limitations under the License.
+*/
+
+package resourcequota


in the admission_test.go, the test case is TestAdmitPodInNamespaceWithoutQuota? what about adding the access to quotaAccessor just for the testing purpose? then you may keep the test case there, instead of adding a new one here.

plugin/pkg/admission/resourcequota/admission_test.go

yliaog

ok. that makes sense.

I think one other way to do it is to add a method SetExternalLookupCache (similar to the method SetExternalKubeClientSet, or SetExternalKubeInformerFactory). But i think it's fine to test the lookupCache separately (as you currently do).

plugin/pkg/admission/resourcequota/admission_test.go

yliaog · 2020-08-04T00:37:46Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/resource_access_test.go

+limitations under the License.
+*/
+
+package resourcequota


ok. so this is testing the cache hit case, probably better to add a cache miss case too.

test/integration/quota/quota_test.go

plugin/pkg/admission/resourcequota/BUILD

pkg/controller/resourcequota/resource_quota_monitor.go

timuthy · 2020-08-04T12:59:57Z

Thanks for your review @yliaog. W.r.t. your suggestion, I've added more unit tests for the LRU cache lookup via 78c4ae4.

yliaog · 2020-08-04T19:44:43Z

thanks for the pr.

/lgtm

timuthy · 2020-08-05T05:42:09Z

/retest

timuthy · 2020-08-06T07:11:40Z

pinging @deads2k @soltysh for approval

timuthy · 2020-08-20T07:07:50Z

@deads2k @soltysh is there anything I can support you with to get this merged?

timuthy · 2020-09-04T13:41:18Z

Changes were needed for rebasing.
@mikedanese may you help to get this in since you reviewed #93946?

yliaog · 2020-09-04T16:45:41Z

/lgtm

yliaog · 2020-09-04T16:47:01Z

/milestone v1.20

k8s-ci-robot · 2020-09-04T16:47:03Z

@yliaog: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.20

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/test-infra repository.

timuthy · 2020-09-15T08:16:19Z

@yliaog thanks for you review. Do you have an idea how we can proceed with this PR?

yliaog · 2020-09-15T16:27:57Z

/assign @lavalamp

lavalamp · 2020-09-15T18:22:29Z

/approve
/milestone v1.20

Deferring to @yliaog :)

k8s-ci-robot · 2020-09-15T18:23:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, timuthy

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:

~~cmd/kube-controller-manager/OWNERS~~ [lavalamp]
~~hack/OWNERS~~ [lavalamp]
~~pkg/controller/OWNERS~~ [lavalamp]
~~pkg/kubeapiserver/OWNERS~~ [lavalamp]
~~pkg/quota/v1/OWNERS~~ [lavalamp]
~~plugin/pkg/admission/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp]
~~test/OWNERS~~ [lavalamp]
~~vendor/OWNERS~~ [lavalamp]

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 requested review from deads2k, yue9944882 and a team July 29, 2020 15:19

timuthy force-pushed the enhancement.move-resourcequota branch from fd2f272 to 4b8e4f8 Compare July 30, 2020 10:59

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2020

timuthy force-pushed the enhancement.move-resourcequota branch from 4b8e4f8 to a804727 Compare July 30, 2020 11:04

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 30, 2020

timuthy force-pushed the enhancement.move-resourcequota branch from a804727 to 90b6b0e Compare July 30, 2020 12:26

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 30, 2020

yliaog reviewed Jul 30, 2020

View reviewed changes

timuthy force-pushed the enhancement.move-resourcequota branch from 4c5c237 to 7b638fc Compare July 31, 2020 06:38

yliaog reviewed Aug 1, 2020

View reviewed changes

yliaog reviewed Aug 4, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2020

timuthy added 2 commits September 4, 2020 14:53

Move ResourceQuota admission to k8s.io/apiserver

70d440b

Add more tests for LRU cache lookup

cc0b86f

timuthy force-pushed the enhancement.move-resourcequota branch from 78c4ae4 to cc0b86f Compare September 4, 2020 13:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2020

k8s-ci-robot assigned lavalamp Sep 15, 2020

k8s-ci-robot added this to the v1.20 milestone Sep 15, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2020

k8s-ci-robot merged commit e7b9453 into kubernetes:master Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ResourceQuota admission to k8s.io/apiserver lib #93537

Move ResourceQuota admission to k8s.io/apiserver lib #93537

timuthy commented Jul 29, 2020 •

edited

Loading

k8s-ci-robot commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

yliaog Jul 30, 2020

timuthy Jul 31, 2020

yliaog Aug 1, 2020

yliaog Aug 4, 2020

yliaog commented Jul 30, 2020

timuthy commented Jul 31, 2020 •

edited

Loading

timuthy commented Jul 31, 2020

yliaog Aug 1, 2020

yliaog left a comment

yliaog Aug 4, 2020

timuthy commented Aug 4, 2020

yliaog commented Aug 4, 2020

timuthy commented Aug 5, 2020

timuthy commented Aug 6, 2020

timuthy commented Aug 20, 2020

timuthy commented Sep 4, 2020

yliaog commented Sep 4, 2020

yliaog commented Sep 4, 2020

k8s-ci-robot commented Sep 4, 2020

timuthy commented Sep 15, 2020

yliaog commented Sep 15, 2020

lavalamp commented Sep 15, 2020

k8s-ci-robot commented Sep 15, 2020

Move ResourceQuota admission to k8s.io/apiserver lib #93537

Move ResourceQuota admission to k8s.io/apiserver lib #93537

Conversation

timuthy commented Jul 29, 2020 • edited Loading

k8s-ci-robot commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

yliaog Jul 30, 2020

Choose a reason for hiding this comment

timuthy Jul 31, 2020

Choose a reason for hiding this comment

yliaog Aug 1, 2020

Choose a reason for hiding this comment

yliaog Aug 4, 2020

Choose a reason for hiding this comment

yliaog commented Jul 30, 2020

timuthy commented Jul 31, 2020 • edited Loading

timuthy commented Jul 31, 2020

yliaog Aug 1, 2020

Choose a reason for hiding this comment

yliaog left a comment

Choose a reason for hiding this comment

yliaog Aug 4, 2020

Choose a reason for hiding this comment

timuthy commented Aug 4, 2020

yliaog commented Aug 4, 2020

timuthy commented Aug 5, 2020

timuthy commented Aug 6, 2020

timuthy commented Aug 20, 2020

timuthy commented Sep 4, 2020

yliaog commented Sep 4, 2020

yliaog commented Sep 4, 2020

k8s-ci-robot commented Sep 4, 2020

timuthy commented Sep 15, 2020

yliaog commented Sep 15, 2020

lavalamp commented Sep 15, 2020

k8s-ci-robot commented Sep 15, 2020

timuthy commented Jul 29, 2020 •

edited

Loading

timuthy commented Jul 31, 2020 •

edited

Loading