Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Try MeshLoadBalancingStrategy with MeshMultizoneService #11968

Open
bartsmykla opened this issue Nov 4, 2024 · 10 comments · Fixed by #11980
Open

Try MeshLoadBalancingStrategy with MeshMultizoneService #11968

bartsmykla opened this issue Nov 4, 2024 · 10 comments · Fixed by #11980
Assignees
@michaelbeaumont
Labels
kind/improvement Improvement on an existing feature triage/accepted The issue was reviewed and is complete enough to start working on it

Comments

@bartsmykla
Copy link
Contributor

bartsmykla commented Nov 4, 2024 •
edited
Loading

Description

We should try MeshLoadBalancingStrategy with MeshMultizoneService as it should behave differently that we should prioritize local zone instead of prioritizing splitting traffic between all zones.

This is followup for: #11966
@bartsmykla bartsmykla added triage/accepted The issue was reviewed and is complete enough to start working on it kind/improvement Improvement on an existing feature labels Nov 4, 2024
@lukidzi
Copy link
Contributor

lukidzi commented Nov 5, 2024

Additionally, we need to ensure that when a user disables locality-aware routing using MeshLoadBalancingStrategy, traffic is properly distributed across zones.

@michaelbeaumont michaelbeaumont self-assigned this Nov 5, 2024
@michaelbeaumont michaelbeaumont mentioned this issue Nov 5, 2024
Merged
michaelbeaumont added a commit that referenced this issue Nov 6, 2024
@michaelbeaumont
fix(MeshLoadBalancingStrategy): set all priorities equal if localityA…
daed4c8 
…ware is disabled (#11980)

We already set `priority: 1` for remote zones in
`fillRemoteMeshServices` so we need to explicitly set `priority: 0` if
we _don't_ want locality awareness.

Also fix an insufficient condition in an existing case, where we weren't
verifying that requests really go to all zones, only that it's OK if
they do.

Fixes #11968

Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
Copy link
Contributor

we should prioritize local zone instead of prioritizing splitting traffic between all zones.

btw this is already the case

@lahabana lahabana reopened this Nov 6, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2024

Removing closed state labels due to the issue being reopened.

@lahabana
Copy link
Contributor

lahabana commented Nov 6, 2024 •
edited
Loading

Fix was reverted in:

@lukidzi
Copy link
Contributor

lukidzi commented Nov 6, 2024

We have two options to set locality-aware load balancing in Kuma:

  1. Mesh: Locality-Aware Load Balancing
  2. MeshLoadBalancingStrategy (MLBS): Locality Awareness in MLBS

These options allow control over whether traffic should be split across different zones or prioritized within the local zone.

  • MeshService: Prioritizes the local zone.
  • MeshMultiZoneService: Prioritizes the local zone.
  • kuma.io/service: Sets the default priority to 0, distributing traffic across different zones.

When the locality-aware flag is enabled on the Mesh, traffic for kuma.io/service will remain within the same zone. The same effect occurs with MLBS: when set to true, traffic stays within the local zone. If both sets, MLBS can override it

@michaelbeaumont
Copy link
Contributor

michaelbeaumont commented Nov 6, 2024 •
edited
Loading

@lahabana @lukidzi do we have any action to take given the above? or can we close this again

Note: The fix was not reverted, instead #11987 was the issue

@lukidzi
Copy link
Contributor

lukidzi commented Nov 7, 2024

I think there is one more thing to try, we should check how we behave by the default (no policy/lb mesh configured)

@michaelbeaumont
Copy link
Contributor

For this we have an e2e test I think right? It's locality aware

@lahabana lahabana added triage/pending This issue will be looked at on the next triage meeting and removed triage/accepted The issue was reviewed and is complete enough to start working on it labels Nov 8, 2024
@lahabana
Copy link
Contributor

lahabana commented Nov 8, 2024

Putting this back in pending so we discuss this

@lukidzi lukidzi added triage/accepted The issue was reviewed and is complete enough to start working on it and removed triage/pending This issue will be looked at on the next triage meeting labels Nov 12, 2024
@lukidzi
Copy link
Contributor

lukidzi commented Nov 12, 2024

lets check if there is a test covering this case and if we can confirm it works as expected, close the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@michaelbeaumont michaelbeaumont

Labels
kind/improvement Improvement on an existing feature triage/accepted The issue was reviewed and is complete enough to start working on it
Projects
None yet
Milestone
No milestone
4 participants
@lahabana @michaelbeaumont @bartsmykla @lukidzi