Project request quota admission control by csrwng · Pull Request #6768 · openshift/origin

csrwng · 2016-01-21T19:15:56Z

Adds admission control plugin to control quota per plugin configuration.

csrwng · 2016-01-21T19:16:42Z

@deads2k @liggitt ptal

smarterclayton · 2016-01-21T19:19:07Z

pkg/project/admission/reqlimit/admission.go

Is this going to be shared?

It is... same cache used in the other project admission control plugins

csrwng · 2016-01-21T19:32:34Z

updated package name

csrwng · 2016-01-21T22:08:59Z

[test]

deads2k · 2016-01-22T00:29:35Z

unit test failures

liggitt · 2016-01-22T00:40:39Z

@deads2k I thought we were wondering if we even needed this. Did you resolve that we did?

liggitt · 2016-01-22T00:43:19Z

pkg/project/admission/requestlimit/admission.go

Don't you need to find all the limits that apply and take the min (or the max, not sure which direction we'd go)

Not sure that we have a use case for that or that selectors are going to be that complex. Right now it's relying on the order of selectors in the config and, for what we want this for, imho it's sufficient.

If I apply a limit based on label selection, I'd be surprised if it didn't take effect. I think we should pick a direction (either max or min of matching limits), iterate to determine that limit, and document which direction we chose. I think I would tend toward taking the max (just like policy... you can do something if any of your roles allows it).

If I apply a limit based on label selection, I'd be surprised if it didn't take effect. I think we should pick a direction (either max or min of matching limits), iterate to determine that limit, and document which direction we chose. I think I would tend toward taking the max (just like policy... you can do anything allowed by any policy grant).

I prefer first one wins. Otherwise we'll pick a direction and not like it. I think its more likely that I know which labels have priority over which other labels and build my config file that way.

Not sure that picking the direction would alleviate the problem of having a limit not apply. Suppose we say max wins, and we have a very general rule that gives you a max of 100, and we add a more specific selector with a max of 5. Your more specific rule would never apply. Suppose we say min wins... then we can't easily configure something like the online use case where we want the default to be 1, but only for certain users increase that limit to 100. The more selective limit would never apply.

I prefer first one wins. Otherwise we'll pick a direction and not like it. I think its more likely that I know which labels have priority over which other labels and build my config file that way.

I could buy that. It would also let you restrict specific users while letting everyone else have a higher limit.

liggitt · 2016-01-22T00:46:06Z

Also, this doesn't solve the issue with a lagging store allowing a few requests through until it catches up

csrwng · 2016-01-22T02:21:23Z

Also, this doesn't solve the issue with a lagging store allowing a few requests through until it catches up

Can you think of a way to mitigate this?

liggitt · 2016-01-22T02:26:52Z

pkg/project/admission/requestlimit/admission_test.go

isn't the yaml for this level: platinum?

fixed. Oddly it worked with either form.

liggitt · 2016-01-22T02:28:38Z

Can you think of a way to mitigate this?

Not without recording the update somewhere that would generate a conflict (like an annotation on the user), and having a reconciling controller to sweep and fix the annotation if it got out of sync (like resource quota)... not sure I like something that complex for this

csrwng · 2016-01-22T03:30:40Z

Added requester index to project cache

deads2k · 2016-01-22T12:46:02Z

@deads2k I thought we were wondering if we even needed this. Did you resolve that we did?

We could work around it, but since this is has general utility (its commonly requested), useful for us, and relatively small it seemed better to bring it in.

csrwng · 2016-01-22T15:20:50Z

[test]

smarterclayton · 2016-01-22T16:29:36Z

You could require an atomic mutation on the user in order to allow it. You
could store the request into etcd, and then have a controller delete the
request when it satisfies it (doesn't limit the number of requests you can
create though). All the project requesters could mutate a single object
which limits to one inflight per user at a time. The last is probably
worth considering - it's effectively a lock on project creations, and then
we just have to decide whether we want to rate limit project creations on
the cluster in general.

