Limitador sort function refactor #1100
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `sort.Sort` was failing to sort the list of limitador limits in a standard fashion. This was causing multiple updates to the limitador CR. By using `slices.SortFunc` a custom compare function is used, and now the sorting happens as expected. Extra logging has been added to help future investigations located issues in the limits reconcile. Signed-off-by: Jim Fitzpatrick <[email protected]>
The istio extension reconciler was reporting a route not belonging to a listener as error. This is not the case, a gateway may have many routes with these routes belonging to different listeners. Signed-off-by: Jim Fitzpatrick <[email protected]>
If there are numerous resources removed from the cluster at the same time we can sometimes be trying to update resources that have already being removed. This is not an error, just noisy. An info log has being added to cover when this happens. By rights, I think this should be a warning message, but that is not a log level that we use. Signed-off-by: Jim Fitzpatrick <[email protected]>
KevFan
reviewed
Jan 9, 2025
controllers/event_logger.go
Outdated
| @@ -6,6 +6,7 @@ import ( | |||
|
|
|||
| "github.com/kuadrant/policy-machinery/controller" | |||
| "github.com/kuadrant/policy-machinery/machinery" | |||
| "golang.org/x/exp/slices" | |||
There was a problem hiding this comment.
Suggested change
| "golang.org/x/exp/slices" | |
| "slices" |
Can we use slices package from the standard library instead?
There was a problem hiding this comment.
Auto import, and never noticed. I will change.
Refactor the event log message to not be as noisy when in Info log level. When there are a lot of resources the logs where not helpfully. Signed-off-by: Jim Fitzpatrick <[email protected]>
The log message was very noisy when no tls is used on the setup. The massage also does not give any helpfully information. For now, it has being moved to debug log level. Signed-off-by: Jim Fitzpatrick <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partial: #1085
What
From the investigate to reconciling large amounts of gateways and listeners it was found that the sort function used to sort the limitador limits before doing the compare on the existing and desired was not working. This change use the
slices.SortFuncto define a custom sort function.There are now not multiple writes to the limitador Resource.
Some very noisy log messages were refactored.
Note
Currently I have only looked into the affects on AuthPolices and RateLimitPolices on the reconcile. I still need to look into the TLS and DNS policies, but I found this to be a big enough change to introduce with out waiting for the rest.
Verification
The best way to verify these chances would be to run the load test, which can be found here: https://github.com/Kuadrant/testsuite/tree/main/scale_test
If load test seems a bit to much to verify this, I do have a set of scripts that can be used for testing it locally. They are still a work in progress. You may need to ask how to run them, but they can be found here: https://github.com/Boomatang/probable-octo-bassoon