Implementing advmss ip route option#489
Conversation
slyon
left a comment
There was a problem hiding this comment.
Thank you very much for your contribution to Netplan, this PR is looking great! You covered all the backends and added nice testing around it. Kudos!
Could you please move the integration test (in routinging.py) up into the _CommonTests class? So we can check it actually works on NetworkManager, in addition to test_route_advmss (__main__.TestNetworkd) ... eth42 .............ok, e.g. next to test_per_route_advertised_receive_window.
Also, please add the new setting to the fuzzer config in tests/config_fuzzer/schemas/common.js:
"advertised-mss": {
type: "integer",
minimum: 0
}
Besides that, I left a few small inline comments. Also, let's wait for some comments from @vorlonofportland and @daniloegea.
slyon
left a comment
There was a problem hiding this comment.
Thank you very much for adding proper integration tests!
The PR overall LGTM!
But: CI is still failing the NetworkManager tests... this seems to be because the advmss route_option (https://networkmanager.dev/docs/api/latest/nm-settings-nmcli.html) is only supported since NetworkManager 1.39.8 (1.40.0 stable) (NetworkManager/NetworkManager@90e7afc), which is not available in Ubuntu Jammy, that is still used in our CI system currently.
I see this output on Jammy, while it works fine on Noble:
# nmcli connection modify netplan-eth0 +ipv4.routes "192.168.122.0/24 10.10.10.1 advmss=1400"
Error: failed to modify ipv4.routes: invalid option 'advmss=1400': unknown attribute 'advmss'.
So we might need to block this PR on #465, or alternatively, mark the corresponding NetworkManager tests to be skipped on NetworkManager < 1.39.8, e.g. something along those lines:
import gi
gi.require_version("NM", "1.0")
from gi.repository import NM
print("version:", NM.Client.new(None).get_version())And then @unittest.skipif() backend is NetworkManager and version is too small.
| 'advertised_receive_window': next_value.advertised_receive_window, | ||
| 'onlink': next_value.onlink | ||
| 'onlink': next_value.onlink, | ||
| 'advertised_mss': next_value.advmss |
There was a problem hiding this comment.
I'm not sure about this one. But considering this is part of a private data structure _NetdefRouteIterator, accessing internal data structures without a getter should be acceptable.
95d0188 to
8c880b1
Compare
…rk manager. Param name for systemd networkd: TCPAdvertisedMaximumSegmentSize Param name for network manager: advmss
8c880b1 to
104b239
Compare
|
I implemented the test skipping, rebased and squashed your commits. This should now be ready for merging! Thanks a lot for your contribution to Netplan :) |
Hello everybody. I was missing the advmss functionality for static routes, and I decided to implement it. This option has been tested successfully in systemd-networkd, but has not tested in network manager.
Param name for ip route: advmss
Param name for systemd networkd: TCPAdvertisedMaximumSegmentSize
Param name for network manager: advmss
Checklist
make checksuccessfully.make check-coverage).