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

Add PVC storage to LimitRange #30145

Merged
merged 1 commit into from
Oct 13, 2016
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
18 changes: 18 additions & 0 deletions docs/design/admission_control_limit_range.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ as part of admission control.
4. Ability to specify default resource limits for a container
5. Ability to specify default resource requests for a container
6. Ability to enforce a ratio between request and limit for a resource.
7. Ability to enforce min/max storage requests for persistent volume claims

## Data Model

Expand Down Expand Up @@ -209,6 +210,23 @@ Across all containers in pod, the following must hold true
| Max | Limit (required) <= Max |
| LimitRequestRatio | LimitRequestRatio <= ( Limit (required, non-zero) / Request (non-zero) ) |

**Type: PersistentVolumeClaim**

Supported Resources:

1. storage

Supported Constraints:

Across all claims in a namespace, the following must hold true:

| Constraint | Behavior |
| ---------- | -------- |
| Min | Min >= Request (required) |
| Max | Max <= Request (required) |

Supported Defaults: None. Storage is a required field in `PersistentVolumeClaim`, so defaults are not applied at this time.
Copy link
Member

Choose a reason for hiding this comment

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

Many fields that are required are assigned server side if omitted, I had thought you needed that use case.

It's fine not doing it now, I was just surprised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. An admin might assign 1Gi as the default and that is applied before validation.

I wasn't specifically asked for that but I can add it if you think it is important to follow an established pattern.

Copy link
Member

Choose a reason for hiding this comment

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

defaulting support can come later when or if there is an actual need.


## Run-time configuration

The default ```LimitRange``` that is applied via Salt configuration will be
Expand Down
1 change: 1 addition & 0 deletions pkg/api/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ func IsStandardContainerResourceName(str string) bool {
var standardLimitRangeTypes = sets.NewString(
string(LimitTypePod),
string(LimitTypeContainer),
string(LimitTypePersistentVolumeClaim),
)

// IsStandardLimitRangeType returns true if the type is Pod or Container
Expand Down
2 changes: 2 additions & 0 deletions pkg/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -2655,6 +2655,8 @@ const (
LimitTypePod LimitType = "Pod"
// Limit that applies to all containers in a namespace
LimitTypeContainer LimitType = "Container"
// Limit that applies to all persistent volume claims in a namespace
LimitTypePersistentVolumeClaim LimitType = "PersistentVolumeClaim"
)

// LimitRangeItem defines a min/max usage limit for any resource that matches on kind
Expand Down
2 changes: 2 additions & 0 deletions pkg/api/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -3109,6 +3109,8 @@ const (
LimitTypePod LimitType = "Pod"
// Limit that applies to all containers in a namespace
LimitTypeContainer LimitType = "Container"
// Limit that applies to all persistent volume claims in a namespace
LimitTypePersistentVolumeClaim LimitType = "PersistentVolumeClaim"
Copy link
Member

Choose a reason for hiding this comment

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

can you add something to validation_test.go that tests with this limit type?

)

// LimitRangeItem defines a min/max usage limit for any resource that matches on kind.
Expand Down
11 changes: 11 additions & 0 deletions pkg/api/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -2942,6 +2942,17 @@ func ValidateLimitRange(limitRange *api.LimitRange) field.ErrorList {
}
}

if limit.Type == api.LimitTypePersistentVolumeClaim {
_, minQuantityFound := limit.Min[api.ResourceStorage]
_, maxQuantityFound := limit.Max[api.ResourceStorage]
if !minQuantityFound {
allErrs = append(allErrs, field.Required(idxPath.Child("min"), "minimum storage value is required"))
}
if !maxQuantityFound {
allErrs = append(allErrs, field.Required(idxPath.Child("max"), "maximum storage value is required"))
}
}

