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.

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

Add DNSSettings for Tracer Configuration (API part) #2934

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aatarasoff
Copy link

@aatarasoff aatarasoff commented Sep 20, 2023

A new section called dns_settings has been added to the Tracer Configuration. The purpose of this addition is to limit the Istio proxy's DNS queries to 10 minutes instead of the default 30 seconds when Zipkin tracing is enabled. This can be achieved by defining the below structure in MeshConfig using this PR:

dns_settings:
  refresh_rate: "600s"
  respect_ttl: false

Related PR: istio/istio#47049

@aatarasoff aatarasoff requested a review from a team as a code owner September 20, 2023 12:15
@istio-policy-bot
Copy link

😊 Welcome @aatarasoff! This is either your first contribution to the Istio api repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: aatarasoff / name: Aleksandr Tarasov (ccfc94a)

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test labels Sep 20, 2023
@istio-testing
Copy link
Collaborator

Hi @aatarasoff. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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.

This DNS issue only applies to the legacy tracing configuration. I think a more appropriate fix would be to transition to https://istio.io/latest/docs/tasks/observability/telemetry/ ?

@costinm
Copy link
Contributor

costinm commented Sep 25, 2023

Does this config apply to ALL tracers ? I agree with John, it's something that otel collector should handle.

Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

@aatarasoff thank you for your contribution!

Could you check if it make sense to add it to the Telemetry API (per John mentioned earlier) given this is telemetry related config?

A small tip- for API change, might be good to present to our Wed's working group meeting.

@hzxuzhonghu
Copy link
Member

I think dns refresh rate is not only to tracers, but to all DNS services. We have a DnsRefreshRate in MeshConfig now, but actually we do not allow setting respect_ttl, which is hardcoded to true.

We can add a respect_ttl to meshConfig, and allow agent to get both values

@hzxuzhonghu
Copy link
Member

Find that we have also supported dynamic bootstrap discovery, which can be used to customize the whole bootstrap config

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 16, 2024
@istio-testing
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants