Its bad to spawn a gofunc per quota with large number of quotas

[derekwaynecarr]

Scenario:

1. Create 500 namespaces
2. Create a quota per namespace
3. Wait

Result:
System crashes

Cause:
We had a legacy pattern of creating a gofunc per quota during every synch period.

Fix:
Process quota updates in sequence during synch loop.

Related issue:
openshift/origin#3126

Future enhancements:
I have other optimization strategies in mind, but this stops bleeding and stabilizes the system.

/cc @smarterclayton

[thockin]

Is this saying that 500 goroutines is too much? Because that is a problem -
go claims tens of thousands of goroutines should be fine..
On Jun 12, 2015 8:49 AM, "Derek Carr" notifications@github.com wrote:

Scenario:

1. Create 500 namespaces
2. Create a quota per namespace
3. Wait

Result:
System crashes

Cause:
We had a legacy pattern of creating a gofunc per quota during every synch
period.

Fix:
Process quota updates in sequence during synch loop.

Related issue:
openshift/origin#3126 openshift/origin#3126

Future enhancements:
I have other optimization strategies in mind, but this stops bleeding and
stabilizes the system.

/cc @smarterclayton https://github.com/smarterclayton

You can view, comment on, or merge this pull request online at:

#9716
Commit Summary

• Its bad to spawn a gofunc per quota with large number of quotas

File Changes

• M pkg/resourcequota/resource_quota_controller.go
  https://github.com/GoogleCloudPlatform/kubernetes/pull/9716/files#diff-0
  (17)

Patch Links:

• https://github.com/GoogleCloudPlatform/kubernetes/pull/9716.patch
• https://github.com/GoogleCloudPlatform/kubernetes/pull/9716.diff

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

[derekwaynecarr]

@thockin - I think the problem was less the gofunc, and more what each gofunc did concurrently in action. so in this case, I had 500 gofuncs using a shared client pegging the apiserver concurrently, and it caused etcd to report too many open files (which crashed the api server and that is a separate issue we need to fix). there was no need for that much concurrency for a background synch task.

[JasonGiedymin]

@thockin Unless there is some blocking going on which would eventually pile up.

[derekwaynecarr]

To elaborate:

 E0612 02:25:55.618495 18587 errors.go:66] apiserver received an error that is not an api.Status: 501: All the given peers are not reachable Get https://10.0.2.15:4001/v2/keys/kubernetes.io/pods/project-402?quorum=false&recursive=true&sorted=true: dial tcp 10.0.2.15:4001: too many open files]

This was able to happen in that scenario, which crashed our process hard. So I also need to track that status response from etcd causing us problems.

Either way, this PR is a must-have in my opinion for systems that make heavier use of namespaces that may have a quota.

[derekwaynecarr]

A good future e2e test case is to spin up a large number of gofuncs that use a client to peg the api server and ensure it and etcd lives, but my current focus is making sure that I can grow number of items in OpenShift without awful consequences, so I will need to write that test probably next week.

[smarterclayton]

Also, I think because of the pile up, we open a gofunc per controller loop, which will be rate controlled by the apiserver, which then causes clients to back off and retry. However, those gofuncs may pile up waiting for rate control to let them in, which means that the next time the sync loop fires, some of the gofuncs are still around, which means the total limit goes up. Each of those requests creates a TCP socket to the server, which steals files from the backend. If you're running these in the same process it's more of an issue than when you are spread across processes.

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

To elaborate:

 E0612 02:25:55.618495 18587 errors.go:66] apiserver received an error that
 is not an api.Status: 501: All the given peers are not reachable Get
 https://10.0.2.15:4001/v2/keys/kubernetes.io/pods/project-402?quorum=false&recursive=true&sorted=true:
 dial tcp 10.0.2.15:4001: too many open files]

This was able to happen in that scenario, which crashed our process hard. So
I also need to track that status response from etcd causing us problems.

Either way, this PR is a must-have in my opinion for systems that make
heavier use of namespaces that may have a quota.

---

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

[k8s-bot]

GCE e2e build/test failed for commit d40ff87.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[smarterclayton]

Nm, we have a wait group here on the multi loop.

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

Also, I think because of the pile up, we open a gofunc per controller loop,
which will be rate controlled by the apiserver, which then causes clients to
back off and retry. However, those gofuncs may pile up waiting for rate
control to let them in, which means that the next time the sync loop fires,
some of the gofuncs are still around, which means the total limit goes up.
Each of those requests creates a TCP socket to the server, which steals
files from the backend. If you're running these in the same process it's
more of an issue than when you are spread across processes.

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

To elaborate:

 E0612 02:25:55.618495 18587 errors.go:66] apiserver received an error that
 is not an api.Status: 501: All the given peers are not reachable Get
 https://10.0.2.15:4001/v2/keys/kubernetes.io/pods/project-402?quorum=false&recursive=true&sorted=true:
 dial tcp 10.0.2.15:4001: too many open files]

This was able to happen in that scenario, which crashed our process hard.
So
I also need to track that status response from etcd causing us problems.

Either way, this PR is a must-have in my opinion for systems that make
heavier use of namespaces that may have a quota.

---

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

[JasonGiedymin]

I do think there will be problems making the code synchronous and possibly the purpose of and name synchronize() is now no longer valid.

[smarterclayton]

Definitely the long term for quota is that it needs to adopt the worker pattern on controller manager.

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

I do think there will be problems making the code synchronous and possibly
the purpose of and name synchronize() is now no longer valid.

---

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

[derekwaynecarr]

The gofuncs are not around on a subsequent fire of synch.

util.RunUntil runs a single goroutine the calls the func, and then sleeps the interval... you cannot get parallel calls into synchronize using current code because the wait group was properly waiting to return from the func.

[derekwaynecarr]

But the wait group was a problem on a single pass through that many quota items.

Either way, I agree with @smarterclayton that this will move to a worker queue model, just not in this PR.

[derekwaynecarr]

@JasonGiedymin - the synchronize name is still accurate since the func does a full sync of quota state when called at the specified interval from the util.Forever block. As noted previously, util.Forever does not make concurrent calls into a function. It just fires the function, blocks to complete, and then sleeps the desired interval (10s), and then fires again, so I think this is fine.

[JasonGiedymin]

@derekwaynecarr +1

[davidopp]

LGTM