On Fri, Jan 22, 2016 at 10:20 AM, Cesar Wong notifications@github.com
wrote:

[test]

—
Reply to this email directly or view it on GitHub
#6768 (comment).

smarterclayton · 2016-01-22T16:31:39Z

I'm tempted though to say that since we're paying the price of the project
cache already the squeaking through doesn't bother me - it just means that
you have to wait post creation and ensure that the resource version
returned by the write of the project matches the project cache, and check
your quota again. You can still burst across the cluster, but done right
you can parallelize to max number of masters.

On Fri, Jan 22, 2016 at 11:29 AM, Clayton Coleman ccoleman@redhat.com
wrote:

You could require an atomic mutation on the user in order to allow it.
You could store the request into etcd, and then have a controller delete
the request when it satisfies it (doesn't limit the number of requests you
can create though). All the project requesters could mutate a single
object which limits to one inflight per user at a time. The last is
probably worth considering - it's effectively a lock on project creations,
and then we just have to decide whether we want to rate limit project
creations on the cluster in general.

On Fri, Jan 22, 2016 at 10:20 AM, Cesar Wong notifications@github.com
wrote:

[test]

—
Reply to this email directly or view it on GitHub
#6768 (comment).

csrwng · 2016-01-22T16:45:42Z

it just means that
you have to wait post creation and ensure that the resource version
returned by the write of the project matches the project cache, and check
your quota again.

@smarterclayton how can I do this?

smarterclayton · 2016-01-22T17:21:37Z

Since we don't have post admission control yet (as @derekwaynecarr has floated) the current solution is fine, assuming a) the leak window is minimal even if an attacker is blowing up the server and b) you have a corresponding controller that goes and fixes up projects that are out of band (i.e. the oldest projects are set to zero quota). The controller can be post 3.2, but it's essentially quota reconciliation and needs to tie in with the proposed organizational quota constraint.

csrwng · 2016-01-22T17:29:01Z

Sounds good to me

derekwaynecarr · 2016-01-22T18:17:36Z

Do we ever plan on letting users organize their projects via labels? If so, would we basically have to no longer treat a Project as a virtual resource? Or would we store the project labels as annotations on the Namespace and then update the strategy code to pull a Project label from the annotation instead of the normal location? I guess the latter would make sense but it would be nice to have a plan...

derekwaynecarr · 2016-01-22T18:18:48Z

pkg/project/admission/requestlimit/admission.go

Glad we went with this approach. :-)

I like it too :) Allowed us to remove the embarrassing hack that merged the openshift and kube clients. Thank you for suggesting it.

csrwng · 2016-01-25T17:37:23Z

@deads2k @liggitt @smarterclayton Is the way that this plugin is configured (via user labels) ok? Do you have any suggestions for changes? Writing doc for it :-)

deads2k · 2016-01-25T17:57:56Z

pkg/cmd/server/api/latest/helpers.go

Add a TODO. This should go away once we have the yaml serializer in the rebase after this one.

deads2k · 2016-01-25T18:06:36Z

@deads2k @liggitt @smarterclayton Is the way that this plugin is configured (via user labels) ok?

I like using labels on users for it.

csrwng · 2016-01-25T22:24:24Z

Addressed comments so far

csrwng · 2016-01-26T18:36:09Z

@ironcladlou ptal

deads2k · 2016-01-26T18:39:57Z

pkg/project/admission/requestlimit/v1/types.go

json tags instead of yaml.

csrwng · 2016-01-26T19:02:21Z

struct tags fixed

csrwng · 2016-01-26T19:47:13Z

Test error seems like a new error from the vagrant plugin:

Running TEST_ASSETS=true TEST_ASSETS_HEADLESS=true make check -j --output-sync=recurse...
make: unrecognized option '--output-sync=recurse'

deads2k · 2016-01-26T19:49:19Z

Test error seems like a new error from the vagrant plugin:

@smarterclayton did you turn on parallel? the RHEL images don't have a recent level of make

