add junos_pmtud module #263
Conversation
|
@mkomon I like the idea of this module and will review the code today. On a related note... |
library/junos_pmtud
Outdated
| # Check if ICMP passes | ||
| ping_params['size'] = str(64) | ||
| rpc_reply = dev.rpc.ping(**ping_params) | ||
| loss = int(rpc_reply.find('probe-results-summary').findtext("packet-loss").strip()) |
There was a problem hiding this comment.
This line will throw an exception if the expected XML elements are not present. This could be avoided with something like:
loss = int(rpc_reply.findtext('probe-results-summary/packet-loss', default='100').strip())
(Yes, we need to make similar changes in the junos_ping module in the long run.)
There was a problem hiding this comment.
Thanks for the hint! Fixed.
library/junos_pmtud
Outdated
| step = step / 2 if step >= 2 else 0 | ||
| ping_params['size'] = str(test_size) | ||
| rpc_reply = dev.rpc.ping(**ping_params) | ||
| loss = int(rpc_reply.find('probe-results-summary').findtext("packet-loss").strip()) |
There was a problem hiding this comment.
See comment on line 219.
| ping_params = dict( | ||
| host=m_args['dest_ip'], | ||
| count='3', | ||
| do_not_fragment=True, |
There was a problem hiding this comment.
As it currently stands, a user could enter an IPv6 address for dest-ip (or a hostname which resolves to an IPv6 address) and the module will return a value that is 20 bytes less than the actual IPv6 MTU because it assumes IPv4 sized header.
For now, we can force IPv4 only and avoid an incorrect answer by adding inet=True to ping_params
In the long run, additional arguments and code could be added for other non-IPv4 address families.
There was a problem hiding this comment.
Admittedly, I had only IPv4 in mind when writing this. At the moment I don't have environment with IPv6 running on JUNOS so I agree to limit the module to IPv4 now.
library/junos_pmtud
Outdated
| loss = int(rpc_reply.find('probe-results-summary').findtext("packet-loss").strip()) | ||
| if loss < 100 and test_size == int(m_args['max_size']): | ||
| # ping success with max test_size, save and break | ||
| results["mtu"] = test_size |
There was a problem hiding this comment.
See comment on line 187.
library/junos_pmtud
Outdated
| break | ||
| elif loss < 100: | ||
| # ping success, increase test_size | ||
| results["mtu"] = test_size |
There was a problem hiding this comment.
See comment on line 187.
There was a problem hiding this comment.
At first I didn't get this one. Sure, inet_mtu makes sense, I've pushed a new commit.
library/junos_pmtud
Outdated
| if not results["mtu"]: | ||
| module.fail_json(msg='MTU too low, increase max_range.', **results) | ||
| else: | ||
| results["mtu"] += 28 # adjust for IP and ICMP headers |
There was a problem hiding this comment.
See comment on line 187.
|
@mkomon It looks like you've address all of my feedback except for changing the result key from |
2 participants
Add junos_pmtud module to perform path MTU discovery on Junos devices.
Finds path MTU to destination IP, with defaults it finds MTU in range 989-1500 B in exactly 9 steps (always log2(max_range) steps). Tolerates any packet loss lower than 100 pct.