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

discovery: use thanos resolver for endpoint groups #7565

Merged

Conversation

MichaHoffmann
Copy link
Contributor

@MichaHoffmann MichaHoffmann commented Jul 23, 2024

Endpoint Groups behave differently from regular endpoints right now. Regular endpoints are resolved periodically by our own resolvers but endpoint groups use the grpc "dns:///" resolver. This PR adds a "thanos:///" resolver that remedies this.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

@MichaHoffmann MichaHoffmann force-pushed the mhoffmann-use-thanos-resolver-for-endpoint-groups branch 3 times, most recently from a9adf95 to 01fe986 Compare July 23, 2024 19:06
@@ -891,14 +906,12 @@ func prepareEndpointSet(
}

for _, eg := range endpointGroupAddrs {
addr := fmt.Sprintf("dns:///%s", eg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this is a breaking change; its so that people can chose to keep previous behavior by adding "dns:///" prefix or opt into thanos resolver by adding "thanos:///" prefix; alternatively we could snoop if thanos:/// is added and if not add dns:/// or just add "thanos:///" by default, in theory it should be not noticeable mostly. I like it better to state the resolver explicitely here.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the main difference between the Thanos DNS resolver and the default DNS resolver we are using?

Trying to understand if it is a big difference. If it is, then I think the detection is needed and if no prefix is added we can still default to dns://. If there is no big difference I think I am fine either way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thanos one understands our "dnssrv+" stuff while the dns one does not. Otherwise it should be roughly the same.

@MichaHoffmann MichaHoffmann force-pushed the mhoffmann-use-thanos-resolver-for-endpoint-groups branch from 01fe986 to 557a848 Compare July 24, 2024 16:20
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Will this allow us to resolve addresses periodically? The dns resolver will never do re-resolution, unless all targets become unhealthy.

@yeya24
Copy link
Contributor

yeya24 commented Jul 25, 2024

The dns resolver will never do re-resolution, unless all targets become unhealthy.

Is this true? I remember it resolves every 30 min. But still almost unusable.

yeya24
yeya24 previously approved these changes Jul 25, 2024
@MichaHoffmann
Copy link
Contributor Author

Will this allow us to resolve addresses periodically? The dns resolver will never do re-resolution, unless all targets become unhealthy.

Yeah the resolver per connection runs in a loop and updates the addresses for it every configured SD interval like normal endpoint too

@fpetkovski
Copy link
Contributor

fpetkovski commented Jul 25, 2024

The dns resolver will never do re-resolution, unless all targets become unhealthy.

Is this true? I remember it resolves every 30 min. But still almost unusable.

This is my understanding based on grpc/grpc#12295, but yes, even 30 minutes is very long.

@MichaHoffmann MichaHoffmann changed the title WIP: discovery: use thanos resolver for endpoint groups discovery: use thanos resolver for endpoint groups Jul 25, 2024
@MichaHoffmann MichaHoffmann force-pushed the mhoffmann-use-thanos-resolver-for-endpoint-groups branch from 557a848 to dae758a Compare July 31, 2024 06:10
@MichaHoffmann MichaHoffmann force-pushed the mhoffmann-use-thanos-resolver-for-endpoint-groups branch from dae758a to ef38b8a Compare July 31, 2024 06:32
@MichaHoffmann
Copy link
Contributor Author

Test is flaky on main right now, merging.

@MichaHoffmann MichaHoffmann merged commit bc42129 into main Jul 31, 2024
19 of 20 checks passed
jnyi pushed a commit to jnyi/thanos that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants