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

avoid calling servicesForNamespacedName for headless services #42089

Merged
merged 3 commits into from
Nov 23, 2022
Merged

avoid calling servicesForNamespacedName for headless services #42089

merged 3 commits into from
Nov 23, 2022

Conversation

ramaraochavali
Copy link
Contributor

@ramaraochavali ramaraochavali commented Nov 21, 2022 •
edited
Loading

  • Ambient
  • Configuration Infrastructure
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Policies and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

@ramaraochavali
push nds on end point changes
7f7f8f1 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali ramaraochavali requested a review from a team as a code owner November 21, 2022 12:32
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 21, 2022
howardjohn
howardjohn approved these changes Nov 21, 2022
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.

is there any test we should have to cover this case?

@howardjohn
Copy link
Member

Actually, do we need some logic to detect that smething actually changed? Worry about excessive NDS pushes

@ramaraochavali
Copy link
Contributor Author

Actually, do we need some logic to detect that smething actually changed? Worry about excessive NDS pushes

I am also worried about that - but not sure what can we do/check when endpoint update comes?

@howardjohn
Copy link
Member

Actually, do we need some logic to detect that smething actually changed? Worry about excessive NDS pushes

I am also worried about that - but not sure what can we do/check when endpoint update comes?

Not sure ,but its pretty concerning that this makes a push to 1 endpoint push the full NDS (which is for all services). I think this may be a major performance regression

@ramaraochavali ramaraochavali added the do-not-merge/hold Block automatic merging of a PR. label Nov 21, 2022
@ramaraochavali
Copy link
Contributor Author

I have put a hold on it. Let us see if we can find some better solution.

@ramaraochavali
avoid calling servicesForNamespacedName for headless services
daf3d39 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali ramaraochavali requested a review from a team as a code owner November 22, 2022 06:23
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 22, 2022
@ramaraochavali ramaraochavali changed the title push nds on end point changes avoid calling servicesForNamespacedName for headless services Nov 22, 2022
@ramaraochavali
comments
e09d06e 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@istio-testing istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 22, 2022
@ramaraochavali ramaraochavali added the release-notes-none Indicates a PR that does not require release notes. label Nov 22, 2022
@ramaraochavali
Copy link
Contributor Author

I have verified again and not able to reproduce the problem. May be some inconsistent Istiod state. Looking further in the code, we are actually doing a full push for headless services

istio/pilot/pkg/serviceregistry/kube/controller/endpointcontroller.go

Line 68 in 2b39b30

if features.EnableHeadlessService {
- So this should take care.

I just added few comments and moved condition above. @howardjohn PTAL. If it looks good, please remove DNM label.

hzxuzhonghu
hzxuzhonghu approved these changes Nov 22, 2022
View reviewed changes
Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

LG

@ramaraochavali ramaraochavali removed the do-not-merge/hold Block automatic merging of a PR. label Nov 23, 2022
@ramaraochavali
Copy link
Contributor Author

/test integ-security

@istio-testing istio-testing merged commit a513633 into istio:master Nov 23, 2022
@ramaraochavali ramaraochavali deleted the fix/dns_eds branch November 23, 2022 08:13
@ramaraochavali ramaraochavali mentioned this pull request Nov 26, 2022
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu approved these changes

@howardjohn howardjohn howardjohn approved these changes

Assignees
No one assigned
Labels
area/networking release-notes-none Indicates a PR that does not require release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ramaraochavali @howardjohn @hzxuzhonghu @istio-testing @istio-policy-bot