Add option to specify VU network interface #1293
Conversation
|
My local build is reporting a syntax error: This is a much-needed feature by my team and I'm sure others too. Thank you! |
There was a problem hiding this comment.
I would like to see some tests for this. Not integration/E2E as that would be tricky to setup, but at least that we're properly setting the option from CLI and env var, since that isn't currently working. See https://github.com/loadimpact/k6/blob/7b6b2a684b7f26bbcf1b8549db89614ac398ad54/cmd/config_consolidation_test.go .
Codecov Report
@@ Coverage Diff @@
## master #1293 +/- ##
=========================================
- Coverage 75.44% 75.24% -0.2%
=========================================
Files 150 150
Lines 10885 10916 +31
=========================================
+ Hits 8212 8214 +2
- Misses 2207 2234 +27
- Partials 466 468 +2
Continue to review full report at Codecov.
|
|
Hi Steve ! Thanks for finishing the implementation of this feature. Olivier |
|
@ofauchon Olivier, I couldn't have done it without your initial work! Thank you! At this point all suggested changes are committed. I've tested functionality locally and found no issues so far. The only thing pending from what I can tell is to add tests for this. I looked at the link @imiric posted above, but I'm not really sure where to start. |
| @@ -82,6 +82,7 @@ func optionFlagSet() *pflag.FlagSet { | |||
| flags.StringSlice("tag", nil, "add a `tag` to be applied to all samples, as `[name]=[value]`") | |||
| flags.String("console-output", "", "redirects the console logging to the provided output file") | |||
| flags.Bool("discard-response-bodies", false, "Read but don't process or save HTTP response bodies") | |||
| flags.StringP("nic", "n", "", "Use given network interface") | |||
There was a problem hiding this comment.
Can you use a flags.StringSlice() here? That would allow users to specify multiple NICs by k6 run --nic nic1 --nic nic2, and would also spare us the need to do strings.Split(ifacesList, ",") in js/runner.go.
| var addresses []net.Addr | ||
|
|
||
| for _, ifaceName := range strings.Split(ifacesList, ",") { | ||
| targetIf, err := net.InterfaceByName(ifaceName) | ||
| if err != nil { | ||
| return nil, errors.New("Cannot find network interface '" + ifaceName + "'") | ||
| } | ||
|
|
||
| addrs, err := targetIf.Addrs() | ||
| if err != nil { | ||
| return nil, errors.New("Cannot get addresses on network interface '" + ifaceName + "'") | ||
| } | ||
|
|
||
| addresses = append(addresses, addrs...) | ||
| } |
There was a problem hiding this comment.
This code should probably be called only one time, it doesn't make sense to execute it for every VU initialization. For now a sync.Once() should do, though long-term (i.e. after most of #883 is resolved), we can move most of this logic in a proper config validation layer and have the list of all possible IPs before we even start doing any VU initialization.
| return nil, errors.New("Cannot find any usable addresses on network interface(s) '" + ifacesList + "'") | ||
| } | ||
|
|
||
| rand.Shuffle(len(addresses), func(i, j int) { addresses[i], addresses[j] = addresses[j], addresses[i] }) |
There was a problem hiding this comment.
It doesn't make sense to shuffle an array every time you want to choose a random member of it... You can either use something like addr := addresses[rand.Int()], or better yet, also do the array shuffle only once in the sync.Once() block I mentioned above. Then you can just iterate through it (with an atomic counter to avoid races) like this:
myIPIndex := atomic.AddUint32(&globalIPAddrCounter, 1) -1
myIPAddr := addresses[myIPIndex % len(addresses)]Also, if you're using random numbers of shuffling, you should probably make sure that you Seed() the random number generator, or better yet, make your own local one with https://golang.org/pkg/math/rand/#NewSource
| @@ -285,6 +285,8 @@ type Options struct { | |||
|
|
|||
| // Redirect console logging to a file | |||
| ConsoleOutput null.String `json:"-" envconfig:"K6_CONSOLE_OUTPUT"` | |||
|
|
|||
| Nic null.String `json:"nic,omitempty" envconfig:"K6_NIC"` | |||
There was a problem hiding this comment.
As I mentioned in the CLI flag configuration, this should probably be a string slice.
| tip, _, err := net.ParseCIDR(addresses[0].String()) | ||
| if err != nil { | ||
| return nil, errors.New("Cannot parse IP address from string '" + addresses[0].String() + "'") | ||
| } | ||
|
|
||
| localAddr, err := net.ResolveIPAddr("ip", tip.String()) | ||
| if err != nil { | ||
| return nil, errors.New("Cannot resolve IP '" + addresses[0].String() + "'") | ||
| } |
There was a problem hiding this comment.
Also, these string-y conversions seem strange... addresses already is of type []net.Addr, and that's what Dialer.LocalAddr is as well. So why do we need to go through this?
|
I did try divfor's rebased branch as I was testing for #1303 (comment link). I had the following .. problems:
Of course, after that, we should check that those IPs do actually happen to be assigned to a nic :) @ofauchon, @divfor, @srguglielmo do you think it's better to list the nic and what are your arguments for it?
Other than that this approach seems to work and I did need it to saturate the 1gbps in the test I did in the linked comment above. |
|
@mstoykov, I think I'd also prefer to specify source IPs (and IP ranges) instead of NICs, for multiple reasons:
It just seems somewhat more sensible in general... Somewhat squishy reasoning, I know, but I don't see any negatives if we go with specifying IP ranges, only positives compared to NIC names. |
|
Hi everyone, Thanks for the feedback. To be honest, I'd also agree that specifying IP ranges would be better than the NIC. I built this PR on top of @ofauchon's original PR. I'm not sure I have the skill/time to refactor it entirely to accept CIDR blocks, but I'll try. Should this PR and the original one #757 be closed? I can open a new one referencing issue #476 Thanks! |
|
@srguglielmo just to clarify (because you used the word I find this to be more useful as I am VERY likely in any kind of setup to know what my IPs will be but not so much what my network interface is called (I don't even know if on windows interfaces have names). Also while I agree that listing 20 IPs will be more work then 1 network interface or 1 IP range it is also IMO more easy to understand :D. Also AFAIK you can have 5 network interfaces having the same IP and through which one will actually be used to send the request is up to the OS not us as an application - we can only request that it goes out of certain IP. p.s. |
|
@mstoykov For my specific use case (not sure about others), it wouldn't be practical to specify every IP on the command line. Here's our use case: We are load testing a emergency alerting website running in Azure cloud. Azure has a rate limit on SSL handshakes per IP address (confirmed by Azure support). I think this is a global DDoS protection on their end; Azure support was unable to disable it for our account/service. Our goal was to support 10,000 users hitting the site, so we created five on-prem VMs and assigned them each 400 public IPs (total of 2,000) to work around the Azure limit. I'm able to run the load test now using the I would propose supporting an
It might look at little messy, but it would work for any use case. We could even keep the current code to let it accept a NIC too! The string would be split on |
@mstoykov Of cause I think IPs is better than nic name. On the number of IPs, I think additionally supporting IP range or cidr is better because k6 is often used in load testing cloud services that expect huge number of users from different source IP addresses. a few number of IPs are better for L3/L4 or L7 throughput testing but not better for distributed connection rate testing. |
|
Yeah I guess if you need to specify 200 IPs even using a script to generate the values will make it pretty hard to read and I am pretty sure implementing a IP range or cidr parsing will not be all that complicated so 👍 |
|
|
|
Hi @divfor , @srguglielmo, we have released v0.27.0 last week (:tada: ) and are now again open for non-1007 PRs :tada:! |
This adds a "nic" option (e.g. `k6 run --nic eth0`) which will cause the Virtual User to use the IP address(es) assigned to the given network interface. If the NIC has multiple IPs, they will be used by each VU at random. Credit for the original code goes to @ofauchon. Closes GitHub issue loadimpact#476 and original PR loadimpact#757.
Codecov Report
@@ Coverage Diff @@
## master #1293 +/- ##
==========================================
- Coverage 77.11% 75.24% -1.87%
==========================================
Files 162 150 -12
Lines 13192 10916 -2276
==========================================
- Hits 10173 8214 -1959
+ Misses 2499 2234 -265
+ Partials 520 468 -52
Continue to review full report at Codecov.
|
|
@mstoykov It has been rebased! I might be getting in over my head, but I'll see what I can do regarding the additional improvements discussed above. I'd appreciate any help! Thanks, |
|
hello, I found a way to use a lots of IP as source IP addresses but avoid to add them to interfaces - just add an local route entry， for example: root@STGAX05:~# ip route add local 1.2.3.0/24 dev lo
root@STGAX05:~# ip route show table local | grep /
local 1.2.3.0/24 dev lo scope host
local 127.0.0.0/8 dev lo proto kernel scope host src 127.0.0.1then k6 can bind any of 1.2.3.0/24 to start loading testing, as long as those IPs are routable back from outside to k6. |
@srguglielmo root@STGAX04:~# ip route show table local | grep kernel
local 127.0.0.0/8 dev lo proto kernel scope host src 127.0.0.1
local 127.0.0.1 dev lo proto kernel scope host src 127.0.0.1
local 192.168.250.104 dev eth0 proto kernel scope host src 192.168.250.104
root@STGAX04:~# ping 127.0.2.4
PING 127.0.2.4 (127.0.2.4) 56(84) bytes of data.
64 bytes from 127.0.2.4: icmp_seq=1 ttl=64 time=0.063 ms
^C
--- 127.0.2.4 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.063/0.063/0.063/0.000 msgrep non-kernel IP addresses: root@STGAX04:~# ip route show table local | grep -v kernel
local 27.126.195.0/24 dev lo scope host
local 27.126.207.0/24 dev lo scope host
local 103.3.24.0/24 dev lo scope host
root@STGAX04:~# ping 103.3.24.123
PING 103.3.24.123 (103.3.24.123) 56(84) bytes of data.
64 bytes from 103.3.24.123: icmp_seq=1 ttl=64 time=0.048 ms
^Cif so, k6 can enable to use IP addresses that are not configured on any interfaces as src IP -- this is good news for cloud-based OS like kvm/docker. |
|
or simply, k6 does not need to check any existence of given cidr/ip-range, just shuffle and use them to connect servers. Setup: root@STGAX06:~# ip link set k6 type dummy
root@STGAX06:~# ifconfig k6
k6: flags=130<BROADCAST,NOARP> mtu 1500
inet 10.60.0.1 netmask 255.255.0.0 broadcast 0.0.0.0
ether 42:80:20:9f:8f:12 txqueuelen 1000 (Ethernet)
RX packets 0 bytes 0 (0.0 B)
RX errors 0 dropped 0 overruns 0 frame 0
TX packets 0 bytes 0 (0.0 B)
TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
root@STGAX06:~# ip route add local 10.12.0.0/16 dev k6
root@STGAX06:~# ip route show table local | grep ^local | grep /
local 10.12.0.0/16 dev k6 scope hostRun K6 using 65536 source IP addresses for VUs root@STGAX06:~# k6 run --cidr 10.12.0.0/16 test.jsor run K6 using 2 x 256 source IP addresses for VUs root@STGAX06:~# k6 run --cidr 10.12.0.0/24 --cidr 10.17.0.0/24 test.jsTeardown: root@STGAX06:~# ip route del local 10.12.0.0/16 dev k6
root@STGAX06:~# ip link del k6If someone want to multiple NICs to using k6, just set to Linux the egress routes via correct NIC interface. |
|
Assigning millions of IPs to a server: |
|
@srguglielmo sorry I didn't see you were asking for help ... I don't know how I can help you apart from writing the code :D . We have internally decided that after almost 2 years of this and the previous PRs being around we will actually push this ourselves to a mergeable PR. Thanks a lot to everybody for the PRs and discussions, this has been really helpful, and sorry that this taking so long, but we will get it in 0.29.0 ... I promise ™️ ;) So you should expect that we will make a new PR withing a couple of weeks fixing the remaining issues ... I am not really certain how we will manage to test this but will try to figure something out ;) |
|
Thank you so much! Should I close this PR or leave it open for now? |
|
@srguglielmo I guess I will close it :D |
This adds a "nic" option (e.g.
k6 run --nic eth0) which will cause theVirtual User to use the IP address(es) assigned to the given network
interface. If the NIC has multiple IPs, they will be used by each VU at
random.
Credit for the original code goes to @ofauchon.
Closes GitHub issue loadimpact#476 and original PR loadimpact#757.