quota: allow ignored resources to be customized by nikhita · Pull Request #64310 · kubernetes/kubernetes

nikhita · 2018-05-25T12:53:07Z

Allows the list of resources the resource quota controller should ignore to be customizable, so downstream integrators can add their own resources to the list, if necessary.

Fixes #64201 (comment)

Builds on top of #64201

/cc sttts deads2k derekwaynecarr
/assign deads2k

Release note:

Resources to be ignored by the resource quota controller can now be customized.

nikhita · 2018-05-25T12:54:49Z

The changes relevant to this PR are in e0a4384.

nikhita · 2018-05-25T13:09:40Z

/cc @ncdc

since you did the GC one (#45897)

sttts · 2018-05-25T14:25:49Z

cmd/controller-manager/app/options/resourcequotacontroller.go

 type ResourceQuotaControllerOptions struct {
 	ResourceQuotaSyncPeriod      metav1.Duration
 	ConcurrentResourceQuotaSyncs int32
+	RQIgnoredResources           []componentconfig.GroupResource


Do we need the RQ prefix?

Do we need the RQ prefix?

IMO no. I named it that way for consistency:

The sync period is called ResourceQuotaSyncPeriod and not just SyncPeriod.

The ignored resources for GC are also called GCIgnoredResources.

I guess we can change the above and remove the RQ prefix. Wdyt?

sttts · 2018-05-25T14:27:34Z

cmd/kube-controller-manager/app/core.go

-	// Periodically the quota controller to detect new resource types
-	go resourceQuotaController.Sync(discoveryFunc, 30*time.Second, ctx.Stop)
+	// Periodically sync the quota controller to detect new resource types
+	go resourceQuotaController.Sync(resourceQuotaControllerClient.Discovery(), 30*time.Second, ctx.Stop)


is this intentional that resourceQuotaControllerClient.Discovery() is only executed once?

sttts · 2018-05-25T14:27:49Z

cmd/kube-controller-manager/app/core.go

 	resourceQuotaController, err := resourcequotacontroller.NewResourceQuotaController(resourceQuotaControllerOptions)
 	if err != nil {
-		return false, err
+		return true, fmt.Errorf("Failed to start the resource quota controller: %v", err)


lower case errors

sttts · 2018-05-25T14:28:23Z

cmd/kube-controller-manager/app/options/options.go


 	s.GarbageCollectorController.GCIgnoredResources = gcIgnoredResources

+	rqIgnoredResources := make([]componentconfig.GroupResource, 0, len(quotainstall.DefaultIgnoredResources()))


assign quotainstall.DefaultIgnoredResources() to a var. Calling it twice looks fishy.

Done. Also rebased on top of master.

deads2k · 2018-05-25T14:32:19Z

I think this is highlighting the need to have a dynamic shared informer factory. If I've reading this code correctly, we've just doubled the number of times we're caching resources and not created the capital to make this easy in general.

With a dynamic shared informer factory, it becomes possible wire the "get informer for resource" to delegate across multiple candidates as we do in openshift. All that helps the general cause and avoids unnecessary duplication. I think the feature can wait until 1.12 so the proper infrastructure can be developed.

nikhita · 2018-05-25T14:46:24Z

I think this is highlighting the need to have a dynamic shared informer factory.

Makes sense. I'll create an issue for it.

nikhita · 2018-05-25T15:00:50Z

I'll create an issue for it.

Created #64319

nikhita · 2018-05-26T11:54:15Z

@deads2k @sttts can we still get the customization of ignored resources in for 1.11? It is independent from that of supporting custom resources.

k8s-ci-robot · 2018-05-26T18:27:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nikhita
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers: deads2k, mtaufen

Assign the PR to them by writing /assign @deads2k @mtaufen in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

Allow the list of resources the resource quota controller should ignore to be customizable, so downstream integrators can add their own resources to the list, if necessary.

k8s-ci-robot · 2018-05-26T19:16:03Z

@nikhita: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-device-plugin-gpu	`4558a47`	link	`/test pull-kubernetes-e2e-gce-device-plugin-gpu`

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.

Details

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.

nikhita · 2018-05-31T18:14:07Z

@sttts @deads2k what do you think about this + making sure that GetQuotableResources does not return an error (#59826 (comment)) for 1.11?

k8s-ci-robot · 2018-06-02T21:33:25Z

@nikhita: PR needs rebase.

Details

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.

liqlin2015 · 2018-08-10T06:47:31Z

@nikhita Is there any impact we just log error but not return error here https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/resourcequota/resource_quota_controller.go#L167

nikhita · 2018-08-10T08:30:16Z

@nikhita Is there any impact we just log error but not return error here https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/resourcequota/resource_quota_controller.go#L167

@liqlin2015 If we return the error here, the controller manager can crash. :)

This function here is modelled similar to how GetDeletableResources work for GC:

kubernetes/pkg/controller/garbagecollector/garbagecollector.go

Lines 640 to 643 in ecc64f2

    
           // All discovery errors are considered temporary. Upon encountering any error, 
        
           // GetDeletableResources will log and return any discovered resources it was 
        
           // able to process (which may be none). 
        
           func GetDeletableResources(discoveryClient discovery.ServerResourcesInterface) map[schema.GroupVersionResource]struct{} {

#55022 (issue) outlines the problems caused if we returned an error here. #55259 (PR) changed the GC to tolerate partial discovery errors to fix this issue.

ncdc · 2018-08-17T13:37:16Z

/uncc

fejta-bot · 2018-11-15T13:49:09Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2018-12-15T14:34:06Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2019-01-14T15:18:42Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2019-01-14T15:18:51Z

@fejta-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 25, 2018

k8s-ci-robot assigned deads2k May 25, 2018

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 25, 2018

k8s-ci-robot requested review from deads2k, derekwaynecarr and sttts May 25, 2018 12:53

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 25, 2018

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 25, 2018

nikhita mentioned this pull request May 25, 2018

Support object count quota for custom resources #64201

Closed

nikhita force-pushed the quota-customize-ignored-resources branch from 2ccd7f0 to c5bc701 Compare May 25, 2018 13:02

k8s-ci-robot requested a review from ncdc May 25, 2018 13:09

nikhita force-pushed the quota-customize-ignored-resources branch 2 times, most recently from 2ef5578 to 5e3122d Compare May 25, 2018 13:31

sttts reviewed May 25, 2018

View reviewed changes

nikhita mentioned this pull request May 25, 2018

Create dynamic informers and listers #64319

Closed

nikhita force-pushed the quota-customize-ignored-resources branch from 5e3122d to 14a575e Compare May 26, 2018 18:27

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 26, 2018

nikhita force-pushed the quota-customize-ignored-resources branch from 14a575e to 4558a47 Compare May 26, 2018 18:36

quota: allow ignored resources to be customized

acc804b

Allow the list of resources the resource quota controller should ignore to be customizable, so downstream integrators can add their own resources to the list, if necessary.

update generated files

4558a47

nikhita mentioned this pull request May 31, 2018

Resource Quota controller should allow discover-resources errors #59826

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2018

k8s-ci-robot removed the request for review from ncdc August 17, 2018 13:37

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 15, 2018

k8s-ci-robot closed this Jan 14, 2019


		s.GarbageCollectorController.GCIgnoredResources = gcIgnoredResources

		rqIgnoredResources := make([]componentconfig.GroupResource, 0, len(quotainstall.DefaultIgnoredResources()))

Conversation

nikhita commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikhita commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikhita commented May 25, 2018

Uh oh!

sttts May 25, 2018

Choose a reason for hiding this comment

Uh oh!

nikhita May 26, 2018

Choose a reason for hiding this comment

Uh oh!

sttts May 25, 2018

Choose a reason for hiding this comment

Uh oh!

sttts May 25, 2018

Choose a reason for hiding this comment

Uh oh!

sttts May 25, 2018

Choose a reason for hiding this comment

Uh oh!

nikhita May 26, 2018

Choose a reason for hiding this comment

Uh oh!

deads2k commented May 25, 2018

Uh oh!

nikhita commented May 25, 2018

Uh oh!

nikhita commented May 25, 2018

Uh oh!

nikhita commented May 26, 2018

Uh oh!

k8s-ci-robot commented May 26, 2018

Uh oh!

k8s-ci-robot commented May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikhita commented May 31, 2018

Uh oh!

k8s-ci-robot commented Jun 2, 2018

Uh oh!

liqlin2015 commented Aug 10, 2018

Uh oh!

nikhita commented Aug 10, 2018

Uh oh!

ncdc commented Aug 17, 2018

Uh oh!

fejta-bot commented Nov 15, 2018

Uh oh!

fejta-bot commented Dec 15, 2018

Uh oh!

fejta-bot commented Jan 14, 2019

Uh oh!

k8s-ci-robot commented Jan 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nikhita commented May 25, 2018 •

edited

Loading

nikhita commented May 25, 2018 •

edited

Loading

k8s-ci-robot commented May 26, 2018 •

edited

Loading