Make List Projects authorization aware

[derekwaynecarr]

This PR adds support to to List Projects based on the user's authorization.

[deads2k]

Is there a WeakHashMap construct in Go? That makes a Locker easier to reason about since you never have to worry about deletion logic. Once the references are gone, the object can be GC'ed. Otherwise, you'll need a sort of ref counter prior to delete to protect from multiple GetOrCreates racing with a Delete on a different thread and ending up with different locks on each thread.

[derekwaynecarr]

Have not seen that, but yes, good point on ref counters needed for each lock

Sent from my iPhone

On Feb 11, 2015, at 8:32 AM, David Eads [email protected] wrote:

In pkg/project/auth/locker.go:

•   return keyLock
  

• }
• l.lock.RUnlock()

• // we need to create one
• keyLock = &sync.RWMutex{}

• l.lock.Lock()
• defer l.lock.Unlock()

• l.items[key] = keyLock
• return keyLock
  +}

+// Delete the lock for the specified key
+func (l *locker) Delete(key string) {
Is there a WeakHashMap construct in Go? That makes a Locker easier to reason about since you never have to worry about deletion logic. Once the references are gone, the object can be GC'ed. Otherwise, you'll need a sort of ref counter prior to delete to protect from multiple GetOrCreates racing with a Delete on a different thread and ending up with different locks on each thread.

—
Reply to this email directly or view it on GitHub.

[derekwaynecarr]

Will probably make Delete be a Release func and ensure its called after every GetOrCreate. Will probably rename GetOrCreate to Acquire.

Sent from my iPhone

On Feb 11, 2015, at 8:32 AM, David Eads [email protected] wrote:

In pkg/project/auth/locker.go:

•   return keyLock
  

• }
• l.lock.RUnlock()

• // we need to create one
• keyLock = &sync.RWMutex{}

• l.lock.Lock()
• defer l.lock.Unlock()

• l.items[key] = keyLock
• return keyLock
  +}

+// Delete the lock for the specified key
+func (l *locker) Delete(key string) {
Is there a WeakHashMap construct in Go? That makes a Locker easier to reason about since you never have to worry about deletion logic. Once the references are gone, the object can be GC'ed. Otherwise, you'll need a sort of ref counter prior to delete to protect from multiple GetOrCreates racing with a Delete on a different thread and ending up with different locks on each thread.

—
Reply to this email directly or view it on GitHub.

[deads2k]

Will probably make Delete be a Release func and ensure its called after every GetOrCreate. Will probably rename GetOrCreate to Acquire.

Sounds good.

[deads2k]

Either scope items in the for loop or name it more precisely. The reuse makes it harder to read.

[derekwaynecarr]

Prerequsite PR: #1038

[derekwaynecarr]

This is ready for review.

It cannot be merged until the following are merged:

1. Project is Namespace #1038 - project is namespace
2. ignore setSelfLink errors #1049 - fix resource access reviews after latest rebase.

A user is able to create a project, and not see the project in a list until the sync runs.

/cc @smarterclayton @deads2k @liggitt

[smarterclayton]

For #1049 @deads2k can you add tests to RAR that prevent this from being broken again?

On Feb 17, 2015, at 4:03 PM, Derek Carr [email protected] wrote:

This is ready for review.

It cannot be merged until the following are merged:

1. Project is Namespace #1038 - project is namespace
2. ignore setSelfLink errors #1049 - fix resource access reviews after latest rebase.

A user is able to create a project, and not see the project in a list until the sync runs.

/cc @smarterclayton @deads2k @liggitt

—
Reply to this email directly or view it on GitHub.

[derekwaynecarr]

This works now.

Note:
Due to #1049 , you see the following in the log at startup:

E0218 14:17:21.322616   26099 resthandler.go:351] error generating link: unable to find object fields on reflect.Value{typ:(*reflect.rtype)(0x1159cc0), ptr:(unsafe.Pointer)(0xc208fdd6e0), flag:0xd9}: couldn't find Namespace field in api.TypeMeta{Kind:"", APIVersion:""}
E0218 14:17:21.442787   26099 resthandler.go:351] error generating link: unable to find object fields on reflect.Value{typ:(*reflect.rtype)(0x1159cc0), ptr:(unsafe.Pointer)(0xc2090a9380), flag:0xd9}: couldn't find Namespace field in api.TypeMeta{Kind:"", APIVersion:""}
E0218 14:17:21.578445   26099 resthandler.go:351] error generating link: unable to find object fields on reflect.Value{typ:(*reflect.rtype)(0x1159cc0), ptr:(unsafe.Pointer)(0xc208b72060), flag:0xd9}: couldn't find Namespace field in api.TypeMeta{Kind:"", APIVersion:""}
E0218 14:17:21.703789   26099 resthandler.go:351] error generating link: unable to find object fields on reflect.Value{typ:(*reflect.rtype)(0x1159cc0), ptr:(unsafe.Pointer)(0xc208d3fa40), flag:0xd9}: couldn't find Namespace field in api.TypeMeta{Kind:"", APIVersion:""}

