-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Acceleration update Endpoints #11046
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: zvlb <vl.zemtsov@gmail.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zvlb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @zvlb. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
✅ Deploy Preview for kubernetes-ingress-nginx canceled.
|
@zvlb Looks like the nginx config generation e2e tests are complaining about this change. |
/kind feature We have had others in the past complain about this issue, with large ingress and the syncing taking a while, I believe this will help. |
I think we need to discuss this more with the acceleration of endpoint updates and serial reloads. This may lead to some unforeseen issues. |
This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach |
The This bot removes
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /remove-lifecycle frozen |
What this PR does / why we need it:
When you have many ingresses in K8s cluster and frequent deploys you have many problems.
Why:
SyncIngresses
function takes about one and a half minutes to complete (the longest processes being nginx -t and nginx -s reload). Each process takes about 40 seconds in my case (~3000 ingresses, nginx config file - 200mb)SyncIngresses
, immediately after one SyncIngresses finishes, the next one starts. And so on in a perpetual loop.My problem:
The process of updating Endpoints is part of the SyncIngresses function. This means Nginx will only update Endpoints 1-2-3 minutes after the actual IP change of the POD.
Types of changes
Which issue/s this PR fixes
I add new queue for update Only Endpoints, and copy logic from
SyncIngresses
for update dynamic configs (LUA).I see a potential issue with data consistency due to the parallel execution of SyncIngresses and SyncEndpoints at the moment n.runningConfig = pcfg. Because of the duration of execution in SyncIngresses, which also updates Endpoints, the list of endpoints will likely be outdated (by one and a half minutes).
(However, I think we can fix it)
How Has This Been Tested?
I start my fork in K8s cluster with ~3000 ingresses and hyperactive developers (deploys every 1 minute)
Checklist:
(I skip adding documetation and test, bc firstly I want discuss about my solution. May be my solution not very good)