UPSTREAM: 30145: Add PVC storage to Limit Range #11396
Conversation
|
@pweil- @liggitt PTAL? Not sure who to ping for this cherry pick. Docs added here: openshift/openshift-docs#3084 |
|
@markturansky since this is going in to Kube 1.5 and 3.4 is based on 1.4 is there justification for merge? Ie, is this a critical feature for 3.4? @smarterclayton thoughts on the above? At this point I think we should be holding off cherry-picks as much as possible. |
|
Our Dedicated offering can get by with ClusterResourceQuota, which would quota storage by project. This solution works with the limited numbers in Dedicated. In the Online version, CRQ won't scale as needed and so we can't use it. We need to limit the size of PVCs in a project in Online. We can already limit the count. This would limit overall consumption. @abhgupta please confirm |
|
Also worth noting, the Ops teams really want to get multi-zone volumes in Online. 3.3 supports multi-zone, but we can't because our custom provisioner is what provides our current storage consumption limits. So we're looking to kill our custom provisioner in 3.4, but we need this cherry-pick. |
|
@pweil- this is required for Online DevPreview and the upcoming paid tier that will be based on OCP 3.4. We have been hit with multiple issues maintaining the custom dynamic provisioner with Online and really need to be able to get rid of that and just leverage the product to provide the quota/limit restrictions. If this does not become part of OCP 3.4, then we will need to rebase our custom provisioner to ensure it works well with the stock PVC controllers (OCP 3.4), which poses additional challenges and delays at our end. |
| @@ -2544,6 +2544,17 @@ func ValidateLimitRange(limitRange *api.LimitRange) field.ErrorList { | |||
| } | |||
| } | |||
|
|
|||
| if limit.Type == api.LimitTypePersistentVolumeClaim { | |||
| _, minQuantityFound := limit.Min[api.ResourceStorage] | |||
There was a problem hiding this comment.
@markturansky - i must have missed this in the review upstream. i should be able to set just a min and not a max, or a max and not a min, disagree?
There was a problem hiding this comment.
Allowing either to be empty opens the limit range to whatever the underlying infrastructure wants to enforce. IDK if that's good or bad, but AWS, for example, would enforce a 1Gi minimum and some large max Gi.
0 is an effective "no min".
There was a problem hiding this comment.
this is a confusing special case and I would prefer to get rid of it.
can we get rid of it here and make a separate pr to upstream that i can merge?
There was a problem hiding this comment.
we can loosen the validation upstream and merge this now.
|
[merge] |
|
re-[merge] Failure in |
|
Evaluated for origin merge up to c0b3b9d |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to c0b3b9d |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10451/) (Base Commit: bdf48b1) (Image: devenv-rhel7_5218) |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10450/) (Base Commit: 2016d68) |
|
FYI - 3.4 has the ability quota cumulative requests.storage and not just On Thu, Oct 20, 2016 at 4:13 PM, Abhishek Gupta [email protected]
|
…lidation Automatic merge from submit-queue Loosened validation on PVC LimitRanger This PR loosens validation on PVC LimitRanger so that either Min or Max are required, but not both. Per @derekwaynecarr openshift/origin#11396 (comment)
Cherry-pick of kubernetes/kubernetes#30145
@abhgupta @derekwaynecarr @smarterclayton