deads2k · 2016-01-27T15:40:51Z

lgtm

csrwng · 2016-01-27T17:49:37Z

looks like an etcd flake:

2016-01-27 11:19:48.486493 E | etcdserver: publish error: etcdserver: request timed out

[merge]

csrwng · 2016-01-27T22:02:49Z

Needs rebase on top of Kube rebase

openshift-bot · 2016-01-27T22:35:21Z

Evaluated for origin test up to 4391347

openshift-bot · 2016-01-27T23:53:41Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/513/)

openshift-bot · 2016-01-28T02:45:27Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4768/) (Image: devenv-rhel7_3269)

csrwng · 2016-01-28T13:36:12Z

[merge]

openshift-bot · 2016-01-28T13:40:24Z

Evaluated for origin merge up to 4391347

Merged by openshift-bot

smarterclayton reviewed Jan 21, 2016
View reviewed changes

csrwng force-pushed the projectreq_limit branch from 6f6bfc7 to 591cbd4 Compare January 21, 2016 19:32

liggitt reviewed Jan 22, 2016
View reviewed changes

csrwng force-pushed the projectreq_limit branch from 591cbd4 to 7e9a2f7 Compare January 22, 2016 02:29

csrwng force-pushed the projectreq_limit branch from dd62f15 to a8257c7 Compare January 22, 2016 03:33

derekwaynecarr reviewed Jan 22, 2016
View reviewed changes

deads2k reviewed Jan 25, 2016
View reviewed changes

csrwng mentioned this pull request Jan 25, 2016

Document the project request limit plugin openshift/openshift-docs#1476

Merged

csrwng force-pushed the projectreq_limit branch 2 times, most recently from 949ce7a to 29444e9 Compare January 25, 2016 22:23

deads2k reviewed Jan 26, 2016
View reviewed changes

pkg/project/admission/requestlimit/v1/types.go Outdated

Copy link

Contributor

deads2k Jan 26, 2016

Choose a reason for hiding this comment

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

json tags instead of yaml.

csrwng force-pushed the projectreq_limit branch from 5f07bb0 to 90e8d65 Compare January 27, 2016 15:56

Project request quota admission control

4391347

csrwng force-pushed the projectreq_limit branch from 90e8d65 to 4391347 Compare January 27, 2016 22:30

openshift-bot pushed a commit that referenced this pull request Jan 28, 2016

Merge pull request #6768 from csrwng/projectreq_limit

86a15eb

Merged by openshift-bot

openshift-bot merged commit 86a15eb into openshift:master Jan 28, 2016

csrwng deleted the projectreq_limit branch February 1, 2016 18:09

Comments

Conversation

csrwng commented Jan 21, 2016

Uh oh!

csrwng commented Jan 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csrwng commented Jan 21, 2016

Uh oh!

csrwng commented Jan 21, 2016

Uh oh!

deads2k commented Jan 22, 2016

Uh oh!

liggitt commented Jan 22, 2016

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!

liggitt commented Jan 22, 2016

Uh oh!

csrwng commented Jan 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Jan 22, 2016

Uh oh!

csrwng commented Jan 22, 2016

Uh oh!

deads2k commented Jan 22, 2016

Uh oh!

csrwng commented Jan 22, 2016

Uh oh!

smarterclayton commented Jan 22, 2016

Uh oh!

smarterclayton commented Jan 22, 2016

Uh oh!

csrwng commented Jan 22, 2016

Uh oh!

smarterclayton commented Jan 22, 2016

Uh oh!

csrwng commented Jan 22, 2016

Uh oh!

derekwaynecarr commented Jan 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csrwng commented Jan 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jan 25, 2016

Uh oh!

csrwng commented Jan 25, 2016

Uh oh!

csrwng commented Jan 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csrwng commented Jan 26, 2016

Uh oh!

csrwng commented Jan 26, 2016

Uh oh!

deads2k commented Jan 26, 2016

Uh oh!