etcd: extend rate limiting to consider the number of inflight requests #25817

giorio94 · 2023-06-01T09:45:43Z

Reporting the description of the last commit for convenience:

Currently, the etcd rate limiter operates based on a token bucket of a
fixed size. Yet, this is problematic when etcd is overloaded, since we
may continue to issue new requests even if it cannot keep up. Hence,
let's start using the custom APILimiter, which also takes into account
the number of currently inflight requests. The maximum amount of
inflight requests can configured through the etcd.maxInflight parameter,
and defaults to the same value as etcd.qps if unset.

etcd: extend rate limiting to consider the number of inflight requests

giorio94 · 2023-06-01T09:48:28Z

/test

marseel · 2023-06-01T11:23:56Z

Related Etcd issue: etcd-io/etcd#15993

pkg/kvstore/etcd.go

giorio94 · 2023-06-07T11:59:57Z

pkg/kvstore/etcd.go

@@ -1162,7 +1214,6 @@ func (e *etcdClient) determineEndpointStatus(ctx context.Context, endpointAddres

 	e.getLogger().Debugf("Checking status to etcd endpoint %s", endpointAddress)

-	e.limiter.Wait(ctxTimeout)


I've dropped the rate limiter wait here, to prevent that the status check fails due to rate limiting, as that would put a high pressure on etcd in case the connection gets restarted.

giorio94 · 2023-06-07T13:08:58Z

The last push added the support to increase the metrics when the rate limit wait process fails.

giorio94 · 2023-06-07T13:37:51Z

Rebased onto main to pick the CI fixes. I've additionally added a new commit to assign pkg/rate to @cilium/sig-agent and pkg/rate/metrics to @cilium/metrics (please let me know if a different team would fit better). Pulling in @cilium/metrics for review given the above.

giorio94 · 2023-06-07T13:41:57Z

/test

pkg/kvstore/etcd.go

marseel · 2023-06-07T14:12:08Z

pkg/kvstore/etcd.go

-	e.limiter.Wait(ctx)
+	lr, err := e.limiter.Wait(ctx)
+	if err != nil {
+		increaseMetric(key, metricRead, "GetLocked", duration.EndError(err).Total(), err)


Can we do defer increaseMetric(key, metricRead, "GetLocked", duration.EndError(err).Total(), err) two lines above (not inside if) and remove another increaseMetric below?

We cannot with the current increaseMetric implementation, since duration.EndError(err).Total() would be executed immediately, hence not counting correctly the duration. It should work modifying increaseMetric to take the span as input (rather than the pre-computed duration), but the change tends to be on the large side. WDYT?

ah yea, we could do that in separate PR.

I'm giving it a try. If it ends up being too large I'll open a separate PR

I've updated the PR adding one more commit which does the increaseMetrics deferring, to avoid having to repeat that in case of errors. @marseel PTAL

pkg/kvstore/etcd.go

marseel

lgtm except for small nits.

giorio94 · 2023-06-08T06:47:17Z

/test

This commit extracts the APILimiterObserver implementation from the daemon package to a separate one, to allow for its reuse. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

Let's assign the newly introduced `pkg/rate/metrics` folder to the @cilium/metrics team. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

This commit slightly refactors the approach adopted to increase the kvstore metrics, deferring this operation at the beginning of each function (similarly to how tracing is handled). This allows to transparently handle early returns due to errors. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

Currently, the etcd rate limiter operates based on a token bucket of a fixed size. Yet, this is problematic when etcd is overloaded, since we may continue to issue new requests even if it cannot keep up. Hence, let's start using the custom APILimiter, which also takes into account the number of currently inflight requests. The maximum amount of inflight requests can configured through the etcd.maxInflight parameter, and defaults to the same value as etcd.qps if unset. The rate limiter wait is removed when checking the status of the etcd endpoints, to prevent that the check fails due to rate limiting, as that would put a high pressure on etcd in case the connection gets restarted. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

giorio94 · 2023-06-08T12:32:01Z

Rebased onto main to fix conflicts

giorio94 · 2023-06-08T12:32:05Z

/test

giorio94 · 2023-06-09T06:53:31Z

/ci-awscni

giorio94 · 2023-06-09T06:53:37Z

/ci-eks

giorio94 added kind/performance There is a performance impact of this. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/kvstore Impacts the KVStore package interactions. labels Jun 1, 2023

giorio94 requested review from a team as code owners June 1, 2023 09:45

giorio94 requested review from tklauser, YutaroHayakawa and marseel June 1, 2023 09:45

marseel mentioned this pull request Jun 1, 2023

Clustermesh-apiserver: CiliumEndpoint out of order deletions leads to dropping packets for valid network policies #25562

Closed

2 tasks

marseel reviewed Jun 1, 2023

View reviewed changes

pkg/kvstore/etcd.go Show resolved Hide resolved

oblazek reviewed Jun 1, 2023

View reviewed changes

pkg/kvstore/etcd.go Outdated Show resolved Hide resolved

giorio94 force-pushed the mio/etcd-max-inflight branch from 2d44c0b to 5d74763 Compare June 1, 2023 14:00

YutaroHayakawa approved these changes Jun 2, 2023

View reviewed changes

giorio94 force-pushed the mio/etcd-max-inflight branch from 5d74763 to 496ca38 Compare June 7, 2023 11:58

giorio94 commented Jun 7, 2023

View reviewed changes

giorio94 force-pushed the mio/etcd-max-inflight branch from 496ca38 to 2dcb6ca Compare June 7, 2023 13:08

giorio94 force-pushed the mio/etcd-max-inflight branch from 2dcb6ca to 18b4f0d Compare June 7, 2023 13:35

giorio94 requested a review from a team as a code owner June 7, 2023 13:35

giorio94 requested review from a team and chancez and removed request for a team June 7, 2023 13:38

marseel reviewed Jun 7, 2023

View reviewed changes

pkg/kvstore/etcd.go Outdated Show resolved Hide resolved

marseel reviewed Jun 7, 2023

View reviewed changes

pkg/kvstore/etcd.go Outdated Show resolved Hide resolved

marseel reviewed Jun 7, 2023

View reviewed changes

pkg/kvstore/etcd.go Outdated Show resolved Hide resolved

marseel reviewed Jun 7, 2023

View reviewed changes

pkg/kvstore/etcd.go Outdated Show resolved Hide resolved

marseel reviewed Jun 7, 2023

View reviewed changes

pkg/kvstore/etcd.go Outdated Show resolved Hide resolved

marseel approved these changes Jun 7, 2023

View reviewed changes

giorio94 marked this pull request as draft June 7, 2023 15:08

chancez approved these changes Jun 7, 2023

View reviewed changes

giorio94 force-pushed the mio/etcd-max-inflight branch from 18b4f0d to 2f2bc63 Compare June 8, 2023 06:45

giorio94 marked this pull request as ready for review June 8, 2023 06:47

tklauser approved these changes Jun 8, 2023

View reviewed changes

giorio94 added 4 commits June 8, 2023 14:16

daemon: extract APILimiterObserver implementation to separate package

c35ff82

This commit extracts the APILimiterObserver implementation from the daemon package to a separate one, to allow for its reuse. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

CODEOWNERS: assign pkg/rate/metrics to @cilium/metrics

2856340

Let's assign the newly introduced `pkg/rate/metrics` folder to the @cilium/metrics team. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

giorio94 force-pushed the mio/etcd-max-inflight branch from 2f2bc63 to e84ba5b Compare June 8, 2023 12:31

michi-covalent approved these changes Jun 8, 2023

View reviewed changes

This was referenced Jun 9, 2023

CI: Conformance EKS - Run connectivity test with IPSec - pod-to-cidr #26063

Closed

CI: Conformance AWS-CNI - pod-to-cird #26064

Closed

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 9, 2023

dylandreimerink merged commit 7908f20 into cilium:main Jun 9, 2023
63 of 64 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcd: extend rate limiting to consider the number of inflight requests #25817

etcd: extend rate limiting to consider the number of inflight requests #25817

giorio94 commented Jun 1, 2023

giorio94 commented Jun 1, 2023

marseel commented Jun 1, 2023

giorio94 Jun 7, 2023

giorio94 commented Jun 7, 2023

giorio94 commented Jun 7, 2023 •

edited

giorio94 commented Jun 7, 2023

marseel Jun 7, 2023

giorio94 Jun 7, 2023

marseel Jun 7, 2023

giorio94 Jun 7, 2023

giorio94 Jun 8, 2023

marseel left a comment

giorio94 commented Jun 8, 2023

giorio94 commented Jun 8, 2023

giorio94 commented Jun 8, 2023

giorio94 commented Jun 9, 2023

giorio94 commented Jun 9, 2023

		@@ -1162,7 +1214,6 @@ func (e *etcdClient) determineEndpointStatus(ctx context.Context, endpointAddres

		e.getLogger().Debugf("Checking status to etcd endpoint %s", endpointAddress)

		e.limiter.Wait(ctxTimeout)

etcd: extend rate limiting to consider the number of inflight requests #25817

etcd: extend rate limiting to consider the number of inflight requests #25817

Conversation

giorio94 commented Jun 1, 2023

giorio94 commented Jun 1, 2023

marseel commented Jun 1, 2023

giorio94 Jun 7, 2023

Choose a reason for hiding this comment

giorio94 commented Jun 7, 2023

giorio94 commented Jun 7, 2023 • edited

giorio94 commented Jun 7, 2023

marseel Jun 7, 2023

Choose a reason for hiding this comment

giorio94 Jun 7, 2023

Choose a reason for hiding this comment

marseel Jun 7, 2023

Choose a reason for hiding this comment

giorio94 Jun 7, 2023

Choose a reason for hiding this comment

giorio94 Jun 8, 2023

Choose a reason for hiding this comment

marseel left a comment

Choose a reason for hiding this comment

giorio94 commented Jun 8, 2023

giorio94 commented Jun 8, 2023

giorio94 commented Jun 8, 2023

giorio94 commented Jun 9, 2023

giorio94 commented Jun 9, 2023

giorio94 commented Jun 7, 2023 •

edited