Skip to content

support multiple HostLbPolicyData entries per host#43995

Open
jukie wants to merge 3 commits intoenvoyproxy:mainfrom
jukie:multi-lbpolicydata
Open

support multiple HostLbPolicyData entries per host#43995
jukie wants to merge 3 commits intoenvoyproxy:mainfrom
jukie:multi-lbpolicydata

Conversation

@jukie
Copy link
Contributor

@jukie jukie commented Mar 17, 2026

Based on feedback in #43784.
This converts HostLbPolicyData from single-slot to a vector so multiple LB policies (e.g. the new policy in #43784 plus CSWRR as a child) can each attach per-host state and receive ORCA reports independently.

Signed-off-by: jukie <10012479+Jukie@users.noreply.github.com>
@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #43995 was opened by jukie.

see: more, trace.

@jukie jukie marked this pull request as ready for review March 17, 2026 23:25
@jukie jukie requested review from adisuissa and wbpcode as code owners March 17, 2026 23:25
Signed-off-by: jukie <10012479+Jukie@users.noreply.github.com>
wbpcode
wbpcode previously approved these changes Mar 18, 2026
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for this great contribution. And defer this to @paul-r-gall for another check. :)

/**
* @return true if this host data wants router ORCA callbacks.
*/
virtual bool receivesOrcaLoadReport() const { return false; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adisuissa lmk what you think about this API. I think it solves the problem, but I still with that "orca math" were not even part of the router filter at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the default return value here is harmful; imagine if someone had their own implementation of this thing that relied on orca reports. Rather than ceasing to compile, their implementation would start to misbehave. In particular, I recommend that you either make this a pure method or have the default value be true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the bigger problem is that instead of having the 'HostLbPolicyData' in the host it should be part of the LB-policy implementation. But modifying that is probably a big change.

So the question is whether the suggested solution causes some strange side-effects or greatly complicates its maintenance. I don't have sufficient context to answer this at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be willing to work on moving HostLbPolicyData out from host-level but it does seem like a non-trivial change. Would it be reasonable to take that on in a follow-up and use this as the first step?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paul-r-gall updated with your feedback

Signed-off-by: jukie <10012479+Jukie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants