force backend sync when worker starts #2511
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/assign @aledbf |
k8s-ci-robot
added
size/S
|
/approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| @@ -28,6 +28,7 @@ local function get_current_backend() | |||
| local backend = backends:get(backend_name) | |||
|
|
|||
| if not backend then | |||
| -- TODO(elvinefendi) maybe force backend sync here? | |||
There was a problem hiding this comment.
Maybe we want to call sync_backends in the init phase? Which occurs before the worker processes are forked? Alternately you can sync_backend if init_worked hasn't been called at least once. Wondering if you had input @aledbf?
There was a problem hiding this comment.
@andrewlouis93 init phase is not per worker, what's the point of calling this function there? We need to sync backends per worker - that's why we use init_worker to do so, which is init but per worker.
There was a problem hiding this comment.
after this PR I actually can not think of any reason why we would wanna implement what I say in this TODO.
There was a problem hiding this comment.
You're right, we're not populating a shared dictionary - backends is per worker.
after this PR I actually can not think of any reason why we would wanna implement what I say in this TODO.
😂 make sure you nip your TODO next PR :)
|
Have one question, looks good otherwise! |
Codecov Report
@@ Coverage Diff @@
## master #2511 +/- ##
=======================================
Coverage 41.64% 41.64%
=======================================
Files 74 74
Lines 5276 5276
=======================================
Hits 2197 2197
Misses 2781 2781
Partials 298 298
Continue to review full report at Codecov.
|
What this PR does / why we need it:
We have been seeing constant
errors in our clusters. The debugging showed that they correlate to Nginx reloads. What happens is when Nginx reloads we wait 1s before syncing backends to local worker memory. And during that time all requests fail. The PR fix that bug and also makes sure we return 503 when backend is not found.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer: