Fix service entry merge #50711

howardjohn · 2024-04-26T22:54:14Z

Throwing in my attempt at fixing #50478.
Note this pairs well with #50690 to fix the EDS issue (#50688). With both PRs, all known issues are fixed.

There are a few approaches here:

Approach 1

Up to 1.19.8 the created clusters are one per port without LB between ports:

outbound|80||news.google.com::172.217.22.110:80
outbound|8080||news.google.com::172.217.22.110:8080

This behavior is, IMO, what a user expects. While the port names are colliding, user probably doesn't really care -- they care about the number. This also is the behavior if the port names did not collide.

This was the behavior in 1.19 and what I implemented here.

Approach 2

Cross product the ports:

outbound|80||news.google.com::172.217.22.110:80
outbound|80||news.google.com::172.217.22.110:8080
outbound|8080||news.google.com::172.217.22.110:80
outbound|8080||news.google.com::172.217.22.110:8080

I don't see a good reason to ever want this approach

Approach 3

Merge down to a single service port, but with an endpoint per port. Implemented in #50691

outbound|80||news.google.com::172.217.22.110:80
outbound|80||news.google.com::172.217.22.110:8080

The difference from 1 and 3 comes down to how we intepret them. In a Service, they key could logically be considered the name OR the number; because they must be distinct, there is never any difference between those. We happen to use name throughout Istio, but that is an internal detail.

This PR takes the opposite approach and makes the number effectively the key.

Note: I 100% agree the approach here is incredibly hacky. If we want to go down this route, I will try to make it suck less and add tests.

istio-testing · 2024-04-26T22:54:16Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pilot/pkg/model/service.go

keithmattix

Much cleaner than the previous implementation IMO. LGTM

releasenotes/notes/se-conflict.yaml

hzxuzhonghu · 2024-04-30T02:16:49Z

pilot/pkg/model/endpointshards.go

