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

Let the quota evaluator handle mutating specs of pod & pvc #51174

Merged
merged 1 commit into from Aug 26, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/quota/evaluator/core/BUILD
Expand Up @@ -25,6 +25,7 @@ go_library(
"//pkg/api/helper/qos:go_default_library",
"//pkg/api/v1:go_default_library",
"//pkg/api/validation:go_default_library",
"//pkg/kubeapiserver/admission/util:go_default_library",
"//pkg/quota:go_default_library",
"//pkg/quota/generic:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
Expand Down
15 changes: 13 additions & 2 deletions pkg/quota/evaluator/core/persistent_volume_claims.go
Expand Up @@ -32,6 +32,7 @@ import (
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/helper"
k8s_api_v1 "k8s.io/kubernetes/pkg/api/v1"
"k8s.io/kubernetes/pkg/kubeapiserver/admission/util"
"k8s.io/kubernetes/pkg/quota"
"k8s.io/kubernetes/pkg/quota/generic"
)
Expand Down Expand Up @@ -141,8 +142,18 @@ func (p *pvcEvaluator) GroupKind() schema.GroupKind {
}

// Handles returns true if the evaluator should handle the specified operation.
func (p *pvcEvaluator) Handles(operation admission.Operation) bool {
return admission.Create == operation
func (p *pvcEvaluator) Handles(a admission.Attributes) bool {
op := a.GetOperation()
if op == admission.Create {
return true
}
updateUninitialized, err := util.IsUpdatingUninitializedObject(a)
if err != nil {
// fail closed, will try to give an evaluation.
return true
}
// only uninitialized pvc might be updated.
return updateUninitialized
}

// Matches returns true if the evaluator matches the specified quota with the provided input item
Expand Down
18 changes: 14 additions & 4 deletions pkg/quota/evaluator/core/pods.go
Expand Up @@ -34,6 +34,7 @@ import (
"k8s.io/kubernetes/pkg/api/helper/qos"
k8s_api_v1 "k8s.io/kubernetes/pkg/api/v1"
"k8s.io/kubernetes/pkg/api/validation"
"k8s.io/kubernetes/pkg/kubeapiserver/admission/util"
"k8s.io/kubernetes/pkg/quota"
"k8s.io/kubernetes/pkg/quota/generic"
)
Expand Down Expand Up @@ -131,10 +132,19 @@ func (p *podEvaluator) GroupKind() schema.GroupKind {
return api.Kind("Pod")
}

// Handles returns true of the evaluator should handle the specified operation.
func (p *podEvaluator) Handles(operation admission.Operation) bool {
// TODO: update this if/when pods support resizing resource requirements.
return admission.Create == operation
// Handles returns true of the evaluator should handle the specified attributes.
func (p *podEvaluator) Handles(a admission.Attributes) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have a goal to enable the following:
#51370

where do you recommend i best layer in the referenced goal with the need to support initializers?

op := a.GetOperation()
if op == admission.Create {
return true
}
updateUninitialized, err := util.IsUpdatingUninitializedObject(a)
if err != nil {
// fail closed, will try to give an evaluation.
return true
}
// only uninitialized pods might be updated.
return updateUninitialized
}

// Matches returns true if the evaluator matches the specified quota with the provided input item
Expand Down
3 changes: 2 additions & 1 deletion pkg/quota/evaluator/core/services.go
Expand Up @@ -108,7 +108,8 @@ func (p *serviceEvaluator) GroupKind() schema.GroupKind {
}

// Handles returns true of the evaluator should handle the specified operation.
func (p *serviceEvaluator) Handles(operation admission.Operation) bool {
func (p *serviceEvaluator) Handles(a admission.Attributes) bool {
operation := a.GetOperation()
// We handle create and update because a service type can change.
return admission.Create == operation || admission.Update == operation
}
Expand Down
5 changes: 3 additions & 2 deletions pkg/quota/generic/evaluator.go
Expand Up @@ -150,8 +150,9 @@ func (o *ObjectCountEvaluator) GroupKind() schema.GroupKind {
return o.InternalGroupKind
}

// Handles returns true if the object count evaluator needs to track this operation.
func (o *ObjectCountEvaluator) Handles(operation admission.Operation) bool {
// Handles returns true if the object count evaluator needs to track this attributes.
func (o *ObjectCountEvaluator) Handles(a admission.Attributes) bool {
operation := a.GetOperation()
return operation == admission.Create || (o.AllowCreateOnUpdate && operation == admission.Update)
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/quota/interfaces.go
Expand Up @@ -45,9 +45,9 @@ type Evaluator interface {
Constraints(required []api.ResourceName, item runtime.Object) error
// GroupKind returns the groupKind that this object knows how to evaluate
GroupKind() schema.GroupKind
// Handles determines if quota could be impacted by the specified operation.
// Handles determines if quota could be impacted by the specified attribute.
// If true, admission control must perform quota processing for the operation, otherwise it is safe to ignore quota.
Handles(operation admission.Operation) bool
Handles(operation admission.Attributes) bool
// Matches returns true if the specified quota matches the input item
Matches(resourceQuota *api.ResourceQuota, item runtime.Object) (bool, error)
// MatchingResources takes the input specified list of resources and returns the set of resources evaluator matches.
Expand Down
8 changes: 3 additions & 5 deletions plugin/pkg/admission/resourcequota/controller.go
Expand Up @@ -375,8 +375,7 @@ func (e *quotaEvaluator) checkRequest(quotas []api.ResourceQuota, a admission.At
return quotas, nil
}

op := a.GetOperation()
if !evaluator.Handles(op) {
if !evaluator.Handles(a) {
return quotas, nil
}

Expand Down Expand Up @@ -463,7 +462,7 @@ func (e *quotaEvaluator) checkRequest(quotas []api.ResourceQuota, a admission.At
return nil, admission.NewForbidden(a, fmt.Errorf("quota usage is negative for resource(s): %s", prettyPrintResourceNames(negativeUsage)))
}

if admission.Update == op {
if admission.Update == a.GetOperation() {
prevItem := a.GetOldObject()
if prevItem == nil {
return nil, admission.NewForbidden(a, fmt.Errorf("unable to get previous usage since prior version of object was not found"))
Expand Down Expand Up @@ -529,8 +528,7 @@ func (e *quotaEvaluator) Evaluate(a admission.Attributes) error {
}
// for this kind, check if the operation could mutate any quota resources
// if no resources tracked by quota are impacted, then just return
op := a.GetOperation()
if !evaluator.Handles(op) {
if !evaluator.Handles(a) {
return nil
}

Expand Down
79 changes: 79 additions & 0 deletions test/e2e/resource_quota.go
Expand Up @@ -147,6 +147,85 @@ var _ = framework.KubeDescribe("ResourceQuota", func() {
Expect(err).NotTo(HaveOccurred())
})

It("should create a ResourceQuota and capture the life of an uninitialized pod.", func() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test passes only with #50344. I can comment the test out in this PR, or squash this PR with #50344, up to the reviewers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I leave the test here to show what the end goal is. I can move the test to #50344 if that's better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine if 50344 uncomments it.

// TODO: uncomment the test when #50344 is merged.
// By("Creating a ResourceQuota")
// quotaName := "test-quota"
// resourceQuota := newTestResourceQuota(quotaName)
// resourceQuota, err := createResourceQuota(f.ClientSet, f.Namespace.Name, resourceQuota)
// Expect(err).NotTo(HaveOccurred())

// By("Ensuring resource quota status is calculated")
// usedResources := v1.ResourceList{}
// usedResources[v1.ResourceQuotas] = resource.MustParse("1")
// err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources)
// Expect(err).NotTo(HaveOccurred())

// By("Creating an uninitialized Pod that fits quota")
// podName := "test-pod"
// requests := v1.ResourceList{}
// requests[v1.ResourceCPU] = resource.MustParse("500m")
// requests[v1.ResourceMemory] = resource.MustParse("252Mi")
// pod := newTestPodForQuota(f, podName, requests, v1.ResourceList{})
// pod.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "unhandled"}}}
// _, err = f.ClientSet.Core().Pods(f.Namespace.Name).Create(pod)
// // because no one is handling the initializer, server will return a 504 timeout
// if err != nil && !errors.IsTimeout(err) {
// framework.Failf("expect err to be timeout error, got %v", err)
// }
// podToUpdate, err := f.ClientSet.Core().Pods(f.Namespace.Name).Get(podName, metav1.GetOptions{})
// Expect(err).NotTo(HaveOccurred())

// By("Ensuring ResourceQuota status captures the pod usage")
// usedResources[v1.ResourceQuotas] = resource.MustParse("1")
// usedResources[v1.ResourcePods] = resource.MustParse("1")
// usedResources[v1.ResourceCPU] = requests[v1.ResourceCPU]
// usedResources[v1.ResourceMemory] = requests[v1.ResourceMemory]
// err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources)
// Expect(err).NotTo(HaveOccurred())

// By("Not allowing an uninitialized pod to be created that exceeds remaining quota")
// requests = v1.ResourceList{}
// requests[v1.ResourceCPU] = resource.MustParse("600m")
// requests[v1.ResourceMemory] = resource.MustParse("100Mi")
// pod = newTestPodForQuota(f, "fail-pod", requests, v1.ResourceList{})
// pod.Initializers = &metav1.Initializers{Pending: []metav1.Initializer{{Name: "unhandled"}}}
// pod, err = f.ClientSet.Core().Pods(f.Namespace.Name).Create(pod)
// Expect(err).To(HaveOccurred())
// fmt.Println("CHAO: err=", err)

// By("Ensuring an uninitialized pod can update its resource requirements")
// // a pod cannot dynamically update its resource requirements.
// requests = v1.ResourceList{}
// requests[v1.ResourceCPU] = resource.MustParse("100m")
// requests[v1.ResourceMemory] = resource.MustParse("100Mi")
// podToUpdate.Spec.Containers[0].Resources.Requests = requests
// _, err = f.ClientSet.Core().Pods(f.Namespace.Name).Update(podToUpdate)
// Expect(err).NotTo(HaveOccurred())

// By("Ensuring attempts to update pod resource requirements did change quota usage")
// usedResources[v1.ResourceQuotas] = resource.MustParse("1")
// usedResources[v1.ResourcePods] = resource.MustParse("1")
// usedResources[v1.ResourceCPU] = requests[v1.ResourceCPU]
// usedResources[v1.ResourceMemory] = requests[v1.ResourceMemory]
// err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources)
// Expect(err).NotTo(HaveOccurred())

// TODO: uncomment the test when the replenishment_controller uses the
// sharedInformer that list/watches uninitialized objects.
// By("Deleting the pod")
// err = f.ClientSet.Core().Pods(f.Namespace.Name).Delete(podName, metav1.NewDeleteOptions(0))
// Expect(err).NotTo(HaveOccurred())
//
// By("Ensuring resource quota status released the pod usage")
// usedResources[v1.ResourceQuotas] = resource.MustParse("1")
// usedResources[v1.ResourcePods] = resource.MustParse("0")
// usedResources[v1.ResourceCPU] = resource.MustParse("0")
// usedResources[v1.ResourceMemory] = resource.MustParse("0")
// err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources)
// Expect(err).NotTo(HaveOccurred())
})

It("should create a ResourceQuota and capture the life of a pod.", func() {
By("Creating a ResourceQuota")
quotaName := "test-quota"
Expand Down