-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use sharedInformers, fix CPU usage, support all ingress apiGroups #64
Conversation
Signed-off-by: Laurent Marchaud <[email protected]>
Signed-off-by: Laurent Marchaud <[email protected]>
Signed-off-by: Laurent Marchaud <[email protected]>
@Aluxima Thank you very much for the PR, this is awesome! 🚀 Apologies for the delay in looking at this, we've had our focus in other places over the last few months but I'll take a look at this change next week and look to get it merged in. We really appreciate your contribution! 😄 |
I'll also work on reviewing #62 👍 |
Thanks so much @DewaldV ! I'm excited for your reviews, we've also rolled out the TLS secrets synchronization feature based on this branch recently which is game-changer for us, it will come in a future PR :) Aluxima/yggdrasil@v1-ingress...Aluxima:yggdrasil:v1-ing-secrets-sync |
hey @DewaldV ! I hope you're good. Any chance you could have a look at our PRs ? |
Signed-off-by: Laurent Marchaud <[email protected]>
@Aluxima just had a thought, anyway rest ingress apiVersions are unsupported after 1.22. |
We still have some legacy <1.18 clusters along with some others in recent versions hence the support for all possible ingress apiversions at the same time. Without this need it would have been easier to just support |
@Aluxima fair enough, have you tested this with different versions of Kube clusters? I will also test it out with 1.22 |
@surajnarwade This has been running in production with 1.17, 1.21, 1.22 and 1.23 successfully. |
@Aluxima thanks for testing it out. I am about to test out the change on our clusters and hopefully merge this PR on monday :) |
@Aluxima Sorry for the delay, I have tested it out with our setup and it works as expected :) will merge this now, thank you so much for your contribution <3 |
thank you @surajnarwade ! |
This pull request brings 3 main changes:
Use sharedInformerFactory instead of List-Watch
This allows us to use event handlers and easily add more resource types to be watched (e.g. multiple ingress, secrets as per #54)
Fix the high CPU usage issue by changing the snapshotter loop
The snapshotter was building a new snapshot at each and every resource change, including when the cache was resynced (every minute). This caused X snapshots to be done at startup and every minute, X being the total number of ingresses.
With this change, received events only register that a new snapshot is required, which is done every 5s by the new sync loop.
Should this 5s sync period be configurable as a flag?
With almost 1000 ingresses, yggdrasil was constantly using 100% CPU. After this fix, only 0.1% is used.
Support ingresses of all apiGroups
The use of the sharedInformerFactory allows us to easily and dynamically watch ingresses of different apiGroups. The available ingress versions on the apiservers are tried at startup, and selected in the following order:
This requires converting the ingresses to a generic apiGroup-agnostic Ingress structure.
These changes bring no breaking change
Closes #53