@@ -94,15 +94,26 @@ func (es *EndpointShards) Keys() []ShardKey {

 // CopyEndpoints takes a snapshot of all endpoints. As input, it takes a map of port name to number, to allow it to group
 // the results by service port number. This is a bit weird, but lets us efficiently construct the format the caller needs.
-func (es *EndpointShards) CopyEndpoints(portMap map[string]int) map[int][]*IstioEndpoint {
+func (es *EndpointShards) CopyEndpoints(portMap map[string]int, ports sets.Set[int]) map[int][]*IstioEndpoint {


can we initialize the ports sets from portMap in this function

yeah that makes sense. I was thinking it was faster to do it this way, but it shouldn't be

ymesika

LG, thanks

hzxuzhonghu

tks

istio-testing · 2024-05-01T02:07:22Z

In response to a cherrypick label: new pull request created: #50773

istio-testing · 2024-05-01T02:08:01Z

In response to a cherrypick label: #50711 failed to apply on top of branch "release-1.20":

Applying: Fix service entry merge
Applying: Add test
Applying: add release notes
Applying: better test
Applying: working except stable ordering
Using index info to reconstruct a base tree...
M	pilot/pkg/serviceregistry/serviceentry/conversion.go
M	pilot/pkg/xds/cds_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/xds/cds_test.go
Auto-merging pilot/pkg/serviceregistry/serviceentry/conversion.go
Applying: sort
Applying: Refactor
Using index info to reconstruct a base tree...
M	pilot/pkg/model/endpointshards.go
M	pilot/pkg/model/push_context.go
M	pilot/pkg/model/service.go
M	pilot/pkg/serviceregistry/serviceentry/conversion.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceentry/conversion.go
Auto-merging pilot/pkg/model/service.go
Auto-merging pilot/pkg/model/push_context.go
Auto-merging pilot/pkg/model/endpointshards.go
Applying: fix tests
Using index info to reconstruct a base tree...
M	pilot/pkg/model/endpointshards.go
M	pilot/pkg/model/push_context.go
M	pilot/pkg/model/service.go
M	pilot/pkg/serviceregistry/serviceentry/conversion.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceentry/conversion.go
Auto-merging pilot/pkg/model/service.go
Auto-merging pilot/pkg/model/push_context.go
CONFLICT (content): Merge conflict in pilot/pkg/model/push_context.go
Auto-merging pilot/pkg/model/endpointshards.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0008 fix tests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

istio-testing · 2024-05-01T02:08:02Z

In response to a cherrypick label: new issue created for failed cherrypick: #50774

istio-testing · 2024-05-01T02:08:39Z

In response to a cherrypick label: #50711 failed to apply on top of branch "release-1.21":

Applying: Fix service entry merge
Applying: Add test
Applying: add release notes
Applying: better test
Applying: working except stable ordering
Using index info to reconstruct a base tree...
M	pilot/pkg/serviceregistry/serviceentry/conversion.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceentry/conversion.go
Applying: sort
Applying: Refactor
Using index info to reconstruct a base tree...
M	pilot/pkg/model/endpointshards.go
M	pilot/pkg/model/push_context.go
M	pilot/pkg/model/service.go
M	pilot/pkg/serviceregistry/serviceentry/conversion.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceentry/conversion.go
Auto-merging pilot/pkg/model/service.go
Auto-merging pilot/pkg/model/push_context.go
Auto-merging pilot/pkg/model/endpointshards.go
Applying: fix tests
Using index info to reconstruct a base tree...
M	pilot/pkg/model/endpointshards.go
M	pilot/pkg/model/push_context.go
M	pilot/pkg/model/service.go
M	pilot/pkg/serviceregistry/serviceentry/conversion.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceentry/conversion.go
Auto-merging pilot/pkg/model/service.go
Auto-merging pilot/pkg/model/push_context.go
CONFLICT (content): Merge conflict in pilot/pkg/model/push_context.go
Auto-merging pilot/pkg/model/endpointshards.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0008 fix tests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

istio-testing · 2024-05-01T02:08:40Z

In response to a cherrypick label: new issue created for failed cherrypick: #50775

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Apr 26, 2024

istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 26, 2024

howardjohn mentioned this pull request Apr 26, 2024

Fix setting the merged service to servicesByHostname #50691

Merged

hzxuzhonghu reviewed Apr 28, 2024

View reviewed changes

pilot/pkg/model/service.go Outdated Show resolved Hide resolved

howardjohn mentioned this pull request Apr 29, 2024

Change in behavior of ServiceEntries merge #50478

Closed

17 tasks

howardjohn added 7 commits April 29, 2024 15:12

Fix service entry merge

0303ce0

Add test

7a3f350

add release notes

8d5d06f

better test

a2535e5

working except stable ordering

646bf20

sort

5f1716e

Refactor

8dc4a7f

howardjohn force-pushed the pilot/se-merge-fix branch from d17ed67 to 8dc4a7f Compare April 29, 2024 22:26

howardjohn marked this pull request as ready for review April 29, 2024 22:26

howardjohn requested review from a team as code owners April 29, 2024 22:26

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 29, 2024

keithmattix approved these changes Apr 29, 2024

View reviewed changes

howardjohn requested a review from hzxuzhonghu April 30, 2024 01:14

fix tests

15fc2dd

zirain reviewed Apr 30, 2024

View reviewed changes

releasenotes/notes/se-conflict.yaml Outdated Show resolved Hide resolved

hzxuzhonghu reviewed Apr 30, 2024

View reviewed changes

ymesika approved these changes Apr 30, 2024

View reviewed changes

fix tests and notes

6fbd5ce

zirain requested review from hzxuzhonghu and zirain May 1, 2024 01:03

zirain approved these changes May 1, 2024

View reviewed changes

hzxuzhonghu approved these changes May 1, 2024

View reviewed changes

istio-testing merged commit 91868f7 into istio:master May 1, 2024
28 checks passed

istio-testing mentioned this pull request May 1, 2024

[release-1.22] Fix service entry merge #50773

Merged

istio-testing mentioned this pull request May 1, 2024

[release-1.20] Fix service entry merge #50774

Closed

istio-testing mentioned this pull request May 1, 2024

[release-1.21] Fix service entry merge #50775

Closed

This was referenced May 1, 2024

[release-1.20] Fix service entry merge #50776

Merged

[release-1.21] Fix service entry merge #50777

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix service entry merge #50711

Fix service entry merge #50711

howardjohn commented Apr 26, 2024 •

edited

istio-testing commented Apr 26, 2024

keithmattix left a comment

hzxuzhonghu Apr 30, 2024

howardjohn Apr 30, 2024

ymesika left a comment

hzxuzhonghu left a comment

istio-testing commented May 1, 2024

istio-testing commented May 1, 2024

istio-testing commented May 1, 2024

istio-testing commented May 1, 2024

istio-testing commented May 1, 2024

Fix service entry merge #50711

Fix service entry merge #50711

Conversation

howardjohn commented Apr 26, 2024 • edited

Approach 1

Approach 2

Approach 3

istio-testing commented Apr 26, 2024

keithmattix left a comment

Choose a reason for hiding this comment

hzxuzhonghu Apr 30, 2024

Choose a reason for hiding this comment

howardjohn Apr 30, 2024

Choose a reason for hiding this comment

ymesika left a comment

Choose a reason for hiding this comment

hzxuzhonghu left a comment

Choose a reason for hiding this comment

istio-testing commented May 1, 2024

istio-testing commented May 1, 2024

istio-testing commented May 1, 2024

istio-testing commented May 1, 2024

istio-testing commented May 1, 2024

howardjohn commented Apr 26, 2024 •

edited