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

Fix setting the merged service to servicesByHostname #50691

Merged
merged 4 commits into from Apr 29, 2024
Merged

Fix setting the merged service to servicesByHostname #50691

merged 4 commits into from Apr 29, 2024

Conversation

hzxuzhonghu
Copy link
Member

@hzxuzhonghu hzxuzhonghu commented Apr 26, 2024 •
edited

Please provide a description of this PR:

During merge should:

  1. Set the servicesByHostname to the merged service, previously the later svc.

@hzxuzhonghu hzxuzhonghu requested a review from a team as a code owner April 26, 2024 08:44
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 26, 2024
@hzxuzhonghu
Copy link
Member Author

For #50478

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 26, 2024
keithmattix
keithmattix reviewed Apr 26, 2024
View reviewed changes
Copy link
Contributor

@keithmattix keithmattix 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 fixing this. The original issue described the bug in terms of incorrectly configured endpoints; can we add a test that checks that specifically?

releasenotes/notes/service-merge.yaml Outdated
- 50478
releaseNotes:
- |
**Fixed** merge services ports should also compare the port name besides port number, otherwise the the ClusterLoadAssignments could be messed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Fixed** merge services ports should also compare the port name besides port number, otherwise the the ClusterLoadAssignments could be messed.
**Fixed** merge services ports should also compare the port name besides port number, otherwise the the ClusterLoadAssignments could be incorrect.

pilot/pkg/model/sidecar.go
@@ -980,7 +981,7 @@ func (sc *SidecarScope) appendSidecarServices(servicesAdded map[host.Name]sideca
// Update index as well, so that future reads will merge into the new service
foundSvc.svc = copied
servicesAdded[foundSvc.svc.Hostname] = foundSvc
sc.servicesByHostname[s.Hostname] = s
sc.servicesByHostname[s.Hostname] = copied
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems really subtle; should we add a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test to gurantee. Yes, actually copied is the merged Service

@keithmattix keithmattix added cherrypick/release-1.19 Set this label on a PR to auto-merge it to the release-1.19 branch cherrypick/release-1.20 Set this label on a PR to auto-merge it to the release-1.20 branch cherrypick/release-1.21 Set this label on a PR to auto-merge it to the release-1.21 branch cherrypick/release-1.22 Set this label on a PR to auto-merge it to the release-1.22 branch and removed cherrypick/release-1.19 Set this label on a PR to auto-merge it to the release-1.19 branch labels Apr 26, 2024
jaellio
jaellio reviewed Apr 26, 2024
View reviewed changes
Copy link
Contributor

@jaellio jaellio left a comment •
edited

Choose a reason for hiding this comment

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

Could you add a note to the description with what the configured clusters looked like before and after this change?

@howardjohn howardjohn mentioned this pull request Apr 26, 2024
Merged
howardjohn
howardjohn reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

I am not sure I agree the new behavior is desired. I opened an alternative in #50711. This brings us back to 1.19 behavior. LMK what you think.

@hzxuzhonghu
Copy link
Member Author

Yes , i agree behavior below is right

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

@hzxuzhonghu
Copy link
Member Author

It is very tricky for ServiceEntry to aligh with K8s service semantics. For that both portName and number can not be same

@hzxuzhonghu hzxuzhonghu mentioned this pull request Apr 28, 2024
Closed
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 28, 2024
@hzxuzhonghu hzxuzhonghu changed the title Fix services merge Fix setting the merged service to servicesByHostname Apr 28, 2024
@hzxuzhonghu hzxuzhonghu added the release-notes-none Indicates a PR that does not require release notes. label Apr 28, 2024
@hzxuzhonghu
Copy link
Member Author

left some fix to #50711, only keep fixing servicesByHostname

@istio-testing istio-testing merged commit 81284e3 into istio:master Apr 29, 2024
28 checks passed
@istio-testing
Copy link
Collaborator

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

Applying: Fix services in SidecarScope.servicesByHostname not equal to SidecarScope.services
Using index info to reconstruct a base tree...
M	pilot/pkg/model/sidecar.go
M	pilot/pkg/model/sidecar_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/model/sidecar_test.go
Auto-merging pilot/pkg/model/sidecar.go
CONFLICT (content): Merge conflict in pilot/pkg/model/sidecar.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix services in SidecarScope.servicesByHostname not equal to SidecarScope.services
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
Copy link
Collaborator

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

@istio-testing istio-testing mentioned this pull request Apr 29, 2024
Closed
@istio-testing
Copy link
Collaborator

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

Applying: Fix services in SidecarScope.servicesByHostname not equal to SidecarScope.services
Using index info to reconstruct a base tree...
M	pilot/pkg/model/sidecar.go
M	pilot/pkg/model/sidecar_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/model/sidecar_test.go
Auto-merging pilot/pkg/model/sidecar.go
CONFLICT (content): Merge conflict in pilot/pkg/model/sidecar.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix services in SidecarScope.servicesByHostname not equal to SidecarScope.services
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
Copy link
Collaborator

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

@istio-testing istio-testing mentioned this pull request Apr 29, 2024
Closed
@istio-testing
Copy link
Collaborator

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

@istio-testing istio-testing mentioned this pull request Apr 29, 2024
Merged
This was referenced Apr 29, 2024
Open
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithmattix keithmattix keithmattix left review comments

@jaellio jaellio jaellio left review comments

@howardjohn howardjohn howardjohn approved these changes

Assignees
No one assigned
Labels
cherrypick/release-1.20 Set this label on a PR to auto-merge it to the release-1.20 branch cherrypick/release-1.21 Set this label on a PR to auto-merge it to the release-1.21 branch cherrypick/release-1.22 Set this label on a PR to auto-merge it to the release-1.22 branch release-notes-none Indicates a PR that does not require release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@hzxuzhonghu @istio-testing @howardjohn @keithmattix @jaellio