Conceptually, you will see one of those messages whenever you add/modify namespace, policy, binding.

[derekwaynecarr]

Ignore my previous comment on log spams now, the log level for that issue has been reduced.

[deads2k]

gofmt?

[deads2k]

You're indexing by name, but retrieving by UID so no projects ever get returned back. Switching this to GetName() makes it work properly.

[derekwaynecarr]

Will fix that up and try to get a unit test with different values to make sure it doesn't happen again.

[derekwaynecarr]

@deads2k - fixed up. thanks for the heads-up, for some reason thought we were using uid.

[deads2k]

Trouble in paradise. With #971 and 500 hundred projects, I see one CPU fully pegged just idling. Exact same etcd, pull #971 and I see 3% CPU utilization.

[derekwaynecarr]

This has not yet merged, so added a commit to fix the peg CPU, and added my project-spawner script

[derekwaynecarr]

FYI: Fixing that bad loop brought create time to < 4 minutes. It will improve more when @deads2k change goes in to further improve policy. CPU utilization is back down below 1% in steady state.

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/993/) (Image: devenv-fedora_852)

[smarterclayton]

Cache test fails gofmt

[derekwaynecarr]

ugh, gofmt rules change with goversions

[smarterclayton]

Yeah, gofmt on 1.3 for now

----- Original Message -----

ugh, gofmt rules change with goversions

---

Reply to this email directly or view it on GitHub:
#971 (comment)

[derekwaynecarr]

that was a pain to figure out, but should be fixed now.

[openshift-bot]

[Test]ing while waiting on the merge queue

[openshift-bot]

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

[derekwaynecarr]

I forgot to fix my integration test will do that now.

Sent from my iPhone

On Feb 20, 2015, at 5:22 PM, OpenShift Bot [email protected] wrote:

continuous-integration/openshift-jenkins/test Evaluating for testing

—
Reply to this email directly or view it on GitHub.

[derekwaynecarr]

Well, now hack/test-cmd.sh looks to have flaked because it looks to work fine for me locally.

[derekwaynecarr]

[Test]

[derekwaynecarr]

Passed test so re [Merge]

[openshift-bot]

Evaluated for origin up to 3fb1a4d

[smarterclayton]

Looks like the bulk of the performance issue is allocation and the CPU taken by GC - 23GB (33% of CPU) allocated by create project spawner, only 20MB actually allocated at any one time.

Sample below of alloc_total was over 30 seconds while project spawner was running:

