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

Implementing advmss ip route option #489

Merged
merged 3 commits into from
Jul 24, 2024

Conversation

barvius
Copy link

@barvius barvius commented Jul 14, 2024

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

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@slyon slyon added the community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. label Jul 15, 2024
@slyon slyon self-requested a review July 17, 2024 10:44
@slyon slyon added the schema change This PR includes a change to netplan's YAML schema, which needs sign-off label Jul 17, 2024
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

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.

doc/netplan-yaml.md Outdated Show resolved Hide resolved
python-cffi/netplan/netdef.py Outdated Show resolved Hide resolved
python-cffi/netplan/netdef.py Outdated Show resolved Hide resolved
src/netplan.c Outdated Show resolved Hide resolved
src/parse.c Outdated Show resolved Hide resolved
src/types-internal.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

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.

@@ -388,7 +390,8 @@ def __next__(self):
'mtubytes': next_value.mtubytes,
'congestion_window': next_value.congestion_window,
'advertised_receive_window': next_value.advertised_receive_window,
'onlink': next_value.onlink
'onlink': next_value.onlink,
'advertised_mss': next_value.advmss
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@slyon slyon force-pushed the implement-ip-route-advmss branch 2 times, most recently from 95d0188 to 8c880b1 Compare July 24, 2024 13:27
Oleg and others added 3 commits July 24, 2024 15:33
@slyon slyon force-pushed the implement-ip-route-advmss branch from 8c880b1 to 104b239 Compare July 24, 2024 14:49
@slyon
Copy link
Collaborator

slyon commented Jul 24, 2024

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 :)

@slyon slyon merged commit b13b518 into canonical:main Jul 24, 2024
15 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. schema change This PR includes a change to netplan's YAML schema, which needs sign-off schema ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants