Operator/CES: Simplify rate limit configuration #32523
Conversation
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
thorn3r
force-pushed
the
cesRateLimit
branch
3 times, most recently
from
18d7d1a
to
39d7592
Compare
|
/test |
thorn3r
added
the
release-note/minor
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
|
/test |
|
/test |
|
/test |
thorn3r
force-pushed
the
cesRateLimit
branch
2 times, most recently
from
dda13df
to
cb4443c
Compare
|
oof would help if I remembered to run the make targets in the correct order 😅 fixed a comment in values.yaml to avoid flagging |
Ah, I see. I wasn't considering that this is only affecting a beta feature.
If the options are for a beta feature and not documented outside of the cmdref I'd say it's probably okay to just deprecate the Helm values and replace the options. But I'm not entirely sure of the policy in that regard, so might be good to confirm with maintainers and/or in the #development channel on slack. |
|
/test |
There was a problem hiding this comment.
Nice work, always in favor of simplifying the Cilium's configuration!
I've left a few suggestions inline, mainly about the possibility of making the default settings more explicit, further removing the multiplier setting, and clarifying the semantic around the deprecated wording.
thorn3r
force-pushed
the
cesRateLimit
branch
3 times, most recently
from
e5b8f11
to
2970589
Compare
|
/test |
thorn3r
force-pushed
the
cesRateLimit
branch
2 times, most recently
from
2ccd753
to
520bf68
Compare
|
/test |
|
Changed the config parsing to use a decoder and disallow unknown fields to avoid unexpected behavior if a user enters a typo and the intended field remains 0. Added a short test for this as well. |
Cilium Operator has 6 flags related to configuring the client work queue
rate-limiter for the CiliumEndpointSlice controller. This commit
deprecates the existing flags and adds a single new flag for supplying a
custom dynamic rate limit configuration. The following flags are now deprecated
and will be removed in a future release:
```
--ces-write-qps-limit
--ces-write-qps-burst
--ces-enable-dynamic-rate-limit
--ces-dynamic-rate-limit-nodes
--ces-dynamic-rate-limit-qps-limit
--ces-dynamic-rate-limit-qps-burst
```
The `enableCiliumEndpointSlice` field in the helm values
has been deprecated and replaced by `ciliumEndpointSlice.enabled`.
`enableCiliumEndpointSlice` will be removed in a future release.
The static rate limit options have been removed in favor of always using
a dynamic rate limit. The default rate limits defined in
`ciliumEndpointSlice.rateLimits` should be sufficient
for most clusters, however a custom configuration can be supplied for
specific use-cases.
The new flag 'ces-rate-limits' accepts a string containing
a JSON list of 'rateLimit' objects, each containing a limit and burst
setting, and the number of nodes at which the rate limit should be
applied. Example:
"[{\"nodes\": 5, \"limit\": 15.0, \"burst\": 30}]"
Custom rate limits can also be supplied via the Helm
value `ciliumEndpointSlice.rateLimits`, which accepts a yaml
list of rate limit objects that will be formatted into a properly
escaped JSON string.
Signed-off-by: Tim Horner <timothy.horner@isovalent.com>
|
/test |
The CiliumEndpointSlice controller has 6 rate limit related config options. This PR attempts to simplify configuration by reducing that set to 2: one for supplying a custom, dynamic rate limit config, and another to supply a base multiplier for simple tuning of an existing config.
Note: The supplied default dynamic rate limit may need some adjustment. This will likely be updated after future scale testing and/or PR feedback.
See individual commits for more details