Multiple Spanning Tree Protocol (MSTP) HLD #1298
Conversation
hamnarauf
force-pushed
the
mstp_hld
branch
3 times, most recently
from
5321c1e
to
7d5eb3e
Compare
|
@zhangyanzhao can you please assign this HLD to @hamnarauf Thanks! |
|
SONiC community review recording https://zoom.us/rec/share/3XxqJ20eag07iK0x7T_4YqBq6jhbyWXGjpk72AvHL35peWSOXZ04w_ZQjGlwXJbM.0DPyXu03R_bS0Gry |
doc/stp/MSTP_HLD.md
Outdated
| ||SAI_SWITCH_ATTR_MAX_STP_INSTANCE| | ||
|
|
||
| ## New SAI Attributes | ||
| MSTP design requires one new attribute `SAI_HOSTIF_TRAP_TYPE_MSTP` for control trap packets which will be defined in saihostif.h. |
There was a problem hiding this comment.
Please explain why SAI_HOSTIF_TRAP_TYPE_STP is not sufficient. The DMAC and LLC fields are identical for STP and MSTP. Creating separate trap types for STP and MSTP would required the hardware to parse/match the BPDU protocol identifier.
There was a problem hiding this comment.
We can discuss this in coming SAI meeting. For reference, please see this PR Add hostif trap for MSTP
|
@lguohan , Please find following are unmerged PRs of PVST, that needs to merged for MSTP: |
|
@zhangyanzhao pls add @wajahatrazi as assignee for this HLD. |
|
@hamnarauf can you please help to add the code PR list in this HLD? Thanks. |
Hi @zhangyanzhao , Hamna is no longer assignee on this feature. Please consider @Wajahat as assignee on this from now onward. We are working on MSTP code and waiting for @adyeung help in unmereged code PR of PVST. Thanks! |
|
@ridahanif96 can you please help to add the code PRs to this HLD by referring to #806 ? Thanks! |
@zhangyanzhao thanks for help! |
| |Switch STP Attributes|SAI_SWITCH_ATTR_DEFAULT_STP_INST_ID| | ||
| ||SAI_SWITCH_ATTR_MAX_STP_INSTANCE| | ||
|
|
||
| ## New SAI Attributes |
There was a problem hiding this comment.
Discussed in SAI Community Call, no new trap will be created and we will use the existing traps as DMAC and LLC fields are identical for STP and MSTP.
doc/stp/MSTP_HLD.md
Outdated
| # Configuration Commands | ||
| Following configuration commands will be provided for configuration of MSTP: | ||
| ## Global Level | ||
| - **config spanning_tree {enable|disable} {mstp|pvst}** |
There was a problem hiding this comment.
Are we going to support pvst mode also? There are variations of PVST like PVST+ and RPVST+. Do we support all the STP variations?
There was a problem hiding this comment.
@balajib-cisco , SONiC already has PVST mode, proposed in HLD. We are proposing MSTP mode.
There was a problem hiding this comment.
AFAIK, PVST mode support is not committed/merged to SONiC community branch. Those PVST PRs are still open sonic-net/sonic-swss#1058. Are you going to commit those along with this MSTP commit? How are we going to coexist?
There was a problem hiding this comment.
Yes, there are some open PRs for PVST. BRCM will help with those PRs. We are adding MSTP mode along with existing STP functionalities. PVST & MSTP can coexist and used accordingly depending on usecases.
| Multiple Spanning Tree Protocol (MSTP) enhances the Spanning Tree Protocol (STP) by enabling the creation of multiple spanning tree instances within a network. It provides a mechanism to map VLANs to specific spanning tree instances which offers network segmentation and improved control over traffic flow. | ||
|
|
||
| # Introduction | ||
| Spanning Tree Protocol (STP) prevents bridge looping on LANs that include redundant links. Per VLAN Spanning Tree (PVST) is a modification of STP that allows for running separate instances of spanning tree for each VLAN. |
There was a problem hiding this comment.
Also which STP implementation, you are planning to use? i.e.) sonic stp repro https://github.com/sonic-net/sonic-stp introduced by Broadcom is kind of orphaned not being used and there is Linux natively MSTP support or planning custom MSTP implementation from your side?
There was a problem hiding this comment.
Currently, We are planning to use existing STP repo. But if you have any suggestions or specific use case in this regard we can have discussion on it as well.
|
@rck-innovium can you please help review this PR, thanks. |
@adyeung Hi Adam, Can you please help with these unmerged PRs of PVST, Thanks! |
|
@divyachandralekha from BRCM to review the HLD |
|
@divyachandralekha Hi Divya,can you please review this HLD! |
| The table holds the state of a port i.e forwarding, learning, blocking with respect to each instance. | ||
| #### STP_VLAN_INSTANCE_TABLE | ||
| The table holds the VLAN to instance mapping. | ||
| #### STP_FASTAGEING_FLUSH_TABLE |
There was a problem hiding this comment.
A per-VLAN flush may trigger a lot of churn, especially considering the possibility of 4K VLANs mapped to one MST instance. Here, only one flush is necessary. You may need to consider optimisation in this scenario.
There was a problem hiding this comment.
Yes, it might be cumber-sum to flush for each VLAN. We will consider one FDB Flush for every MSTI for optimization.
doc/stp/MSTP_HLD.md
Outdated
| # Sequence Diagrams | ||
| ## MSTP global enable | ||
|
|
||
| Only the VLANs that are currently present will be mapped to IST instance. A VLAN cannot be mapped to an instance if it has not been created yet. |
There was a problem hiding this comment.
When we have this limitation , managing the IST configuration becomes more complex. Especially in environments where VLANs are frequently added or removed. Administrators must continuously update the IST configuration to reflect changes in VLAN membership.
Creating a VLAN and then adding it to the Internal Spanning Tree (IST) configuration can potentially create convergence churn and disturb the existing network.
You may need to consider pre-configuring VLAN-to-instance mapping before actually creating the VLAN.
There was a problem hiding this comment.
Agreed, we will consider pre-configuring VLAN-to instance mapping to avoid convergence churn.
| Below commands allow configuring on region basis: | ||
|
|
||
| - **config spanning_tree region name \<region-name\>** | ||
| - Edit the name of region |
There was a problem hiding this comment.
Do you support empty region name ?
There was a problem hiding this comment.
Name is an empty string it could have 0 value by default for region name
doc/stp/MSTP_HLD.md
Outdated
| - **config spanning_tree instance (add|del) \<instance-id\>** | ||
| - Creation or deletion of an instance. | ||
| - instance-id: Default: 0, range: 1-63 | ||
| - Instance can not be deleted if VLAN(s) are associated with it. |
There was a problem hiding this comment.
What is the reason behind this restriction?
There was a problem hiding this comment.
The restriction here on an MSTP instance when VLANs are associated with it serves to prevent accidental deletions that could lead to network inconsistencies and loops in VLAN topologies. This approach aligns with industry standards followed by Cisco, Huawei, and SONiC, ensuring consistency and clarity for users familiar with these platforms. Additionally, in SONiC, an interface associated with a VLAN also prevents the deletion of that VLAN, maintaining the integrity of the network configuration.
|
Hi @divyachandralekha , do you have more suggestions on the design? if no can you please help approve this PR. Also we can discuss offline for PVST code PRs and our workflows for bringing STP/PVST/MSTP to SONiC |
|
@ridahanif96 The earlier PVST code PRs will be updated and reposted in the next community release |
@adyeung thanks Adam!, till then it would be great if we also finalize this design and merge this. |
|
@rck-innovium Hi Ravi, can you please help review this design. As per suggestion earlier in SAI community we had updated SAI Modification for MSTP control packets. |
Update CONFIG/APP DB and yang model
|
|
| 2. Assigning VLAN to instance via SAI STP API and SAI VLAN API. | ||
| 3. Creation of STP Port and assigning port state with respect to each instance via SAI STP API. | ||
| 4. Flushing FDB entries via SAI FDB API. | ||
|
|
There was a problem hiding this comment.
BPDU guard functionality requires link-state control, maybe we need to add SAI api that control link state here as well.
| - Configure an interface as a bpdu_guard. | ||
|
|
||
| Interface level CLI configurations of root guard, BPDU guard will also be supported for spanning-tree mode `mstp`. | ||
|
|
There was a problem hiding this comment.
I see STP can be enabled on "portchannel" as well, so im assuming we may need to add CLI restrictions to ensure that when STP is enabled on a portchannel, the portchannel group cannot be deleted/modified.
If thats true, please ensure its captured here or a different section of HLD
| Responsible for all MST protocol related calculations. BPDUs are sent and received in STPd and states are updated accordingly. | ||
|
|
||
| ### STPSync | ||
| A process running as a part of STPD. Responsible for updating all the MSTP states in APP DB. |
There was a problem hiding this comment.
Couple of questions here,
- is "stpSync" process forked within "STPd".
- Is "STPd" taken from a open-source implementation for instance https://github.com/sonic-net/sonic-stp ?
Assuming the answer to the above is 'yes', would be it cleaner to have "STPsync" implemented outside of "STPd" and then have IPCs to communicate port-states, fdb flush & link state events.
Doing this will ensure "STPd" repo has minimal integration changes and syncing it later to open-source repo would be easier.
This document describes the design details for IEEE 802.1s - Multiple Spanning Tree Protocol (MSTP) support in SONiC.