(pprof) top15 -cum
100.21MB of 23354.72MB total ( 0.43%)
Dropped 653 nodes (cum <= 116.77MB)
Showing top 15 nodes out of 180 (cum >= 11232.09MB)
      flat  flat%   sum%        cum   cum%
       2MB 0.0086% 0.0086% 18090.55MB 77.46%  github.com/GoogleCloudPlatform/kubernetes/pkg/conversion.(*Scheme).DecodeInto
         0     0% 0.0086% 17960.54MB 76.90%  github.com/GoogleCloudPlatform/kubernetes/pkg/runtime.(*Scheme).DecodeInto
       3MB 0.013% 0.021% 17866.75MB 76.50%  github.com/ghodss/yaml.Unmarshal
    2.50MB 0.011% 0.032% 17404.44MB 74.52%  github.com/ghodss/yaml.yamlToJSON
         0     0% 0.032% 16181.20MB 69.28%  github.com/GoogleCloudPlatform/kubernetes/pkg/tools.(*EtcdHelper).bodyAndExtractObj
         0     0% 0.032% 15924.18MB 68.18%  github.com/GoogleCloudPlatform/kubernetes/pkg/tools.(*EtcdHelper).ExtractObj
   77.21MB  0.33%  0.36% 15721.20MB 67.31%  github.com/GoogleCloudPlatform/kubernetes/pkg/tools.(*EtcdHelper).extractObj
   10.50MB 0.045%  0.41% 15564.69MB 66.64%  gopkg.in/yaml%2ev2.Unmarshal
         0     0%  0.41% 15303.70MB 65.53%  github.com/GoogleCloudPlatform/kubernetes/pkg/registry/generic/etcd.(*Etcd).Get
         0     0%  0.41% 15032.03MB 64.36%  github.com/openshift/origin/pkg/authorization/registry/etcd.(*Etcd).GetPolicy
         0     0%  0.41% 12871.41MB 55.11%  github.com/openshift/origin/pkg/authorization/rulevalidation.(*DefaultRuleResolver).getPolicy
       4MB 0.017%  0.42% 12354.36MB 52.90%  github.com/openshift/origin/pkg/authorization/rulevalidation.(*DefaultRuleResolver).GetRole
       1MB 0.0043%  0.43% 11524.13MB 49.34%  github.com/openshift/origin/pkg/authorization/rulevalidation.(*DefaultRuleResolver).GetEffectivePolicyRules
         0     0%  0.43% 11232.09MB 48.09%  gopkg.in/yaml%2ev2.(*parser).skip
         0     0%  0.43% 11232.09MB 48.09%  gopkg.in/yaml%2ev2.yaml_parser_parse

[smarterclayton]

Also seeing this on startup

