lb: unified the lifetime mechanism of worker local lb#45100
Conversation
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
wbpcode
changed the title
|
/retest |
|
/gemini review |
|
I guess @tonya11en is pretty busy recently. |
There was a problem hiding this comment.
Pull request overview
This PR updates Envoy’s thread-aware load balancing implementation so worker-local load balancers refresh their internal state in place (via priority set member-update callbacks) rather than being recreated when hosts change, aligning behavior with other LB implementations.
Changes:
- Modify
ThreadAwareLoadBalancerBaseworker LB instances to refresh cached per-priority state from the shared factory on member updates, and setrecreateOnHostChange()tofalse. - Add Maglev unit tests to validate “no recreation on host change”, in-place refresh behavior, per-worker independence, and callback unregistration on destruction.
- Remove an assertion in deferred cluster initialization tests that assumed
recreateOnHostChange()behavior for RING_HASH.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/extensions/load_balancing_policies/maglev/maglev_lb_test.cc | Adds unit tests covering in-place worker LB refresh semantics for Maglev. |
| test/common/upstream/deferred_cluster_initialization_test.cc | Updates a test to stop asserting LB recreation behavior for RING_HASH. |
| source/extensions/load_balancing_policies/common/thread_aware_lb_impl.h | Updates thread-aware LB factory/worker LB interfaces; disables recreation on host changes. |
| source/extensions/load_balancing_policies/common/thread_aware_lb_impl.cc | Implements worker LB refresh via member-update callback and factory-backed state copying. |
|
/gemini review |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: code <wbphub@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the thread-aware load balancer implementation to support in-place state refreshes for worker load balancers, avoiding the need for recreation by the cluster manager during host changes. Key changes include the addition of a refresh method to LoadBalancerImpl, the registration of member update callbacks on the priority set, and updating LoadBalancerFactoryImpl to return false for recreateOnHostChange. Comprehensive tests were added to verify independent refreshes and proper callback cleanup. I have no feedback to provide.
There was a problem hiding this comment.
Code Review
This pull request refactors the thread-aware load balancer implementation to allow worker load balancer instances to refresh their state in-place when host changes occur, rather than being recreated. This is achieved by registering a member update callback on the worker's priority set and overriding recreateOnHostChange to return false. The changes also include comprehensive tests for the refresh logic and callback lifecycle. Review feedback suggests updating a comment in the refresh() method to more accurately describe the locking mechanism protecting shared state accessed by multiple threads.
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
agrawroh
left a comment
agrawroh
left a comment
There was a problem hiding this comment.
We'll need to change this as well: envoy/upstream/load_balancer.h:294-309
"Every time there is a host set update on the main thread, all workers will create a new worker local load balancer via the factory."
recreateOnHostChange() is now returning false, and the cluster_manager_impl.cc:1513-1516 recreate branch is skipped for these. We should describe the in-place refresh model.
Also, RingHash seems to have no parallel coverage for the new behavior.
Yeah. Next we will refactor whole factory API of the thread local lb and the recreateOnHostChange() will be removed completely. |
Commit Message: lb: unify the lifetime mechanism of worker local lb Additional Description: In the previous implementation, the thread aware lb will recreate the worker local lb when endpoint set changes. This PR updated the implementation of thread aware lb to update the worker local lb in place rather than to create a new one. The new behavior is same with all other LB implementation. Risk Level: mid. Testing: unit. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alireza <alrzazz98@gmail.com>
Commit Message: lb: unified the lifetime mechanism of worker load lb 2 Additional Description: Similar to previous #45100 but this PR have updated all left LB. Risk Level: Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com> Signed-off-by: code <wbphub@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Commit Message: lb: unify the lifetime mechanism of worker local lb
Additional Description:
In the previous implementation, the thread aware lb will recreate the worker local lb when endpoint set changes. This PR updated the implementation of thread aware lb to update the worker local lb in place rather than to create a new one. The new behavior is same with all other LB implementation.
Risk Level: mid.
Testing: unit.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.