sandia-minimega / minimega

minimega
GNU General Public License v3.0
148 stars 67 forks source link

[bridge] Support for bonded interfaces #1486

Closed activeshadow closed 1 year ago

activeshadow commented 1 year ago
activeshadow commented 1 year ago

@aherna this is ready for review and testing.

Also, be sure to let me squash all the commits before merging the PR. I'll do that after you're done with your review and testing.

aherna commented 1 year ago

awesome

@aherna this is ready for review and testing.

Also, be sure to let me squash all the commits before merging the PR. I'll do that after you're done with your review and testing.

Ill test this out over the next few days

aherna commented 1 year ago

@activeshadow See below scenario made a bond for VM r1 for ports 1,2 then cleared a bond vm info shows host |vm | vlans | taps hosta | r1 | [a (101), b (102), c (103)] | [mega_tap9, mega_tap10, mega_tap11]

upon inspection in ovs-vsctl show looks like the taps got put back but the vlan tags are incorrect, both mega_tap10 and 11 show tag of 102 when they should show 102 and 103 respectively

activeshadow commented 1 year ago

@activeshadow See below scenario made a bond for VM r1 for ports 1,2 then cleared a bond vm info shows host |vm | vlans | taps hosta | r1 | [a (101), b (102), c (103)] | [mega_tap9, mega_tap10, mega_tap11]

upon inspection in ovs-vsctl show looks like the taps got put back but the vlan tags are incorrect, both mega_tap10 and 11 show tag of 102 when they should show 102 and 103 respectively

@aherna okay I made a bad assumption here. I think I can fix it pretty easily.

activeshadow commented 1 year ago

@aherna take a look at my two latest commits and let me know if they address your comments.

artherna commented 1 year ago

@activeshadow i think theres a miscommunication somewhere.

there are three bond modes and there are three lacp modes

Lacp can be active, passive, or off and special lacp option for fallback has to be enabled with one of the bond modes.

basically what you had before was fine just wanted an addition of an lacp field

ie Bridge bond <active-backup,balance-slb,…> [active,passive,off]

bridge bond foobond mega_bridge balance-slb active bridge bond foobond mega_bridge balance-slb passive bridge bond foobond mega_bridge balance-slb off

the user can ommit lacp field and just leave lacp active by default.

if you look at the link posted https://www.man7.org/linux/man-pages/man5/ovs-vswitchd.conf.db.5.html#Port_TABLE

activeshadow commented 1 year ago

@aherna I'm causing problems here, sorry! I'll get the LACP mode added as an explicit option to the API.

Note that for balance-tcp, LACP must be enabled (either passive or active) so I'll error out if someone tries to turn it off.

Also, the LACP fallback option is applicable to all modes so I'll leave it enabled by default but still provide a way to disable it.

activeshadow commented 1 year ago

@aherna okay give that last commit a go and let me know what you think.

aherna commented 1 year ago

@activeshadow alright i did a more through test and it looks great*

we need to fix this so that it works in mesh. its not creating the bonds correctly on remote hosts ie when it tries to create the bonds it looks for the vm locally

activeshadow commented 1 year ago

@aherna okay this is working now in a mesh.

activeshadow commented 1 year ago

@aherna latest commits support QinQ for bonded interfaces. I've tested on a mesh. Can you (or do you want to) test one more time before I squash?

artherna commented 1 year ago

Ill do one more test within 30 min

aherna commented 1 year ago

@activeshadow looks good, one thing i saw is qinq field in vm info is blank even though bond is set to have qinq. will need to adjust so that qinq shows up as true for that bond. I think it's because we only set true if it's done at interface level. In teh example below bond4 is qinq and bond5 has no qinq host1 | r2 | [] | [mega_bond5 (mega_tap135 mega_tap136), mega_bond4 (mega_tap133 mega_tap134)]

activeshadow commented 1 year ago

@activeshadow looks good, one thing i saw is qinq field in vm info is blank even though bond is set to have qinq. will need to adjust so that qinq shows up as true for that bond. I think it's because we only set true if it's done at interface level. In teh example below bond4 is qinq and bond5 has no qinq host1 | r2 | [] | [mega_bond5 (mega_tap135 mega_tap136), mega_bond4 (mega_tap133 mega_tap134)]

Whoops okay @aherna I'll fix this.

activeshadow commented 1 year ago

@aherna okay both are fixed. Do you want to test again or do you want me to squash so you can merge?

artherna commented 1 year ago

Ill do a last pass

aherna commented 1 year ago

@activeshadow LGTM squash and ill merge it in!

activeshadow commented 1 year ago

@aherna commits have been squashed. :+1: