ResourceQuota ability to support default limited resources #36765

derekwaynecarr · 2016-11-14T19:51:55Z

Add support for the ability to configure the quota system to identify specific resources that are limited by default. A limited resource means its consumption is denied absent a covering quota. This is in contrast to the current behavior where consumption is unlimited absent a covering quota. Intended use case is to allow operators to restrict consumption of high-cost resources by default.

Example configuration:

admission-control-config-file.yaml

apiVersion: apiserver.k8s.io/v1alpha1
kind: AdmissionConfiguration
plugins:
- name: "ResourceQuota"
  configuration:
    apiVersion: resourcequota.admission.k8s.io/v1alpha1
    kind: Configuration
    limitedResources:
    - resource: pods
      matchContains:
      - pods
      - requests.cpu
    - resource: persistentvolumeclaims
      matchContains:
      - .storageclass.storage.k8s.io/requests.storage

In the above configuration, if a namespace lacked a quota for any of the following:

cpu
any pvc associated with particular storage class

The attempt to consume the resource is denied with a message stating the user has insufficient quota for the matching resources.

$ kubectl create -f pvc-gold.yaml 
Error from server: error when creating "pvc-gold.yaml": insufficient quota to consume: gold.storageclass.storage.k8s.io/requests.storage
$ kubectl create quota quota --hard=gold.storageclass.storage.k8s.io/requests.storage=10Gi
$ kubectl create -f pvc-gold.yaml 
... created

derekwaynecarr · 2016-11-14T19:53:11Z

@eparis @erinboyd @deads2k -- this is WIP and is not yet functional, opening here to track its completion for kube 1.6.

@bgrant0607 -- this was the general idea we discussed at kubecon for allowing quota to identify resources that are limited by default instead of unlimited.

derekwaynecarr · 2016-12-20T23:06:11Z

In putting this together, it's come to my attention that we do not pass versioned configuration to admission controllers in Kubernetes today. As a result, I think I need to fix-up existing admission plug-ins first, and codify the model for how versioned config is handled before finishing this out.

k8s-reviewable · 2017-01-13T17:12:36Z

This change is

derekwaynecarr · 2017-02-14T22:16:14Z

its a belated xmas miracle, the bots are all happy.

@smarterclayton @deads2k - ptal.

smarterclayton · 2017-02-14T22:23:46Z

plugin/pkg/admission/resourcequota/apis/resourcequota/v1alpha1/types.go

+	// the value would be "persistentvolumeclaims".
+	// If the resource exists in a named API group, the name must be
+	// fully qualified, i.e. "deployments.extensions"
+	APIGroupResource string `json:"apiGroupResource"`


This is consistent with RBAC, correct?

RBAC has the following:

type PolicyRule struct { // Verbs is a list of Verbs that apply to ALL the ResourceKinds and AttributeRestrictions contained in this rule. VerbAll represents all kinds. Verbs []string // APIGroups is the name of the APIGroup that contains the resources. // If multiple API groups are specified, any action requested against one of the enumerated resources in any API group will be allowed. APIGroups []string // Resources is a list of resources this rule applies to. ResourceAll represents all resources. Resources []string // ResourceNames is an optional white list of names that the rule applies to. An empty set means that everything is allowed. ResourceNames []string // NonResourceURLs is a set of partial urls that a user should have access to. *s are allowed, but only as the full, final step in the path // If an action is not a resource API request, then the URL is split on '/' and is checked against the NonResourceURLs to look for a match. // Since non-resource URLs are not namespaced, this field is only applicable for ClusterRoles referenced from a ClusterRoleBinding. // Rules can either apply to API resources (such as "pods" or "secrets") or non-resource URL paths (such as "/api"), but not both. NonResourceURLs []string }

It splits APIGroup from Resource. Unlike Policy, I don't think there is meaning if both are not supplied on this item. For example, I don't think we would want to support that a particular constraint applies to all resources in an APIGroup for ResourceQuota.

I guess I'm okay with it.

smarterclayton · 2017-02-15T21:50:55Z

For alpha is sufficiently compact. API looks ok to me.

deads2k · 2017-02-16T19:56:57Z

plugin/pkg/admission/resourcequota/controller.go

 	}
-	if len(quotas) == 0 {
+	// if limited resources are disabled, we can just return safely when there are no quotas.
+	limitedResourcesDisabled := len(e.config.LimitedResources) == 0


if e.config must be non-nil, why do you allow a nil to be passed through the New function?

i modified new to allow nil, but default to empty config.

deads2k · 2017-02-16T20:08:38Z

plugin/pkg/admission/resourcequota/controller.go

+	// track the cumulative set of resources that were required across all quotas
+	// this is needed to know if we have satisfied any constraints where consumption
+	// was limited by default.
+	requiredResourcesSet := sets.String{}


It took me a long time to puzzle out what this was. This is the list of resources that are restricted by quota applied to this object, right? Maybe restrictedResources?

deads2k · 2017-02-16T20:09:29Z

test/integration/quota/quota_test.go

 	internalClientset := internalclientset.NewForConfigOrDie(&restclient.Config{QPS: -1, Host: s.URL, ContentConfig: restclient.ContentConfig{GroupVersion: &api.Registry.GroupOrDie(v1.GroupName).GroupVersion}})
-	admission, err := resourcequota.NewResourceQuota(quotainstall.NewRegistry(nil, nil), 5, admissionCh)
+	config := &resourcequotaapi.Configuration{}
+	admission, err := resourcequota.NewResourceQuota(quotainstall.NewRegistry(nil, nil), config, 5, admissionCh)


Seems like this feature is worth an integration test.

agreed, will do.

deads2k · 2017-02-16T20:10:52Z

plugin/pkg/admission/resourcequota/apis/resourcequota/types.go

+	// the value would be "persistentvolumeclaims".
+	// If the resource exists in a named API group, the name must be
+	// fully qualified, i.e. "deployments.extensions"
+	APIGroupResource string


Why not a separate Group and Resource field?

i am perfectly happy to separate. it appears we lack a consensus decision on how to handle this.

see: https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/kubernetes-sig-api-machinery/fzmH1gqLuOQ/73H2NJJQBQAJ

to follow the policy convention, is that APIGroup and Resource?

to follow the policy convention, is that APIGroup and Resource?

I'd like that.

deads2k · 2017-02-16T20:11:05Z

minor comments

derekwaynecarr · 2017-02-17T05:25:31Z

@deads2k -- thanks for the review pass, i will look to update.

derekwaynecarr · 2017-02-18T18:32:05Z

@k8s-bot cvm gke e2e test this

derekwaynecarr · 2017-02-18T19:10:14Z

@deads2k updates addressed, ptal

deads2k · 2017-02-20T17:25:24Z

/lgtm
/approve

k8s-ci-robot · 2017-02-20T17:25:25Z

@deads2k: you can't LGTM a PR unless you are an assignee.

Details

In response to this comment:

/lgtm
/approve

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-github-robot · 2017-02-20T17:25:46Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: deads2k, derekwaynecarr

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [deads2k]
~~plugin/pkg/admission/OWNERS~~ [derekwaynecarr]
~~test/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

derekwaynecarr · 2017-02-20T17:29:38Z

@deads2k -- added you as an assignee, care to tag again so bot is happy?

deads2k · 2017-02-20T17:33:40Z

@deads2k -- added you as an assignee, care to tag again so bot is happy?

Tagged, though I don't think anyone would have dinged you for fixing it up yourself.

k8s-github-robot · 2017-02-20T18:37:42Z

Automatic merge from submit-queue (batch tested with PRs 41421, 41440, 36765, 41722)

googlebot added the cla: yes label Nov 14, 2016

k8s-github-robot assigned eparis Nov 14, 2016

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Nov 14, 2016

Hui-Zhi mentioned this pull request Dec 15, 2016

NVIDIA GPU discovery, could support more than 1 GPU on 1 Minion, more than 1 GPU on 1 Pod. #28216

Closed

derekwaynecarr force-pushed the quota-precious-resources branch from d7453f2 to c00857d Compare December 15, 2016 23:07

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 15, 2016

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2016

derekwaynecarr force-pushed the quota-precious-resources branch from c00857d to 80f8d3f Compare December 19, 2016 17:11

derekwaynecarr mentioned this pull request Dec 21, 2016

Admission control support for versioned configuration files #39109

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2017

derekwaynecarr force-pushed the quota-precious-resources branch from 80f8d3f to 8036a20 Compare January 9, 2017 03:06

derekwaynecarr force-pushed the quota-precious-resources branch 2 times, most recently from 9f336e0 to f2f952a Compare January 9, 2017 19:16

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2017

derekwaynecarr force-pushed the quota-precious-resources branch from f2f952a to 0f3252b Compare January 13, 2017 16:09

derekwaynecarr changed the title ~~WIP: Ability to identify resources that are precious in quota~~ Ability to have ResourceQuota configure resources limited by default Jan 13, 2017

derekwaynecarr changed the title ~~Ability to have ResourceQuota configure resources limited by default~~ ResourceQuota has ability to identified default limited resources Jan 13, 2017

derekwaynecarr changed the title ~~ResourceQuota has ability to identified default limited resources~~ ResourceQuota ability to support default limited resources Jan 13, 2017

derekwaynecarr force-pushed the quota-precious-resources branch from 0f3252b to 810ca05 Compare January 13, 2017 17:44

derekwaynecarr removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2017

smarterclayton reviewed Feb 14, 2017

View reviewed changes

deads2k reviewed Feb 16, 2017

View reviewed changes

derekwaynecarr added 2 commits February 18, 2017 12:09

ResourceQuota API configuration type

8575978

Implement support for limited resources in quota

3fad0cb

derekwaynecarr force-pushed the quota-precious-resources branch from 552e8e5 to 3fad0cb Compare February 18, 2017 17:10

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2017

derekwaynecarr assigned deads2k and unassigned smarterclayton, derekwaynecarr and eparis Feb 20, 2017

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2017

k8s-github-robot merged commit 506950a into kubernetes:master Feb 20, 2017

derekwaynecarr mentioned this pull request Aug 18, 2017

HugePages feature #50859

Merged

derekwaynecarr mentioned this pull request Sep 28, 2017

[Proposal] Design doc for priority in resource quota kubernetes/community#933

Merged

jihoon-seo mentioned this pull request Mar 15, 2022

[pt-br] Adding resouces quotas page translation kubernetes/website#32209

Merged

ResourceQuota ability to support default limited resources #36765

ResourceQuota ability to support default limited resources #36765

Uh oh!

Conversation

derekwaynecarr commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekwaynecarr commented Nov 14, 2016

Uh oh!

derekwaynecarr commented Dec 20, 2016

Uh oh!

k8s-reviewable commented Jan 13, 2017

Uh oh!

derekwaynecarr commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Feb 16, 2017

Uh oh!

derekwaynecarr commented Feb 17, 2017

Uh oh!

derekwaynecarr commented Feb 18, 2017

Uh oh!

derekwaynecarr commented Feb 18, 2017

Uh oh!

deads2k commented Feb 20, 2017

Uh oh!

k8s-ci-robot commented Feb 20, 2017

Uh oh!

k8s-github-robot commented Feb 20, 2017

Uh oh!

derekwaynecarr commented Feb 20, 2017

Uh oh!

deads2k commented Feb 20, 2017

Uh oh!

k8s-github-robot commented Feb 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

derekwaynecarr commented Nov 14, 2016 •

edited

Loading

deads2k Feb 16, 2017 •

edited

Loading