E0221 15:23:18.673707    2552 master.go:478] Error creating namespace: &{{ } {master    791232fb-ba07-11e4-a576-7831c1b76042  2015-02-21 15:23:18.67307407 -0500 EST map[] map[]} {} {}} due to request [&{Method:POST URL:https://192.168.1.103:8443/api/v1beta1/namespaces Proto:HTTP/1.1 ProtoMajor:1 ProtoMinor:1 Header:map[] Body:{Reader:} ContentLength:172 TransferEncoding:[] Close:false Host:192.168.1.103:8443 Form:map[] PostForm:map[] MultipartForm:<nil> Trailer:map[] RemoteAddr: RequestURI: TLS:<nil>}] failed (403) 403 Forbidden: Forbidden: "/api/v1beta1/namespaces" denied by default

@deads2k ?

[liggitt]

Probably the loop to ensure the master policy namespace exists failing the first time because bootstrap policy doesn't exist yet

[derekwaynecarr]

Need to flip the ensure call order. I will submit a PR.

Sent from my iPhone

On Feb 21, 2015, at 3:49 PM, Jordan Liggitt [email protected] wrote:

Probably the loop to ensure the master policy namespace exists failing the first time because bootstrap policy doesn't exist yet

—
Reply to this email directly or view it on GitHub.

[smarterclayton]

1GB allocated per second is pretty impressive.

[derekwaynecarr]

We need a realistic workload to actually measure performance of the total system more accurately and make improvements moving forward.

Measuring project and policy in isolation of the resources they hold is problematic. After all, once we start
sticking 2 replication controllers per project, we will start to see completely different issues. Same for 6-10 pods per project, etc.

Let's chat how we can build a go library that can populate data and simulate load (which is really far more read heavy). May want to see if we can build a common core population library with upstream that we can then build around. Populate N namespaces each with X to Y pods, replication controllers, and services. This is a good topic for this weeks Kubernetes meet up as I think many groups building around the project are well served building against an anticipated data set. Once we have something, we should run and report those stats each sprint.

On this topic though, the improvements are obvious and known.

1. We should upgrade etcd to match upstream.
2. We need to measure performance when not embedding etcd in our process.
3. If we have a latest resource version on a cache.Store, we can optimize our synch loop more in steady state to avoid iterating at all.
4. If I can make resource access review calls without invoking a rest client, we can avoid encode/decode cost and associated garbage it generates.
5. If policy stops fetching the same thing from etcd for each request we improve more by avoiding the encode/decode process there and the garbage it collects. Policy should hold frequently accessed items in memory. The global policy and policy bindings seem like an easy candidate. The project auth cache is holding this already so we need to share that data between modules.
6. We need to take a serious look at holding subject to namespace and group to namespace records in a data store, and do an LRU over the data set. Etcd is an option since there is a natural lookup key. This would let us avoid having to warm our cache at process start. Some caution is needed here where I think we should do the previous items first because if we do this first, things may look worse before they get better. I am not sure what to do with namespace review records, it may need the same.

I think 1-5 will get us a long way at getting a better understanding of the real system, but I suspect other issues will come up in other parts of the system at that point.

Time to get hacking and measuring now that we have a working core :)

Sent from my iPhone

On Feb 22, 2015, at 5:13 PM, Clayton Coleman [email protected] wrote:

1GB allocated per second is pretty impressive.

—
Reply to this email directly or view it on GitHub.

[smarterclayton]

----- Original Message -----

We need a realistic workload to actually measure performance of the total
system more accurately and make improvements moving forward.

Measuring project and policy in isolation of the resources they hold is
problematic. After all, once we start
sticking 2 replication controllers per project, we will start to see
completely different issues. Same for 6-10 pods per project, etc.

Let's chat how we can build a go library that can populate data and simulate
load (which is really far more read heavy). May want to see if we can build
a common core population library with upstream that we can then build
around. Populate N namespaces each with X to Y pods, replication
controllers, and services. This is a good topic for this weeks Kubernetes
meet up as I think many groups building around the project are well served
building against an anticipated data set. Once we have something, we should
run and report those stats each sprint.

On this topic though, the improvements are obvious and known.

1. We should upgrade etcd to match upstream.

I don't think we can upgrade until the stability issues folks are seeing are addressed.

1. We need to measure performance when not embedding etcd in our process.

Honestly from the trace this is entirely GC load from allocations in policy. I don't even think we need to look at etcd until we get this down to something more accurate.

1. If we have a latest resource version on a cache.Store, we can optimize
   our synch loop more in steady state to avoid iterating at all.

Don't you know whether you've had a watch passed? High water is one, but any change notification is sufficient.

1. If I can make resource access review calls without invoking a rest
   client, we can avoid encode/decode cost and associated garbage it generates.

The evaluation here definitely needs to start at the lower levels.

1. Is it the minimal set of calls being made to calculate the request
2. As you note in 5, what are the core things we can cache
3. Are we doing more copies than we should for other reasons (i.e. are we using map[string]Type instead of map[string]*Type).

1. If policy stops fetching the same thing from etcd for each request we
   improve more by avoiding the encode/decode process there and the garbage it
   collects. Policy should hold frequently accessed items in memory. The
   global policy and policy bindings seem like an easy candidate. The project
   auth cache is holding this already so we need to share that data between
   modules.

I think this is probably the first item.

1. We need to take a serious look at holding subject to namespace and group
   to namespace records in a data store, and do an LRU over the data set.
   Etcd is an option since there is a natural lookup key. This would let us
   avoid having to warm our cache at process start. Some caution is needed
   here where I think we should do the previous items first because if we do
   this first, things may look worse before they get better. I am not sure
   what to do with namespace review records, it may need the same.

I agree, this is last.

I think 1-5 will get us a long way at getting a better understanding of the
real system, but I suspect other issues will come up in other parts of the
system at that point.

Time to get hacking and measuring now that we have a working core :)

Like I said, generating 1GB of garbage is an achievement. Try that in Ruby...