Skip to content

Add min_size option when forcing LocalityDirect in zone-aware-routing#39058

Merged
wbpcode merged 7 commits intoenvoyproxy:mainfrom
jukie:min-zone-size
Apr 30, 2025
Merged

Add min_size option when forcing LocalityDirect in zone-aware-routing#39058
wbpcode merged 7 commits intoenvoyproxy:mainfrom
jukie:min-zone-size

Conversation

@jukie
Copy link
Copy Markdown
Contributor

@jukie jukie commented Apr 10, 2025 •
edited
Loading

Commit Message: Add force_local_zone to zone-aware routing with min_size option and deprecate force_locality_direct_routing.
Additional Description: Follow-up to #38756. Adds minimum zone size parameter as a gate to forcing LocalityDirect mode in zone-aware-routing and renames force_locality_direct_routing to force_local_zone.
Risk Level:
Testing:
Docs Changes: Yes
Release Notes:
Platform Specific Features:

Fixes #38561

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Apr 10, 2025

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #39058 was opened by jukie.

see: more, trace.

@jukie jukie requested a review from zuercher as a code owner April 10, 2025 04:31
@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 10, 2025

/retest

@nezdolik nezdolik self-assigned this Apr 10, 2025
@jukie jukie changed the title Add prefer_local_zone_min_size to zone-aware-routing Add force_local_zone_min_size to zone-aware-routing Apr 10, 2025
abeyad
abeyad reviewed Apr 10, 2025
View reviewed changes
@jukie jukie requested a review from abeyad April 10, 2025 16:15
abeyad
abeyad reviewed Apr 10, 2025
View reviewed changes
abeyad
abeyad previously approved these changes Apr 10, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

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

/lgtm api

abeyad
abeyad previously approved these changes Apr 10, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

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

/lgtm api

@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 10, 2025

/retest

@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 10, 2025

Failure is due to quic coverage but looking at main branch and other PRs it doesn't look like that's a blocker?

Code coverage for source/common/quic is lower than limit of 93.2 (93.1)

@abeyad
Copy link
Copy Markdown
Contributor

abeyad commented Apr 10, 2025

This looks to be failing on other PRs too, e.g. https://github.com/envoyproxy/envoy/actions/runs/14382475329/job/40329544401.

I'll try to ping Envoy maintainers to sort it out.

@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 11, 2025

/retest

@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 11, 2025

I saw your temp fix @abeyad and rebased

jukie added 3 commits April 11, 2025 10:25
@jukie
Add force_local_zone_min_size to zone-aware-routing and rename force_…
a9059cc 
…locality_direct_routing

Signed-off-by: jukie <10012479+Jukie@users.noreply.github.com>
@jukie
use uint32
b6ef41c 
Signed-off-by: jukie <10012479+Jukie@users.noreply.github.com>
@jukie
cleanup description
eee0177 
Signed-off-by: jukie <10012479+Jukie@users.noreply.github.com>
@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 11, 2025

/retest

1 similar comment
@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 11, 2025

/retest

@jukie
fix test
e7026c7 
Signed-off-by: Jukie <10012479+Jukie@users.noreply.github.com>
@jukie jukie requested a review from abeyad April 11, 2025 19:49
@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 14, 2025

Cc @wbpcode

abeyad
abeyad previously approved these changes Apr 14, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

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

/lgtm api

You will need approval from @nezdolik as a code owner for load_balancing_policies

@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 16, 2025

Could I get a second review here please @nezdolik @wbpcode ?

nezdolik
nezdolik reviewed Apr 21, 2025
View reviewed changes
nezdolik
nezdolik previously approved these changes Apr 22, 2025
View reviewed changes
@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 24, 2025

Thanks for the review here. Could someone please merge?

wbpcode
wbpcode reviewed Apr 24, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Thanks for the contribution. But I think we may cannot change the API directly now because we have cut a release last week... sorry.

@jukie
deprecate force_locality_direct_routing
fccd14b 
Signed-off-by: Jukie <10012479+Jukie@users.noreply.github.com>
@jukie jukie dismissed stale reviews from nezdolik and abeyad via fccd14b April 28, 2025 05:58
@jukie jukie requested review from abeyad, nezdolik and wbpcode April 28, 2025 05:59
jukie added 2 commits April 28, 2025 00:34
@jukie
Fix expect calls
71b815a 
Signed-off-by: Jukie <10012479+Jukie@users.noreply.github.com>
@jukie
proto format
a0005c1 
Signed-off-by: Jukie <10012479+Jukie@users.noreply.github.com>
@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 28, 2025

/retest

@jukie jukie changed the title Add force_local_zone_min_size to zone-aware-routing Add min_size option when forcing LocalityDirect in zone-aware-routing Apr 28, 2025
@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 28, 2025

Ready for re-review please. I've added a deprecation and conversion for force_locality_direct_routing to the new ForceLocalZone field suggested by @wbpcode.

abeyad
abeyad approved these changes Apr 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

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

/lgtm api

@jukie
Copy link
Copy Markdown
Contributor Author

jukie commented Apr 29, 2025 •
edited
Loading

Thanks everyone for the reviews here. It'd be greatly appreciated if this could be merged this week so we can make use of it within EnvoyGateway before the next release. This will allow for differentiating between Kubernetes Topology Aware Routing and Traffic Distribution.

@wbpcode wbpcode merged commit 88490aa into envoyproxy:main Apr 30, 2025
25 checks passed
@jukie jukie deleted the min-zone-size branch May 1, 2025 01:19
@jukie jukie mentioned this pull request May 1, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abeyad abeyad abeyad approved these changes

@tonya11en tonya11en Awaiting requested review from tonya11en tonya11en is a code owner

@zuercher zuercher Awaiting requested review from zuercher zuercher is a code owner

@nezdolik nezdolik Awaiting requested review from nezdolik nezdolik is a code owner

@wbpcode wbpcode Awaiting requested review from wbpcode wbpcode is a code owner

Assignees

@abeyad abeyad

@nezdolik nezdolik

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Zone Aware Routing: Ability to set minimum number of hosts per-zone

4 participants

@jukie @abeyad @wbpcode @nezdolik