Two separate CpuBoundWork semaphores – HTTP | JSONRPC

[Al2Klimov]

Is your feature request related to a problem? Please describe.

One of our customers is DDoSing their Icinga 2 via /v1/hosts (a lot of hosts btw.).
That locks up cluster communication entirely.

Describe the solution you'd like

The separate HTTP CpuBoundWork gets e.g. 1 slot or 1/2 of all so that it doesn't bother the cluster.

Describe alternatives you've considered

Put actual processing of HTTP requests in a queue.

Additional context

ref/IP/48306

[Al2Klimov]

Describe alternatives you've considered

Put actual processing of HTTP requests in a queue.

• Actually the standard thread pool would be nice, the coroutine could suspend itself during processing, just like in CpuBoundWork#CpuBoundWork(): don't spin on atomic int to acquire slot #9990

[Al2Klimov]

Maybe Icinga DB even deserves its own I/O, isolated from everything else. So that it doesn't influence cluster communication throughput.

[Al2Klimov]

Due to the same construction area and especially an expected shortage of slots, this should be built on top of (i.e. after):

• CpuBoundWork#CpuBoundWork(): don't spin on atomic int to acquire slot #9990
  • CpuBoundWork#CpuBoundWork(): don't spin on atomic int to acquire slot #9990 (comment) (The main disagreement on the implementation, matters here)

Once done, stuff here will be simple:

• Two slotsets and queues instead of one (kind of copy&paste)
• CpuBoundWork becomes an abstract base class for two derived ones:
  • HttpCpuBoundWork
  • ClusterCpuBoundWork
  • They just have to provide protected methods which return the slotset/queue to operate on

At least after the mentioned here #9990 (comment), the main question here is how many of the existing slots to delegate to HTTP. I'd say 1/8 with a minimum of one – just cause. Can of course be refined further.

[jschmidt-icinga]

Isn't the actual problem here that in case of HTTP we acquire this semaphore once and keep it locked until the entire request is processed? With #10516 that could technically be avoided. In the case of ObjectQueryHandler for example, the PR uses the new ability of the JsonEncoder to take a generator function (added in #10414) to serialize and write out the response object by object. So technically we could introduce breaks to the scope of the semaphore there and multiplex this kind of CPU-bound work, with the only consequence of too many requests being that the latency goes down up, but that's something the user should expect and work around on their side.

However, due to this generator-function approach, and with the actual IO being done inside JsonEncoder (I wasn't a big fan of this), acquiring an IoBoundWorkSlot isn't really possible here, because it can't know if it's in a scope that has acquired a CpuBoundWork semaphore (which highlights that IoBoundWorkSlot is a crutch at best, that weakens the RAII of the CpuBoundWork).

So instead, we could break up the CpuBoundWork. And since the ObjectQueryHandler treatment could be given to all handlers that potentially take a lot of work or iterate and lock many objects, I'd propose (once #10516 is merged) to move acquiring the semaphore down into the handlers, and only do it where the actual work is done. This would also remove the need for IoBoundWorkSlot entirely, since fully IO-bound handlers like EventsHandler could just never acquire a CpuBoundWork semaphore.

Obviously, with much more acquiring/releasing going on, this would benefit even more from making the semaphore as efficient as possible (i.e. the other PRs I have yet to look into).

[Al2Klimov]

remove the need for IoBoundWorkSlot entirely

making the semaphore as efficient as possible (i.e. the other PRs

• Sounds like CpuBoundWork#CpuBoundWork(): don't spin on atomic int to acquire slot #9990

[jschmidt-icinga]

Yes, that's what I meant by "other PRs", as it's referenced in the comment right above mine. I didn't see that it already removes IoBoundWorkSlot, that's great. However, unlocking the semaphore in StartStreaming() would need to be reworked or dropped, either in #9990 or #10516, depending on which is merged first.