Skip to content

NE-1259 Enforce MTU correctness - #137

Draft
loulecrivain wants to merge 6 commits into
mainfrom
fix/ne-1259-mtu-enforce
Draft

loulecrivain wants to merge 6 commits into
mainfrom
fix/ne-1259-mtu-enforce

Conversation

@loulecrivain

Copy link
Copy Markdown
Contributor

No description provided.

@loulecrivain

Copy link
Copy Markdown
Contributor Author

currently on hold as draft implementation is not satisfactory

@loulecrivain
loulecrivain force-pushed the fix/ne-1259-mtu-enforce branch from 276bfa9 to db28202 Compare October 13, 2026 10:17
@loulecrivain

Copy link
Copy Markdown
Contributor Author

1st draft PR ready for review and validation of general concept

@loulecrivain
loulecrivain force-pushed the fix/ne-1259-mtu-enforce branch from db28202 to 80cc5d4 Compare October 14, 2026 10:53
@loulecrivain
loulecrivain force-pushed the fix/ne-1259-mtu-enforce branch from 80cc5d4 to 4e2b178 Compare October 14, 2026 11:02
@loulecrivain
loulecrivain marked this pull request as ready for review October 14, 2026 12:39
@loulecrivain
loulecrivain requested a review from unfokus October 14, 2026 12:39

@unfokus unfokus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

First of all, good work! I like the modular implementation style, the refactoring of cosmo is really paying off.
There are some small misunderstandings as for the logic/platform behavior, see my other comments about that. But I think that should be easy to fix. Message me if there are any doubts.

Comment thread cosmo/l2vpnhelpertypes.py Outdated
Comment thread cosmo/common.py Outdated
Comment thread cosmo/routerl2vpnvisitor.py Outdated

class RouterL2VPNValidatorVisitor(AbstractL2VPNVisitor):
# 8021ad (vlan-bridge) and 8021q (vlan-ccc) encaps auto-sets phy MTU to correct MTU if logical MTU is not specified
MAX_DEFAULT_PHY_MTU = DEFAULT_LOGICAL_MTU + IEE80213_HDR_BYTES + IEE80213_HDR_BYTES

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why 802.3 header (twice)?
Since this is a L2 Tunnel, the L2 header is part of the payload. So for L2 tunnels without VLAN encap/decap we don't need to account for any overhead, the physical MTU can be equal to the logical MTU. If a VLAN is added/removed, we only need to account for the VLAN header.



class RouterL2VPNValidatorVisitor(AbstractL2VPNVisitor):
# 8021ad (vlan-bridge) and 8021q (vlan-ccc) encaps auto-sets phy MTU to correct MTU if logical MTU is not specified

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is the wrong way around.
The encap on the logical interface does not set the physical MTU. The physical MTU is handed down to the logical.
So if I set my physical MTU to 9000, the logical interface MTU is inherited from the physical.
When I set the logical MTU to 9000 but leave the physical unconfigured, the logicals is clamped by the physicals default (1514 or so).

So I think we should rather have the default logical interface MTU as a constant, as there is no such thing as a default logical interface MTU, and calculate the inherited logical interface MTU from the physicals MTU + encapsulation overhead.
(We can later implement that for L3 interfaces as well and clean up some of the hardcoded logic there!)

Comment thread cosmo/l2vpnhelpertypes.py Outdated
@loulecrivain
loulecrivain marked this pull request as draft October 14, 2026 13:30
@loulecrivain

Copy link
Copy Markdown
Contributor Author

TODO: also implement checks for vlan terminated L2VPNs

@loulecrivain
loulecrivain force-pushed the fix/ne-1259-mtu-enforce branch from 96c4626 to 8ddddd5 Compare October 20, 2026 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants