Skip to content

Conversation

nebril
Copy link
Member

@nebril nebril commented Apr 27, 2022

This commit does the following:

  • Adds a configuration option for controlling the concurrency of the DNS
    proxy
  • Adds a configuration option for the semaphore (from above) timeout
  • Exposes an additional metric for the time taken to perform the policy
    check on a DNS request within the DNS proxy

The concurrency limitation is done by introducing a semaphore to DNS
proxy. By default, no such limit is imposed.

Users are advised to take into account the number of DNS requests[1] and
how many CPUs on each node in their cluster in order to come up with an
appropriate concurrency limit.

In addition, we expose the semaphore grace period as a configurable
option. Assuming the "right" for this timeout is a tradeoff that we
shouldn't really assume for the user.

The semaphore grace period is to prevent the situation where Cilium
deadlocks or consistently high rate of DNS traffic causing Cilium to be
unable to keep up.

See #19543 (comment) by
joe@cilium.io.

The user can take into account the rate that they expect DNS requests to
be following into Cilium and how many of those requests should be
processed without retrying. If retrying isn't an issue then keeping the
grace period at 0 (default) will immediately free the goroutine handling
the DNS request if the semaphore acquire fails. Conversely, if a backlog
of "unproductive" goroutines is acceptable (and DNS request retries are
not), then setting the grace period is advisable. This gives the
goroutines some time to acquire the semaphore. Goroutines could pile up
if the grace period is too high and there's a consistently high rate of
DNS requests.

It's worth noting that blindly increasing the concurrency limit will not
linearly improve performance. It might actually degrade instead due to
internal downstream lock contention (as seen by the recent commits to
move Endpoint-related functions to use read-locks).

Ultimately, it becomes a tradeoff between high number of semaphore
timeouts (dropped DNS requests that must be retried) or high number of
(unproductive) goroutines, which can consume system resources.

[1]: The metric to monitor is

cilium_policy_l7_total{rule="received"}

Co-authored-by: Chris Tarazi chris@isovalent.com
Signed-off-by: Maciej Kwiek maciej@isovalent.com
Signed-off-by: Chris Tarazi chris@isovalent.com

@nebril nebril added kind/performance There is a performance impact of this. needs-backport/1.10 labels Apr 27, 2022
@nebril nebril requested a review from a team April 27, 2022 13:34
@nebril nebril requested a review from a team as a code owner April 27, 2022 13:34
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 27, 2022
@nebril nebril requested review from jibi and michi-covalent April 27, 2022 13:34
@nebril
Copy link
Member Author

nebril commented Apr 27, 2022

/test

@ciliumbot
Copy link

Build finished.

@nebril nebril force-pushed the pr/nebril/dnproxy-concurrency-master branch from 8dfc0be to d400e05 Compare April 28, 2022 09:53
@nebril
Copy link
Member Author

nebril commented Apr 28, 2022

/test

@joestringer joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 29, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 29, 2022
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got confused about the multiple versions of this PR out, so I put my feedback on the wrong version. See #19543 (review) for more details.

Let's get this into the master tree first in the form that is acceptable upstream, then arrange the backports to line up the same.

@christarazi christarazi force-pushed the pr/nebril/dnproxy-concurrency-master branch 3 times, most recently from 971e76b to 29e451b Compare May 3, 2022 22:21
@christarazi christarazi force-pushed the pr/nebril/dnproxy-concurrency-master branch from 29e451b to 1f81231 Compare May 3, 2022 22:50
@christarazi
Copy link
Member

/test

@christarazi
Copy link
Member

christarazi commented May 4, 2022

Noting that all of CI has passed, except for the runtime test due to legitimate failure. See diff for fix.

@christarazi christarazi force-pushed the pr/nebril/dnproxy-concurrency-master branch from 1f81231 to 8551838 Compare May 4, 2022 18:23
@christarazi
Copy link
Member

/test-runtime

@christarazi christarazi force-pushed the pr/nebril/dnproxy-concurrency-master branch from 8551838 to 48fbece Compare May 5, 2022 20:57
@christarazi christarazi requested a review from joestringer May 10, 2022 06:24
@christarazi christarazi force-pushed the pr/nebril/dnproxy-concurrency-master branch from c1bc016 to 75c328c Compare May 10, 2022 16:54
@christarazi
Copy link
Member

/test

@christarazi christarazi force-pushed the pr/nebril/dnproxy-concurrency-master branch from 75c328c to d865d64 Compare May 10, 2022 17:30
@christarazi
Copy link
Member

/test

@christarazi christarazi force-pushed the pr/nebril/dnproxy-concurrency-master branch from d865d64 to f6a33ae Compare May 10, 2022 18:54
@christarazi
Copy link
Member

/test

This commit does the following:

* Adds a configuration option for controlling the concurrency of the DNS
  proxy
* Adds a configuration option for the semaphore (from above) timeout
* Exposes an additional metric for the time taken to perform the policy
  check on a DNS request within the DNS proxy

The concurrency limitation is done by introducing a semaphore to DNS
proxy. By default, no such limit is imposed.

Users are advised to take into account the number of DNS requests[1] and
how many CPUs on each node in their cluster in order to come up with an
appropriate concurrency limit.

In addition, we expose the semaphore grace period as a configurable
option. Assuming the "right" for this timeout is a tradeoff that we
shouldn't really assume for the user.

The semaphore grace period is to prevent the situation where Cilium
deadlocks or consistently high rate of DNS traffic causing Cilium to be
unable to keep up.

See cilium#19543 (comment) by
<joe@cilium.io>.

The user can take into account the rate that they expect DNS requests to
be following into Cilium and how many of those requests should be
processed without retrying. If retrying isn't an issue then keeping the
grace period at 0 (default) will immediately free the goroutine handling
the DNS request if the semaphore acquire fails. Conversely, if a backlog
of "unproductive" goroutines is acceptable (and DNS request retries are
not), then setting the grace period is advisable. This gives the
goroutines some time to acquire the semaphore. Goroutines could pile up
if the grace period is too high and there's a consistently high rate of
DNS requests.

It's worth noting that blindly increasing the concurrency limit will not
linearly improve performance. It might actually degrade instead due to
internal downstream lock contention (as seen by the recent commits to
move Endpoint-related functions to use read-locks).

Ultimately, it becomes a tradeoff between high number of semaphore
timeouts (dropped DNS requests that must be retried) or high number of
(unproductive) goroutines, which can consume system resources.

[1]: The metric to monitor is

```
cilium_policy_l7_total{rule="received"}
```

Co-authored-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
Copy link
Member

CI passed except for legit error in runtime test.

@christarazi christarazi force-pushed the pr/nebril/dnproxy-concurrency-master branch from f6a33ae to 866d91c Compare May 10, 2022 21:56
@christarazi
Copy link
Member

/test-runtime

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. kind/performance There is a performance impact of this. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants