Skip to content

Conversation

@jbemmel
Copy link
Collaborator

@jbemmel jbemmel commented Feb 10, 2025

Context: #1835

This PR adds support for VTEP redundancy, by provisioning a shared IP across both MLAG peers.

Includes - tested - support for:

@jbemmel jbemmel marked this pull request as draft February 10, 2025 01:09
@jbemmel jbemmel force-pushed the mlag_vtep_plugin_new branch from 32ce37c to 7c91ecc Compare February 10, 2025 01:21
@jbemmel jbemmel force-pushed the mlag_vtep_plugin_new branch from 4eacfe8 to 36c4ada Compare February 10, 2025 01:32
@jbemmel jbemmel marked this pull request as ready for review February 10, 2025 02:50
@ipspace
Copy link
Owner

ipspace commented Feb 10, 2025

Are you sure there's need for such a convoluted solution? Wouldn't it be better to create an extra loopback link per node, set static IP on it, and set vxlan.vtep to true. Also, use the existing address pool functionality instead of building your own.

Don't reinvent the wheel where it's not needed.

Closing this PR as I assume it will be cleaner to start from scratch.

@ipspace ipspace closed this Feb 10, 2025
@jbemmel
Copy link
Collaborator Author

jbemmel commented Feb 10, 2025

Are you sure there's need for such a convoluted solution? Wouldn't it be better to create an extra loopback link per node, set static IP on it, and set vxlan.vtep to true.

At its core, that's what this 80-line plugin does (lines 59-67):

# Add an extra loopback interface with the allocated VTEP IP
     vtep_loopback = data.get_empty_box()
     vtep_loopback.type = 'loopback'
     vtep_loopback.name = f"MLAG VTEP VXLAN interface shared between {' - '.join([i.node for i in peers])}"
     vtep_loopback.ipv4 = node.lag.mlag.vtep + "/32"
     vtep_loopback.vxlan.vtep = True
     links.create_virtual_interface(node, vtep_loopback, topology.defaults)
     if 'ospf' in node.get('module',[]):    # Add it to OSPF when used, TODO ISIS
       vtep_loopback.ospf = { 'area': "0.0.0.0", 'passive': True }
     node.interfaces.append( vtep_loopback )

Some devices - like Cumulus NVUE - have their own way of configuring this loopback, so those use a custom script instead

Also, use the existing address pool functionality instead of building your own.

Lines 27-30:

def topology_expand(topology: Box) -> None:
    # Create address pool to check for overlap with other address ranges
    topology.addressing[POOL_NAME] = { 'ipv4'   : topology.defaults.mlag.vtep.address_pool,
                                       'prefix' : 32 }

This adds a separate address pool to the existing functionality, with fallback to the regular loopback pool. I could tweak the logic to allow the user to just use the regular loopback pool, but for debugging/tracing purposes I think it can be helpful if MLAG VTEP addresses are distinct from ordinary loopbacks.

Don't reinvent the wheel where it's not needed.

Closing this PR as I assume it will be cleaner to start from scratch.

I could draft a clean PR, but IMHO the functionality you're asking for is already there - I don't see how I could submit something fundamentally different

@ipspace
Copy link
Owner

ipspace commented Feb 10, 2025

Look, if other people invest time into commenting your stuff, you should at least carefully read what they wrote. For example,

Wouldn't it be better to create an extra loopback link per node

Here's the exact data structure you would have to use: https://netlab.tools/module/vxlan/#changing-the-vxlan-vtep-source-interface-address

Creating a loopback link before link transformation would eliminate all the hassle and the dirty hacks you had to go through to make the plugin work for one use case you had in mind -- OSPF running in area 0. Instead of reinventing a badly-designed wheel, everything would work out of the box.

There would be no need to add feature flags. Either multiple loopbacks, MLAG, and VXLAN work or not.

Similarly for the address pools. Why do you have to invent a new attribute and convert it into an address pool if the address pools work just fine out of the box? Just add an address pool in plugin defaults.

Finally, you should use vrf_loopback pool as the fallback. Addresses are taken sequentially from the vrf_loopback pool while the loopback address allocation uses node ID.

@jbemmel
Copy link
Collaborator Author

jbemmel commented Feb 10, 2025

Look, if other people invest time into commenting your stuff, you should at least carefully read what they wrote.

I wasn't following what you were saying, but now I see what you mean. Will rework things

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