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

back project cache with local authorizer #5760

Merged
merged 1 commit into from
Nov 7, 2015

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Nov 6, 2015

Fixes #5737

@deads2k
Copy link
Contributor Author

deads2k commented Nov 6, 2015

I did not do a very controlled check, but creating a bunch of projects by hand after this change looks like
pprof cpu-20151106122038

as opposed to the picture in the issue which I reproduced before.

@deads2k
Copy link
Contributor Author

deads2k commented Nov 6, 2015

@liggitt This eliminates half of all server requests in @ironcladlou's runs

@ironcladlou
Copy link
Contributor

LGTM, and confirmed as being enough to resolve #5737

@liggitt
Copy link
Contributor

liggitt commented Nov 6, 2015

[test]

@liggitt liggitt added this to the 1.1.0 milestone Nov 6, 2015
@liggitt
Copy link
Contributor

liggitt commented Nov 6, 2015

adding to the milestone while we consider it

@smarterclayton
Copy link
Contributor

Do we know what types don't have conversions?

@smarterclayton
Copy link
Contributor

This is LGTM, and looks as simple as possible. @liggitt agree this is low risk high payoff?

@smarterclayton
Copy link
Contributor

[test]

@liggitt
Copy link
Contributor

liggitt commented Nov 7, 2015

LGTM. This backs the project cache with the same authorizer that is used to satisfy the localresourceaccessreview request submitted previously.

The only difference between the localresourceaccessreview call and this impl is that an empty namespace was explicitly disallowed when processing a localresourceaccessreview, but I verified that

  1. all callers of Review() set a namespace
  2. this implementation of Review() does the right thing when presented with an empty namespace (returns the list of users and groups that can get all namespaces)

@liggitt liggitt added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2015
@smarterclayton smarterclayton added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2015
@liggitt liggitt self-assigned this Nov 7, 2015
@smarterclayton
Copy link
Contributor

[merge], although this may result in test flakes due to improved performance, so I'll be watching for them today (this does not result in goroutine scheduling like previous flow did)

@openshift-bot
Copy link
Contributor

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

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 16d6c23

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6993/) (Extended Tests: core)

@smarterclayton
Copy link
Contributor

Error from server: unable to find api field in struct BuildConfig for the json field "metadata"
!!! Error in test/cmd/builds.sh:32
    'oc patch bc/ruby-sample-build -p "{\"spec\":{\"output\":{\"to\":{\"name\":\"${REAL_OUTPUT_TO}\"}}}}"' exited with status 1
Call stack:
    1: test/cmd/builds.sh:32 main(...)
Exiting with status 1
!!! Error in hack/test-cmd.sh:281
    '${test}' exited with status 1
Call stack:
    1: hack/test-cmd.sh:281 main(...)
Exiting with status 1

I have trouble seeing how this is related - an ACL race wouldn't look like this.

@smarterclayton
Copy link
Contributor

[test]

@smarterclayton
Copy link
Contributor

Worried that we're seeing a lot of flakes here, but none of them look attributable so far. Testing more

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 16d6c23

openshift-bot pushed a commit that referenced this pull request Nov 7, 2015
Merged by openshift-bot
@openshift-bot openshift-bot merged commit bee82d4 into openshift:master Nov 7, 2015
@deads2k deads2k deleted the 5737 branch February 26, 2016 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/performance lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants