Reduce the amount of allocations in policy use by trying a cached resolver first #11028

smarterclayton · 2016-09-21T00:19:08Z

Simplified RuleResolver to be closer to upstream. Used a cached RuleResolver
in the NoEscalation checks and when fetching the binding on update. Remove a use of deepequal on a common path.

Not passing all tests yet, but @deads2k please take a look at the general structure.

This drops policy pretty far down the list in test-cmd - resolving #11003 for good
hopefully.

smarterclayton · 2016-09-21T00:20:15Z

Wrinkle is we can no longer give the nice 'reason' for which rule granted us. I noticed upstream doesn't have it - various ways to solve it including returning two lists or adding a source field to PolicyRule or switching to Bindings earlier.

deads2k · 2016-09-21T12:30:25Z

pkg/authorization/api/v1/defaults.go

-	if kapi.Semantic.Equalities.DeepEqual(oldAllowAllPolicyRule, *obj) && obj.APIGroups == nil {
+	// match the old allow all rule, but only if API groups is nil (not specified in the incoming JSON)
+	oldAllowAllRule := obj.APIGroups == nil &&
+		// avoid calling the very expensive DeepEqual by inlining specific checks


deads2k · 2016-09-21T12:31:06Z

pkg/authorization/api/v1/defaults.go

+		len(obj.Resources) == 1 && obj.Resources[0] == internal.ResourceAll &&
+		len(obj.AttributeRestrictions.Raw) == 0 && len(obj.ResourceNames) == 0 &&
+		len(obj.NonResourceURLsSlice) == 0 &&
+		// semantic equalities will ignore nil vs empty for other fields as a safety


add a scary comment here about not removing this check.

deads2k · 2016-09-21T12:36:37Z

structure looks reasonable.

smarterclayton · 2016-09-27T20:47:03Z

[test]

smarterclayton · 2016-09-29T13:36:53Z

@knobuc @ramr router flake ^

smarterclayton · 2016-09-29T15:11:25Z

Flake link is https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/6545/consoleFull#191293162556bf4006e4b05b79524e5923

[test]

smarterclayton · 2016-09-30T21:14:18Z

@derekwaynecarr from https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9552/

/data/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/test/e2e/framework/framework.go:134
Sep 30 15:55:55.842: Couldn't delete ns: "e2e-tests-limitrange-7yrtq": namespace e2e-tests-limitrange-7yrtq was not deleted with limit: timed out waiting for the condition, namespace is empty but is not yet removed (&errors.errorString{s:"namespace e2e-tests-limitrange-7yrtq was not deleted with limit: timed out waiting for the condition, namespace is empty but is not yet removed"})
/data/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/test/e2e/framework/framework.go:338

[test]

smarterclayton · 2016-09-30T23:59:56Z

Ok, now this is ready for review.

On Sep 30, 2016, at 7:29 PM, OpenShift Bot notifications@github.com wrote:

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

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#11028 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABG_p1B0tSlPmA9ugzumqAo3VduODFveks5qvZtVgaJpZM4KCRxk
.

deads2k · 2016-10-03T12:28:00Z

pkg/authorization/registry/rolebinding/policybased/virtual_storage.go

@@ -248,13 +252,23 @@ func (m *VirtualStorage) updateRoleBinding(ctx kapi.Context, name string, objInf
 	return updatedRoleBinding, false, nil
 }

+// modifyingRoleForEscalation tries to use the CachedRuleResolver if available to avoid expensive checks
+func (m *VirtualStorage) modifyingRoleForEscalation(binding authorizationinterfaces.RoleBinding) (authorizationinterfaces.Role, error) {


I understand the method, but not the name. Looks like I made the name initially, but it sucks.

Preference?

roleForEscalationCheck?

deads2k · 2016-10-03T12:33:41Z

test/integration/authorization_test.go

-	if err := addValerie.AddRole(); !kapierror.IsNotFound(err) {
+	// try to add Valerie to a non-existent role, looping until it is true due to
+	// the policy cache taking time to react
+	if err := wait.Poll(time.Second, 2*time.Minute, func() (bool, error) {


If this is now necessary, it suggests we've gotten slower because the cache on the covers check falls through to live on a miss.

no, this is waiting for the cache to reflect the role deletion above (before it would have failed the live check immediately). 2 minutes seems egregious, though

I've seen integration tests pause for 30-60 seconds.

smarterclayton · 2016-10-03T21:10:56Z

AWS broke [test]

On Mon, Oct 3, 2016 at 2:02 PM, OpenShift Bot notifications@github.com
wrote:

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

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#11028 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_py--2Q-w3wio-4a-lU3fW-gK8SCEks5qwUM7gaJpZM4KCRxk
.

smarterclayton · 2016-10-06T03:35:17Z

Finish your review! Or I'll turn on ReallyCrash

On Oct 5, 2016, at 9:17 PM, OpenShift Bot notifications@github.com wrote:

Origin Action Required: Pull request cannot be automatically merged, please
rebase your branch from latest HEAD and push again

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#11028 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABG_p8BG7joaVBJW3-_dxV9xWFdOSItIks5qxGg7gaJpZM4KCRxk
.

deads2k · 2016-10-07T15:49:56Z

Do you have two of these? I swear I said this looked good.

smarterclayton · 2016-10-08T00:53:39Z

Hrm, never saw it. Bizarre.

On Oct 7, 2016, at 9:50 AM, David Eads notifications@github.com wrote:

Do you have two of these? I swear I said this looked good.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#11028 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABG_p-Vio-cHJzpmY6GhLSVsEeq3oQRcks5qxmongaJpZM4KCRxk
.

Mostly consistent with upstream - did not remove the reason field

Significantly reduces the impact of allocations during policy alteration

Only in certain cases do we need to default the rule.

openshift-bot · 2016-10-09T00:37:16Z

Evaluated for origin test up to 9ea0477

smarterclayton · 2016-10-09T00:41:03Z

Updated and rebased on top of the change to all the same method signatures...

openshift-bot · 2016-10-09T02:55:38Z

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

smarterclayton · 2016-10-09T03:32:07Z

[merge]

On Sat, Oct 8, 2016 at 10:55 PM, OpenShift Bot notifications@github.com
wrote:

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

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#11028 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p7RKk3doEZnNMzc18GZf35X-6fgIks5qyFesgaJpZM4KCRxk
.

openshift-bot · 2016-10-09T03:32:21Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9782/) (Image: devenv-rhel7_5156)

openshift-bot · 2016-10-09T03:32:21Z

Evaluated for origin merge up to 9ea0477

deads2k reviewed Sep 21, 2016

View reviewed changes

smarterclayton force-pushed the reduce_covers branch from 6bd0732 to 9d23492 Compare September 27, 2016 20:46

smarterclayton force-pushed the reduce_covers branch 2 times, most recently from 0680d71 to da79a0b Compare September 29, 2016 03:16

smarterclayton force-pushed the reduce_covers branch 4 times, most recently from 2e65fc9 to befa107 Compare September 30, 2016 18:55

deads2k reviewed Oct 3, 2016

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2016

Collapse local and cluster PolicyRule retrieval

daf2567

Mostly consistent with upstream - did not remove the reason field

smarterclayton force-pushed the reduce_covers branch from 9413e61 to 9ea0477 Compare October 9, 2016 00:36

smarterclayton added 3 commits October 8, 2016 17:36

Try a cached RuleResolver prior to the underlying in a few cases

9d3884a

Significantly reduces the impact of allocations during policy alteration

Avoid doing a naive DeepEquals on all PolicyRules

9dd4fbd

Only in certain cases do we need to default the rule.

Test case must handle caches now

9ea0477

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2016

openshift-bot merged commit 6c5d083 into openshift:master Oct 9, 2016

enj mentioned this pull request Oct 13, 2016

role-reapers test flake #11024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the amount of allocations in policy use by trying a cached resolver first #11028

Reduce the amount of allocations in policy use by trying a cached resolver first #11028

smarterclayton commented Sep 21, 2016

smarterclayton commented Sep 21, 2016

deads2k Sep 21, 2016

deads2k Sep 21, 2016

deads2k commented Sep 21, 2016

smarterclayton commented Sep 27, 2016

smarterclayton commented Sep 29, 2016 via email

smarterclayton commented Sep 29, 2016

smarterclayton commented Sep 30, 2016

smarterclayton commented Sep 30, 2016

deads2k Oct 3, 2016

smarterclayton Oct 3, 2016

deads2k Oct 3, 2016

deads2k Oct 3, 2016

liggitt Oct 3, 2016

smarterclayton Oct 3, 2016

smarterclayton commented Oct 3, 2016

smarterclayton commented Oct 6, 2016

deads2k commented Oct 7, 2016

smarterclayton commented Oct 8, 2016

openshift-bot commented Oct 9, 2016

smarterclayton commented Oct 9, 2016

openshift-bot commented Oct 9, 2016

smarterclayton commented Oct 9, 2016

openshift-bot commented Oct 9, 2016 •

edited

Loading

openshift-bot commented Oct 9, 2016

Reduce the amount of allocations in policy use by trying a cached resolver first #11028

Reduce the amount of allocations in policy use by trying a cached resolver first #11028

Conversation

smarterclayton commented Sep 21, 2016

smarterclayton commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 21, 2016

smarterclayton commented Sep 27, 2016

smarterclayton commented Sep 29, 2016 via email

smarterclayton commented Sep 29, 2016

smarterclayton commented Sep 30, 2016

smarterclayton commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Oct 3, 2016

smarterclayton commented Oct 6, 2016

deads2k commented Oct 7, 2016

smarterclayton commented Oct 8, 2016

openshift-bot commented Oct 9, 2016

smarterclayton commented Oct 9, 2016

openshift-bot commented Oct 9, 2016

smarterclayton commented Oct 9, 2016

openshift-bot commented Oct 9, 2016 • edited Loading

openshift-bot commented Oct 9, 2016

openshift-bot commented Oct 9, 2016 •

edited

Loading