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

lag: Remove 'lag' link type, cleanup virtual links #1646

Merged
merged 36 commits into from
Jan 3, 2025

Conversation

jbemmel
Copy link
Collaborator

@jbemmel jbemmel commented Dec 13, 2024

Conceptually a 'lag' link type does not make a whole lot of sense; the lag module temporarily uses a link object as a grouping construct until the node lag interfaces are created, but there is no reason to keep this around once that is done

This PR cleans things up a little:

  • Replace the "lag" link.type with a "_virtual_lag" flag, and remove those links during post processing
  • Link lag members to the parent device through lag.ifindex, not linkindex (which is now removed from the lag interface)
  • Temporarily uses '_type: lag' interface attribute to pass validation, then renames it to 'type' for backwards compatibility

Includes various bug fixes:

  • Testing and handling of lag attribute inheritance (interface level attributes weren't being considered)
  • Remove MTU from lag interfaces
  • Refactoring of lag module structure (separate 'lag' interface creation)

Copy link
Owner

@ipspace ipspace left a comment

Choose a reason for hiding this comment

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

The "type" interface attribute should not be defined, otherwise it looks reasonable.

netsim/defaults/attributes.yml Outdated Show resolved Hide resolved
netsim/augment/links.py Outdated Show resolved Hide resolved
@ipspace
Copy link
Owner

ipspace commented Dec 13, 2024

Also, what happened to "I will submit a PR when I'm done programming"? Got three notifications about this PR.

@jbemmel jbemmel marked this pull request as draft December 13, 2024 15:20
@jbemmel jbemmel force-pushed the remove-lag-link-type branch from 4702a67 to c812b6c Compare December 14, 2024 17:30
@jbemmel jbemmel force-pushed the remove-lag-link-type branch 5 times, most recently from 43d736c to 3fd9f11 Compare December 15, 2024 18:21
@jbemmel jbemmel marked this pull request as ready for review December 15, 2024 18:30
@ipspace
Copy link
Owner

ipspace commented Dec 23, 2024

Please resolve the merge conflicts first. Thank you!

@ipspace ipspace marked this pull request as draft December 23, 2024 06:17
@jbemmel jbemmel force-pushed the remove-lag-link-type branch from ce43daf to c0adea1 Compare December 27, 2024 21:42
@ipspace
Copy link
Owner

ipspace commented Dec 28, 2024

Looks like you're still working on this one. Change it to "ready for review" when you're done.

@jbemmel jbemmel force-pushed the remove-lag-link-type branch from dc9eae3 to 0d182ec Compare January 1, 2025 17:33
@ipspace
Copy link
Owner

ipspace commented Jan 2, 2025

What's going on with this one? Is it ready to merge (I won't even try to review it)?

@jbemmel
Copy link
Collaborator Author

jbemmel commented Jan 2, 2025

What's going on with this one? Is it ready to merge (I won't even try to review it)?

Waiting on the other lag PR to be merged, then I'll update this one

jbemmel added 11 commits January 2, 2025 16:30
* Modify lag member lag._parentindex to match lag.ifindex, not linkindex
…g interfaces get created

* Fix the moment lag interfaces get created: After interface attribute validation, but before vlan pre link transformation callbacks

Some issue remains with IP addresses being allocated to lag interfaces instead of vlan SVI
TODO: Validate all VLAN cases with/without lag
@jbemmel jbemmel force-pushed the remove-lag-link-type branch from 128221d to 13bf985 Compare January 2, 2025 22:43
@jbemmel jbemmel marked this pull request as ready for review January 3, 2025 00:31
@ipspace ipspace merged commit 469d2d5 into ipspace:dev Jan 3, 2025
6 checks passed
@jbemmel jbemmel deleted the remove-lag-link-type branch January 8, 2025 18:12
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