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.

Sign up for GitHub

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

Jump to bottom

node.clone plugin - simplify repetitive topologies by cloning nodes #1616

Merged
merged 19 commits into from
Dec 10, 2024
Merged

node.clone plugin - simplify repetitive topologies by cloning nodes #1616

merged 19 commits into from
Dec 10, 2024

Conversation

jbemmel
Copy link
Collaborator

@jbemmel jbemmel commented Dec 5, 2024 •
edited
Loading

A plugin to clone nodes and any links/interfaces they are attached to, to simplify provisioning of large clusters of identical devices (like servers in a rack)

Context: #1570

Now includes support for lag links

---
plugin: [ node.clone ]

module: [vlan]

vlans:
  red:
    links: [r-h]        # VLAN links get updated

nodes:
 r:
  device: frr
 h:
  device: linux
  clone.count: 32       # Create 32 hosts

@jbemmel
Copy link
Collaborator Author

jbemmel commented Dec 5, 2024

Ready for review

@jbemmel jbemmel requested a review from ipspace December 5, 2024 20:42
@jbemmel
Copy link
Collaborator Author

jbemmel commented Dec 6, 2024

Apologies for the additional notifications

@Astrotokii
Copy link

Could this be applied to link groups for simplifying etherchannels and redundancy?

I.e.

-group: etherchannel
members: [r-h]
repeat: 1

@ipspace
Copy link
Owner

ipspace commented Dec 7, 2024

Just a suggestion on the name change (as "repeater" can mean anything -- see https://en.wikipedia.org/wiki/Repeater_(disambiguation) ):

  • Rename the plugin to "node.clone"
  • Use "clone" attributes like "clone.count" (for a total count) and "clone.start" (potentially also "clone.step")

If I read the code right, the original node is retained, which I would find confusing -- I would get "h, h-01, h-02, ... h-31" for a total of 32 nodes. I think the first node should be "h-01" (or "h-00" -- this is where "clone.start" would come handy) and the count (however it is called) should specify the total number of nodes.

ipspace
ipspace requested changes Dec 7, 2024
View reviewed changes
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.

Made a few suggestions (if nothing else, I would strongly recommend using "_" in node names). Also, the plugins needs documentation.

netsim/extra/repeater/defaults.yml Outdated Show resolved Hide resolved
netsim/extra/repeater/plugin.py Outdated Show resolved Hide resolved
netsim/extra/repeater/plugin.py Outdated Show resolved Hide resolved
netsim/extra/repeater/plugin.py Outdated Show resolved Hide resolved
netsim/extra/repeater/plugin.py Outdated Show resolved Hide resolved
netsim/extra/repeater/plugin.py Outdated Show resolved Hide resolved
netsim/extra/repeater/plugin.py Outdated Show resolved Hide resolved
@jbemmel jbemmel marked this pull request as draft December 7, 2024 14:12
@jbemmel jbemmel changed the title Repeater plugin - simplify repetitive topologies by cloning nodes node.clone plugin - simplify repetitive topologies by cloning nodes Dec 7, 2024
@jbemmel
Copy link
Collaborator Author

jbemmel commented Dec 7, 2024

Could this be applied to link groups for simplifying etherchannels and redundancy?

I.e.

-group: etherchannel members: [r-h] repeat: 1

It could, but the lag module already greatly simplifies the definition of multi-link etherchannels; the attributes can be specified once for the group, and then the list of links is very short for typical deployment cases

links:
- lag.members: [r-h, r-h, r-h, r-h]

is simpler (and less characters) than:

links:
- lag:
   members: [r-h]
   clone.count: 4

@jbemmel
Copy link
Collaborator Author

jbemmel commented Dec 7, 2024 •
edited
Loading

Ready for review

  • Perhaps link groups could be supported transparently if they were transformed before the plugin(s) get invoked?
    Same for VRF and VLAN links - it would be much easier to have a normalized topology to operate on
    (and perhaps if we really want, an additional plugin callback before normalization)

  • It would simplify things if link normalization would happen before invoking topology-changing plugins

@jbemmel jbemmel marked this pull request as ready for review December 7, 2024 17:30
@jbemmel jbemmel marked this pull request as draft December 7, 2024 17:41
@jbemmel jbemmel marked this pull request as ready for review December 7, 2024 23:43
ipspace
ipspace requested changes Dec 9, 2024
View reviewed changes
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.

Mostly suggestions, the only truly annoying part is the "vlans" link name in VRF links.

netsim/extra/node.clone/plugin.py Outdated Show resolved Hide resolved
netsim/extra/node.clone/plugin.py Outdated Show resolved Hide resolved
netsim/extra/node.clone/plugin.py Outdated Show resolved Hide resolved
netsim/extra/node.clone/plugin.py Outdated Show resolved Hide resolved
netsim/extra/node.clone/plugin.py Outdated Show resolved Hide resolved
@jbemmel jbemmel marked this pull request as draft December 9, 2024 15:20
jbemmel added 13 commits December 9, 2024 15:59
@jbemmel
* Support VLAN lists and groups too
d25518c
@jbemmel
Fix typing
0feee57
@jbemmel
Bugfix: Need to clone interface, else ifindex references previous obj…
0a08852 
…ects
@jbemmel
Remove unused imports, add documentation
24bcab0
@jbemmel
* Adjust node ID offset
390964f 
* Copy any interface attributes from the original node too
@jbemmel
BUGFIX: Groups may include 'bool' flags, test for that
ad5cfeb
@jbemmel
Shorten node names to 16 characters
094ddc7
@jbemmel
* Renamed to 'node.clone'
0eb271b 
* Add clone.count, clone.start and clone.step parameters
* Replace cloned node by first clone (implies LAG links now generate an error, cannot leave them in place)
* Keep link names
* Simplify link cloning logic
* Update documentation

TODO: Generate errors when unsupported constructs are used in topology
@jbemmel
* Add support for VRF links
947f18e 
Note: link groups could in theory be supported transparently if the transformation got applied before the plugin gets invoked.
Similarly, it would be easier if link interfaces were normalized before plugins get invoked
@jbemmel
Wait to assign the new name, else clones may get wrong names
ae2dbdc
@jbemmel
Test both short and long hostnames
65705e2
@jbemmel
* Use start and step parameters, test them
b5e20b1 
* Use '-' in node name template
@jbemmel
Resolve feedback
a32130a 
* implement separate 'process_links'
* don't treat clone 1 different
* use proper name prefix for cloned links
* rename 'clone_interfaces'
@jbemmel
Copy link
Collaborator Author

jbemmel commented Dec 10, 2024

Hit an issue with lag.ifindex value overlap (see #1634) but ready for review

@jbemmel jbemmel marked this pull request as ready for review December 10, 2024 05:42
@ipspace ipspace merged commit 7a6a0a1 into ipspace:dev Dec 10, 2024
5 checks passed
ipspace added a commit that referenced this pull request Dec 10, 2024
@ipspace
Rebuild the #1616 tests after #1634
743db8a 
Note to @jbemmel: it makes no sense to have two transformation
tests testing the same (LAG) functionality. Try to keep tests
focused, throwing extra features in just for the fun of it
makes it harder to figure out what went wrong.

Also: the changed results have an empty 'lag' dictionary (see
diff for more details)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ipspace ipspace ipspace approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jbemmel @Astrotokii @ipspace