Add bridge service by gianlu33 · Pull Request #125 · polycube-network/polycube

gianlu33 · 2019-05-11T12:55:23Z

L2 bridge service, features:

VLAN support
Per-VLAN STP

mauriciovasquezbernal

Hello @gianlu33,

The code overall looks good to me, I some comments of things to be improved, just to summarize:

trunk and access inside the ports are very similar to the stp case we already discussed, those objects are created automatically by the code and the user is not able to create nor delete them, additionally those objects should only exists when the port is configured in such mode, this will avoid all the checks of port mode in PortsTrunk and PortsAccess.
there are some functions that are not used, they can be removed.
remove this-> and unseful comments, add copyright headers.

mauriciovasquezbernal

Some minor details yet to fix, overall looks very good.
I'll try to do some tests in few hours/days.

mauriciovasquezbernal

It looks very food to me, kudos on the work.

There are two small details that I would like to get improved.

The 300 constant is used some times in the code, this should be avoided as this is defined in the datamodel.
When the bridge is created this is possible that the code has to be compiled twice, the first one in the bridge constructor and the second one when setAgingTime. It would be nice to rework the code a little bit so it takes the correct value of the aging time from he beginning and and compiles just once.

Features: - The datapath code is regenerated automatically whenever stp is enabled or disabled, so it is not necessary to configure that parameter at the creation of the bridge. - For each port, there are one or more PortsStp instances according to the VLANs allowed for that port. - A Stp instance is created automatically, when the first PortStp instance for the same VLAN is created. - In the same way, a Stp instance is destroyed automatically, when the last PortsStp instance for the same VLAN is destroyed. - Since the management of Stp/PortsStp instances is done automatically, the user is not allowed to create/destroy manually those instances, but only to configure their parameters. - Containers Access/Trunk do not exist at the same time; if the port is configured in access mode the user can't configure trunk parameters and vice versa Signed-off-by: Gianluca Scopelliti <gianlu.1033@gmail.com>

These tests verify the correctness of the bridge. In each folder there are tests for each feature (connectivity, vlan, stp, vlan_and_stp). Signed-off-by: Gianluca Scopelliti <gianlu.1033@gmail.com>

Add documentation for bridge, with three examples: - Connectivity - VLAN configuration - STP configuration Signed-off-by: Gianluca Scopelliti <gianlu.1033@gmail.com>

frisso · 2019-06-06T12:49:36Z

Really great, @gianlu33 (and also @mauriciovasquezbernal)!

gianlu33 requested a review from a team as a code owner May 11, 2019 12:55

gianlu33 force-pushed the project_stppbridge branch 2 times, most recently from 295b398 to 821c556 Compare May 14, 2019 21:00

mauriciovasquezbernal reviewed May 15, 2019

View reviewed changes

acloudiator assigned gianlu33 May 17, 2019

acloudiator added the under-review label May 17, 2019

gianlu33 force-pushed the project_stppbridge branch 7 times, most recently from 66f157d to 2860c23 Compare May 20, 2019 10:35

mauriciovasquezbernal reviewed May 20, 2019

View reviewed changes

gianlu33 force-pushed the project_stppbridge branch 5 times, most recently from 06b95a7 to 7a55e89 Compare May 23, 2019 16:20

mauriciovasquezbernal approved these changes May 23, 2019

View reviewed changes

Comment thread src/services/pcn-bridge/src/Bridge.cpp Outdated

Comment thread src/services/pcn-bridge/src/Bridge.cpp Outdated

Comment thread src/services/pcn-bridge/src/Bridge.cpp Outdated

Comment thread src/services/pcn-bridge/src/Fdb.h Outdated

mauriciovasquezbernal reviewed May 23, 2019

View reviewed changes

Comment thread src/services/pcn-bridge/src/Fdb.cpp Outdated

gianlu33 force-pushed the project_stppbridge branch from 7a55e89 to 77f7867 Compare May 24, 2019 09:52