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

Overriding DNS #494

Merged
merged 3 commits into from Feb 6, 2018

Conversation

Projects
None yet
4 participants
@luizbafilho
Contributor

luizbafilho commented Feb 3, 2018

This PR closes #474. Adds the option to override DNS lookup with a specific array of IPs.

Although I implemented the option of passing multiple IPs per domain, I'm not entirely sure if that is the best options for some reasons:

  • If we consider @markingman needs, I think the option using environment variables that @robingustafsson gave it is a much better approach, once it makes easier to compare the results and there is no need to change the code to do that.
  • Using multiples IPs per domain implies in required use of noConnectionReuse. Otherwise, the IP will be selected only in the first iteration of each VU.
  • Using only one IP, we match the user experience of /etc/hosts.
  • If one need to test multiple servers at once, it will probably put all those servers behind a load balancer instead of using various IPs per domain.

So, I would like to ask if there is something important that I might be missing. Otherwise, I will advise for not letting the options of multiple IPs and follow with the simplest one, and set only one IP per domain.

luizbafilho added some commits Jan 28, 2018

Overriding DNS
This commit adds the hosts options. That enables DNS
overriding similar to /etc/hosts on *nix systems
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Feb 3, 2018

Codecov Report

Merging #494 into master will increase coverage by 0.11%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage   62.37%   62.48%   +0.11%     
==========================================
  Files          93       93              
  Lines        6732     6686      -46     
==========================================
- Hits         4199     4178      -21     
+ Misses       2284     2264      -20     
+ Partials      249      244       -5
Impacted Files Coverage Δ
js/runner.go 85.51% <100%> (+0.1%) ⬆️
lib/options.go 60% <100%> (+2.1%) ⬆️
lib/netext/dialer.go 42.42% <71.42%> (+7.94%) ⬆️
converter/har/converter.go 27.92% <0%> (-2.85%) ⬇️
cmd/options.go 33.33% <0%> (-1.59%) ⬇️
js/modules/k6/http/http_request.go 88.5% <0%> (-0.5%) ⬇️
js/modules/k6/http/response.go 67.36% <0%> (ø) ⬆️
stats/stats.go 51.56% <0%> (ø) ⬆️
stats/cloud/api.go 31% <0%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b843d0a...b495c7e. Read the comment docs.

codecov-io commented Feb 3, 2018

Codecov Report

Merging #494 into master will increase coverage by 0.11%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage   62.37%   62.48%   +0.11%     
==========================================
  Files          93       93              
  Lines        6732     6686      -46     
==========================================
- Hits         4199     4178      -21     
+ Misses       2284     2264      -20     
+ Partials      249      244       -5
Impacted Files Coverage Δ
js/runner.go 85.51% <100%> (+0.1%) ⬆️
lib/options.go 60% <100%> (+2.1%) ⬆️
lib/netext/dialer.go 42.42% <71.42%> (+7.94%) ⬆️
converter/har/converter.go 27.92% <0%> (-2.85%) ⬇️
cmd/options.go 33.33% <0%> (-1.59%) ⬇️
js/modules/k6/http/http_request.go 88.5% <0%> (-0.5%) ⬇️
js/modules/k6/http/response.go 67.36% <0%> (ø) ⬆️
stats/stats.go 51.56% <0%> (ø) ⬆️
stats/cloud/api.go 31% <0%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b843d0a...b495c7e. Read the comment docs.

@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson

robingustafsson Feb 5, 2018

Member

@luizbafilho After giving this some thought, I agree that going with a single IP per hostname should cover most use cases. If one wants a different IP for different operational environments than using env vars is a good solution as discussed in the issue.

The PR looks good to me otherwise 👍

Member

robingustafsson commented Feb 5, 2018

@luizbafilho After giving this some thought, I agree that going with a single IP per hostname should cover most use cases. If one wants a different IP for different operational environments than using env vars is a good solution as discussed in the issue.

The PR looks good to me otherwise 👍

@luizbafilho

This comment has been minimized.

Show comment
Hide comment
@luizbafilho

luizbafilho Feb 5, 2018

Contributor

Nice @robingustafsson I'll change the PR to accept only one IP per domain.

Contributor

luizbafilho commented Feb 5, 2018

Nice @robingustafsson I'll change the PR to accept only one IP per domain.

@markingman

This comment has been minimized.

Show comment
Hide comment
@markingman

markingman Feb 5, 2018

markingman commented Feb 5, 2018

@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson

robingustafsson Feb 5, 2018

Member

Great @markingman, thanks for confirming this covers your use case.

Member

robingustafsson commented Feb 5, 2018

Great @markingman, thanks for confirming this covers your use case.

@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson

robingustafsson Feb 6, 2018

Member

@luizbafilho Thanks! LGTM, merging.

Member

robingustafsson commented Feb 6, 2018

@luizbafilho Thanks! LGTM, merging.

@robingustafsson robingustafsson merged commit ae14ac5 into loadimpact:master Feb 6, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson

robingustafsson Feb 6, 2018

Member

Have updated docs now as well: https://docs.k6.io/v1.0/docs/options

Member

robingustafsson commented Feb 6, 2018

Have updated docs now as well: https://docs.k6.io/v1.0/docs/options

@markingman

This comment has been minimized.

Show comment
Hide comment
@markingman

markingman Feb 7, 2018

markingman commented Feb 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment