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.

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

ResourceQuota ability to support default limited resources #36765

Merged

Conversation

@derekwaynecarr
Copy link
Member

commented Nov 14, 2016

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

@googlebot googlebot added the cla: yes label Nov 14, 2016

@derekwaynecarr

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2016

@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

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

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.

@derekwaynecarr derekwaynecarr force-pushed the derekwaynecarr:quota-precious-resources branch from 80f8d3f to 8036a20 Jan 9, 2017

@derekwaynecarr derekwaynecarr force-pushed the derekwaynecarr:quota-precious-resources branch 2 times, most recently from 9f336e0 to f2f952a Jan 9, 2017

@derekwaynecarr derekwaynecarr force-pushed the derekwaynecarr:quota-precious-resources branch from f2f952a to 0f3252b Jan 13, 2017

@derekwaynecarr 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 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 derekwaynecarr changed the title ResourceQuota has ability to identified default limited resources ResourceQuota ability to support default limited resources Jan 13, 2017

@k8s-reviewable

This comment has been minimized.

Copy link

commented Jan 13, 2017

This change is Reviewable

@derekwaynecarr derekwaynecarr force-pushed the derekwaynecarr:quota-precious-resources branch from 0f3252b to 810ca05 Jan 13, 2017

@derekwaynecarr

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2017

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

@smarterclayton @deads2k - ptal.

// 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 comment has been minimized.

Copy link
@smarterclayton

smarterclayton Feb 14, 2017

Contributor

This is consistent with RBAC, correct?

This comment has been minimized.

Copy link
@derekwaynecarr

derekwaynecarr Feb 15, 2017

Author Member

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.

This comment has been minimized.

Copy link
@smarterclayton

smarterclayton Feb 15, 2017

Contributor

I guess I'm okay with it.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2017

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

@@ -166,7 +173,9 @@ func (e *quotaEvaluator) checkAttributes(ns string, admissionAttributes []*admis
}
return
}
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

This comment has been minimized.

Copy link
@deads2k

deads2k Feb 16, 2017

Contributor

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

This comment has been minimized.

Copy link
@derekwaynecarr

derekwaynecarr Feb 18, 2017

Author Member

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

// 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{}

This comment has been minimized.

Copy link
@deads2k

deads2k Feb 16, 2017

Contributor

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?

@@ -65,7 +66,8 @@ func TestQuota(t *testing.T) {
admissionCh := make(chan struct{})
clientset := clientset.NewForConfigOrDie(&restclient.Config{QPS: -1, Host: s.URL, ContentConfig: restclient.ContentConfig{GroupVersion: &api.Registry.GroupOrDie(v1.GroupName).GroupVersion}})
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)

This comment has been minimized.

Copy link
@deads2k

deads2k Feb 16, 2017

Contributor

Seems like this feature is worth an integration test.

This comment has been minimized.

Copy link
@derekwaynecarr

derekwaynecarr Feb 17, 2017

Author Member

agreed, will do.

This comment has been minimized.

Copy link
@derekwaynecarr

derekwaynecarr Feb 18, 2017

Author Member

test added

// 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

This comment has been minimized.

Copy link
@deads2k

deads2k Feb 16, 2017

Contributor

Why not a separate Group and Resource field?

This comment has been minimized.

Copy link
@derekwaynecarr

derekwaynecarr Feb 17, 2017

Author Member

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?

This comment has been minimized.

Copy link
@deads2k

deads2k Feb 17, 2017

Contributor

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

I'd like that.

@deads2k

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

minor comments

@derekwaynecarr

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2017

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

@derekwaynecarr derekwaynecarr force-pushed the derekwaynecarr:quota-precious-resources branch from 552e8e5 to 3fad0cb Feb 18, 2017

@derekwaynecarr

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2017

@k8s-bot cvm gke e2e test this

@derekwaynecarr

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2017

@deads2k updates addressed, ptal

@deads2k

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

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

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

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

[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:

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

@derekwaynecarr

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2017

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

@deads2k deads2k added the lgtm label Feb 20, 2017

@deads2k

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

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

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

16 checks passed

Jenkins Bazel Build Build succeeded.
Details
Jenkins Cross Build Build succeeded.
Details
Jenkins GCE Node e2e Build succeeded.
Details
Jenkins GCE e2e Build succeeded.
Details
Jenkins GCE etcd3 e2e Build succeeded.
Details
Jenkins GCI GCE e2e Build succeeded.
Details
Jenkins GCI GKE smoke e2e Build succeeded.
Details
Jenkins GKE smoke e2e Build succeeded.
Details
Jenkins Kubemark GCE e2e Build succeeded.
Details
Jenkins kops AWS e2e Build succeeded.
Details
Jenkins non-CRI GCE Node e2e Build succeeded.
Details
Jenkins non-CRI GCE e2e Build succeeded.
Details
Jenkins unit/integration Build succeeded.
Details
Jenkins verification Build succeeded.
Details
Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation derekwaynecarr authorized
Details
@derekwaynecarr derekwaynecarr referenced this pull request Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.