Skip to content

Conversation

@jbemmel
Copy link
Collaborator

@jbemmel jbemmel commented Oct 10, 2024

A variation on the earlier PR:

links:
- lag.members: [ r1-r2, r1-r2 ]

This forces a check for the lag module when using this functionality, it is more concise

Replaces #1337

@jbemmel jbemmel marked this pull request as draft October 10, 2024 18:58
@jbemmel jbemmel marked this pull request as ready for review October 10, 2024 19:49
@jbemmel jbemmel requested a review from ipspace October 10, 2024 19:49
@ipspace
Copy link
Owner

ipspace commented Oct 11, 2024

You keep adding stuff, so I put the PR back into draft. Please let me know when you're done.

@ipspace ipspace marked this pull request as draft October 11, 2024 14:40
@ipspace
Copy link
Owner

ipspace commented Oct 16, 2024

@jbemmel Is this final and ready for review? Thank you!

@jbemmel jbemmel marked this pull request as ready for review October 16, 2024 10:20
@ipspace
Copy link
Owner

ipspace commented Oct 20, 2024

Found an interesting case that crashes netlab: define LAG link group without enabling the LAG module, and the link transformation crashes:

provider: clab

nodes:
  r1:
  r2:

links:
- group: lg1
  type: lag
  members: [ r1-r2, r1-r2 ]

I think this should be fixed. Alternatively, I could merge the PR and then rework it to use lag.members attribute which would force LAG module to be used. It would also get rid of the group link code.

FWIW, if I use...

links:
- lag.members: [ r1-r2, r1-r2 ]

... then netlab complains about missing module before it complains about the lack of nodes on the link, so it looks like we could do the transformation in a module (i.e. it would not require a plugin)

How would you like to proceed?

@jbemmel
Copy link
Collaborator Author

jbemmel commented Oct 20, 2024

How would you like to proceed?

Thanks for reviewing, I will rework it to use lag.members instead (implying link.type='lag')

@ipspace
Copy link
Owner

ipspace commented Oct 20, 2024

Wow, this was super-fast. Will review it tomorrow, it's getting late over here.

@jbemmel jbemmel changed the title lag based on link groups lag based on lag.members syntax Oct 20, 2024
@ipspace
Copy link
Owner

ipspace commented Oct 21, 2024

Great job, just a ton of minor suggestions ;)

* Check that link.members is a list
* Change naming of links
* Update test results
@jbemmel jbemmel requested a review from ipspace October 24, 2024 23:44
@ipspace ipspace merged commit 4cabdea into ipspace:dev Oct 25, 2024
5 checks passed
@jbemmel jbemmel deleted the linkgroup-lags branch November 8, 2024 12:19
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