Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

catalog: allow root service selector to match backend pods #4303

Merged
merged 2 commits into from
Oct 22, 2021

Conversation

shashankram
Copy link
Member

Description:
Allows root service in a traffic split to have a selector
that matches the pods backing the leaf services. This
scenario was not tested before.

The change updates the inbound policy generation code
in a way such that policies are built for each upstream
service (including root service), where the upstream
service is guaranteed to be unique. Additionally,
traffic directed to the root service are routed to
a cluster corresponding to the root service and not
the leaf service.

Resolves #4296

Testing done:

  • unit test
  • e2e test

Affected area:

Functional Area
Control Plane [X]
SMI Policy [X]

Please answer the following questions with yes/no.

  1. Does this change contain code from or inspired by another project? no

    • Did you notify the maintainers and provide attribution?
  2. Is this a breaking change? no

@shashankram shashankram added the wip Work-in-Progress label Oct 22, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2021

Codecov Report

Merging #4303 (21ed388) into main (4baa2a3) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4303      +/-   ##
==========================================
- Coverage   69.25%   69.22%   -0.03%     
==========================================
  Files         211      211              
  Lines       14241    14226      -15     
==========================================
- Hits         9862     9848      -14     
+ Misses       4331     4330       -1     
  Partials       48       48              
Flag Coverage Δ
unittests 69.22% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/catalog/inbound_traffic_policies.go 93.44% <100.00%> (-0.50%) ⬇️
pkg/crdconversion/crdconversion.go 69.92% <0.00%> (+0.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4baa2a3...21ed388. Read the comment docs.

@shashankram shashankram removed the wip Work-in-Progress label Oct 22, 2021
@shashankram shashankram marked this pull request as ready for review October 22, 2021 15:43
@shashankram shashankram requested a review from a team as a code owner October 22, 2021 15:43
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

LGTM. One overall question: how does load balancing work when an apex service's selector matches pods for different backends? e.g. if I have apex service SA splitting between S1 with weight 10 and S2 with weight 90 and a selector on SA matching pods for both services, is that weight defined in the traffic split always honored?

pkg/catalog/inbound_traffic_policies.go Show resolved Hide resolved
tests/e2e/e2e_trafficsplit_selector_test.go Outdated Show resolved Hide resolved
// of services. An upstream service is associated with another service if it is a backend for an apex/root service
// in a TrafficSplit config. This function returns a list consisting of the given upstream services and all apex
// services associated with each of those services.
func (mc *MeshCatalog) getUpstreamServicesIncludeApex(upstreamServices []service.MeshService) []service.MeshService {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great naming! Crisp and clear!

Thanks for the comment too!

@shashankram
Copy link
Member Author

LGTM. One overall question: how does load balancing work when an apex service's selector matches pods for different backends? e.g. if I have apex service SA splitting between S1 with weight 10 and S2 with weight 90 and a selector on SA matching pods for both services, is that weight defined in the traffic split always honored?

Traffic splitting weights are associated with the backend services and not the pods backing those services. So in the above example, traffic directed to the apex will be split S1(10)-S2(90), where S1 and S2 are clusters in envoy, whose endpoints are the pods backing S1 and S2. Traffic to endpoints of a cluster are load balanced equally.

Allows root service in a traffic split to have a selector
that matches the pods backing the leaf services. This
scenario was not tested before.

The change updates the inbound policy generation code
in a way such that policies are built for each upstream
service (including root service), where the upstream
service is guaranteed to be unique. Additionally,
traffic directed to the root service are routed to
a cluster corresponding to the root service and not
the leaf service.

Resolves openservicemesh#4296

Signed-off-by: Shashank Ram <[email protected]>
Signed-off-by: Shashank Ram <[email protected]>
@shashankram shashankram merged commit d9baef6 into openservicemesh:main Oct 22, 2021
@shashankram shashankram deleted the fix-split branch October 22, 2021 19:14
allenlsy pushed a commit to allenlsy/osm that referenced this pull request Dec 28, 2021
…cemesh#4303)

Allows root service in a traffic split to have a selector
that matches the pods backing the leaf services. This
scenario was not tested before.

The change updates the inbound policy generation code
in a way such that policies are built for each upstream
service (including root service), where the upstream
service is guaranteed to be unique. Additionally,
traffic directed to the root service are routed to
a cluster corresponding to the root service and not
the leaf service.

Resolves openservicemesh#4296

Signed-off-by: Shashank Ram <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TrafficSplit does not work when root service selector matches pod backing the backend service
5 participants