for k, q := range limit.MaxLimitRequestRatio {
allErrs = append(allErrs, validateLimitRangeResourceName(limit.Type, string(k), idxPath.Child("maxLimitRequestRatio").Key(string(k)))...)
keys.Insert(string(k))
Expand Down
39 changes: 39 additions & 0 deletions pkg/api/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6542,6 +6542,11 @@ func TestValidateLimitRange(t *testing.T) {
DefaultRequest: getResourceList("10m", "200Mi"),
MaxLimitRequestRatio: getResourceList("10", ""),
},
{
Type: api.LimitTypePersistentVolumeClaim,
Max: getStorageResourceList("10Gi"),
Min: getStorageResourceList("5Gi"),
},
},
},
},
Expand Down Expand Up @@ -6752,6 +6757,40 @@ func TestValidateLimitRange(t *testing.T) {
}},
"must be a standard limit type or fully qualified",
},
"invalid missing required min field": {
api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
Limits: []api.LimitRangeItem{
{
Type: api.LimitTypePersistentVolumeClaim,
Max: getStorageResourceList("10000T"),
},
},
}},
"minimum storage value is required",
},
"invalid missing required max field": {
api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
Limits: []api.LimitRangeItem{
{
Type: api.LimitTypePersistentVolumeClaim,
Min: getStorageResourceList("10000T"),
},
},
}},
"maximum storage value is required",
},
"invalid min greater than max": {
api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
Limits: []api.LimitRangeItem{
{
Type: api.LimitTypePersistentVolumeClaim,
Min: getStorageResourceList("10Gi"),
Max: getStorageResourceList("1Gi"),
},
},
}},
"min value 10Gi is greater than max value 1Gi",
},
}

for k, v := range errorCases {
Expand Down
36 changes: 33 additions & 3 deletions plugin/pkg/admission/limitranger/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -397,25 +397,55 @@ func (d *DefaultLimitRangerActions) Limit(limitRange *api.LimitRange, resourceNa
switch resourceName {
case "pods":
return PodLimitFunc(limitRange, obj.(*api.Pod))
case "persistentvolumeclaims":
return PersistentVolumeClaimLimitFunc(limitRange, obj.(*api.PersistentVolumeClaim))
}
return nil
}

// SupportsAttributes ignores all calls that do not deal with pod resources since that is
// all this supports now. Also ignores any call that has a subresource defined.
// SupportsAttributes ignores all calls that do not deal with pod resources or storage requests (PVCs).
// Also ignores any call that has a subresource defined.
func (d *DefaultLimitRangerActions) SupportsAttributes(a admission.Attributes) bool {
if a.GetSubresource() != "" {
return false
}

return a.GetKind().GroupKind() == api.Kind("Pod")
return a.GetKind().GroupKind() == api.Kind("Pod") || a.GetKind().GroupKind() == api.Kind("PersistentVolumeClaim")
}

// SupportsLimit always returns true.
func (d *DefaultLimitRangerActions) SupportsLimit(limitRange *api.LimitRange) bool {
return true
}

// PersistentVolumeClaimLimitFunc enforces storage limits for PVCs.
// Users request storage via pvc.Spec.Resources.Requests. Min/Max is enforced by an admin with LimitRange.
// Claims will not be modified with default values because storage is a required part of pvc.Spec.
// All storage enforced values *only* apply to pvc.Spec.Resources.Requests.
func PersistentVolumeClaimLimitFunc(limitRange *api.LimitRange, pvc *api.PersistentVolumeClaim) error {
var errs []error
for i := range limitRange.Spec.Limits {
limit := limitRange.Spec.Limits[i]
limitType := limit.Type
if limitType == api.LimitTypePersistentVolumeClaim {
for k, v := range limit.Min {
// normal usage of minConstraint. pvc.Spec.Resources.Limits is not recognized as user input
if err := minConstraint(limitType, k, v, pvc.Spec.Resources.Requests, api.ResourceList{}); err != nil {
errs = append(errs, err)
}
}
for k, v := range limit.Max {
// reverse usage of maxConstraint. We want to enforce the max of the LimitRange against what
// the user requested.
if err := maxConstraint(limitType, k, v, api.ResourceList{}, pvc.Spec.Resources.Requests); err != nil {
errs = append(errs, err)
}
}
}
}
return utilerrors.NewAggregate(errs)
}

// PodLimitFunc enforces resource requirements enumerated by the pod against
// the specified LimitRange. The pod may be modified to apply default resource
// requirements if not specified, and enumerated on the LimitRange
Expand Down