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

Use timeout for chooseProtocol #458

Merged
merged 5 commits into from
Apr 24, 2019
Merged

Conversation

thorfour
Copy link
Contributor

If an address DNS resolve takes a while, a probe can easily exceed its timeout limit.

This threads the given context to the probe into the chooseProtocol func to exit lookup of the IP address upon timeout.

@brian-brazil
Copy link
Contributor

Could you use one of the Resolver methods that take a context?

@thorfour
Copy link
Contributor Author

That's what I initially looked into doing, but couldn't find a Resolver function that supported both protocol, and context. LookupIPAddr doesn't take the protocol, but will return ipv4 and ipv6, which changes the behavior of the chooseProtocol function some.

And there's currently // TODO(bradfitz): Timeout time.Duration? to add timeout to the resolver in general?

@brian-brazil
Copy link
Contributor

We could filter the result of LookupIPAddr.

@thorfour
Copy link
Contributor Author

If that's an acceptable change I'd be happy to make that.

@thorfour
Copy link
Contributor Author

I've made the changes to use LookupIPAddr and return the requested IP protocol, or fallback if necessary.

@thorfour
Copy link
Contributor Author

Is travis-ci a flake? Unable to get failures locally

@thorfour
Copy link
Contributor Author

Doesn't appear to be a flake. I wonder if LookupIPAddr wont work since this technically changes the underlying behavior and does an ipv6 lookup too?

prober/utils.go Outdated Show resolved Hide resolved
@thorfour
Copy link
Contributor Author

Failure seems to be unique to Travis. I'm unable to get it to fail on the other platforms I've tried

Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

I've had that in the past, it was a v6 issue iirc.

prober/utils.go Outdated Show resolved Hide resolved
@thorfour thorfour force-pushed the master branch 2 times, most recently from 2735fe8 to 9062fdd Compare April 23, 2019 01:18
@thorfour
Copy link
Contributor Author

Since Travis doesn't support v6 and it's technically changing the behavior of the code to use the LookupIPAddr I've reverted it to the wrapped ctx method. Hopefully in the future the net package will become more mature and make this easier.

@brian-brazil
Copy link
Contributor

The problem with that though is that we don't actually stop the request. I'd prefer the LookupIPAddr method, and then do whatever to get Travis to pass.

@thorfour
Copy link
Contributor Author

Okay, I moved it back to use LookupIPAddr, and skipped the 4 failing tests on the travis environment

@brian-brazil brian-brazil merged commit f763e92 into prometheus:master Apr 24, 2019
@brian-brazil
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants