-
Notifications
You must be signed in to change notification settings - Fork 759
Fixing MSTP global command enable issue #4127
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.
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
522aa7f
af11fa0
da56043
32216c1
1b34555
066f77b
6338ae2
64f75a2
7b39a3f
e0f671c
5348f0e
da7c4b8
5c2745a
92608c8
449b11f
235ebff
44d54b7
c26b693
3e5f50b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,19 +167,26 @@ def spanning_tree(ctx): | |
| global g_stp_mode | ||
| if 'pvst' == global_cfg['mode']: | ||
| g_stp_mode = 'PVST' | ||
| elif 'mst' == global_cfg['mode']: | ||
| g_stp_mode = 'MSTP' | ||
|
|
||
| if ctx.invoked_subcommand is None: | ||
| keys = g_stp_appl_db.keys(g_stp_appl_db.APPL_DB, "*STP_VLAN_TABLE:Vlan*") | ||
| if not keys: | ||
| return | ||
| vlan_list = [] | ||
| for key in keys: | ||
| result = re.search('.STP_VLAN_TABLE:Vlan(.*)', key) | ||
| vlanid = result.group(1) | ||
| vlan_list.append(int(vlanid)) | ||
| vlan_list.sort() | ||
| for vlanid in vlan_list: | ||
| ctx.invoke(show_stp_vlan, vlanid=vlanid) | ||
| # For MSTP mode, use new format | ||
| if g_stp_mode == 'MSTP': | ||
| ctx.invoke(show_mstp_summary) | ||
| else: | ||
| # For PVST mode, use existing format | ||
| keys = g_stp_appl_db.keys(g_stp_appl_db.APPL_DB, "*STP_VLAN_TABLE:Vlan*") | ||
| if not keys: | ||
| return | ||
| vlan_list = [] | ||
| for key in keys: | ||
| result = re.search('.STP_VLAN_TABLE:Vlan(.*)', key) | ||
| vlanid = result.group(1) | ||
| vlan_list.append(int(vlanid)) | ||
| vlan_list.sort() | ||
| for vlanid in vlan_list: | ||
| ctx.invoke(show_stp_vlan, vlanid=vlanid) | ||
|
|
||
|
|
||
| @spanning_tree.group('vlan', cls=clicommon.AliasedGroup, invoke_without_command=True) | ||
|
|
@@ -401,3 +408,321 @@ def show_stp_vlan_statistics(ctx, vlanid): | |
|
|
||
| click.echo("{:17}{:15}{:15}{:15}{}".format( | ||
| ifname, entry['bpdu_sent'], entry['bpdu_received'], entry['tc_sent'], entry['tc_received'])) | ||
|
|
||
|
|
||
| def format_bridge_id(bridge_id_str): | ||
| """Format bridge ID from hex string to the display format""" | ||
| # Input is like "8000.80a2.3526.0c5e" or similar | ||
| # Just return as-is if already formatted, otherwise format it | ||
| if not bridge_id_str or bridge_id_str == 'NA': | ||
| return 'NA' | ||
| return bridge_id_str | ||
|
|
||
|
|
||
| def format_vlan_list(vlan_list_str): | ||
| """Convert vlan mask string to readable vlan ranges""" | ||
| if not vlan_list_str: | ||
| return "" | ||
|
|
||
| """Convert comma VLAN list to merged ranges. Example: | ||
| '1,2,4,5,6,10,11,12,20' b'1-2,4-6,10-12,20' | ||
| """ | ||
|
|
||
| # Fast conversion & sorting | ||
| vlans = sorted({int(v) for v in vlan_list_str.split(",") if v.strip()}) | ||
|
|
||
| if not vlans: | ||
| return "" | ||
|
|
||
| ranges = [] | ||
| start = prev = vlans[0] | ||
|
|
||
| for v in vlans[1:]: | ||
| if v == prev + 1: | ||
| prev = v | ||
| continue | ||
|
|
||
| # close range | ||
| ranges.append(f"{start}-{prev}" if start != prev else str(start)) | ||
| start = prev = v | ||
|
|
||
| # close last range | ||
| ranges.append(f"{start}-{prev}" if start != prev else str(start)) | ||
|
|
||
| return ",".join(ranges) | ||
|
|
||
|
|
||
| def get_mst_global_info(): | ||
| """Get MST global configuration information""" | ||
| mst_global = g_stp_cfg_db.get_entry("STP_MST", "GLOBAL") | ||
| if not mst_global: | ||
| mst_global = {} | ||
|
|
||
| # Set defaults if not present | ||
| if 'name' not in mst_global: | ||
| mst_global['name'] = '' | ||
| if 'revision' not in mst_global: | ||
| mst_global['revision'] = '0' | ||
| if 'max_hops' not in mst_global: | ||
| mst_global['max_hops'] = '20' | ||
| if 'max_age' not in mst_global: | ||
| mst_global['max_age'] = '20' | ||
| if 'hello_time' not in mst_global: | ||
| mst_global['hello_time'] = '2' | ||
| if 'forward_delay' not in mst_global: | ||
| mst_global['forward_delay'] = '15' | ||
| if 'hold_count' not in mst_global: | ||
| mst_global['hold_count'] = '6' | ||
|
|
||
| return mst_global | ||
|
|
||
|
|
||
| def get_mst_instance_entry(mst_id): | ||
| """Get MST instance entry from application database""" | ||
| entry = stp_get_all_from_pattern(g_stp_appl_db, g_stp_appl_db.APPL_DB, | ||
| "*STP_MST_INST_TABLE:{}".format(mst_id)) | ||
| if not entry: | ||
| return None | ||
|
|
||
| # Set defaults for missing fields | ||
| if 'bridge_address' not in entry: | ||
| entry['bridge_address'] = 'NA' | ||
| if 'root_address' not in entry: | ||
| entry['root_address'] = 'NA' | ||
| if 'regional_root_address' not in entry: | ||
| entry['regional_root_address'] = 'NA' | ||
| if 'root_path_cost' not in entry: | ||
| entry['root_path_cost'] = '0' | ||
| if 'regional_root_cost' not in entry: | ||
| entry['regional_root_cost'] = '0' | ||
| if 'root_port' not in entry: | ||
| entry['root_port'] = '' | ||
| if 'remaining_hops' not in entry: | ||
| entry['remaining_hops'] = '20' | ||
| if 'root_hello_time' not in entry: | ||
| entry['root_hello_time'] = '2' | ||
| if 'root_forward_delay' not in entry: | ||
| entry['root_forward_delay'] = '15' | ||
| if 'root_max_age' not in entry: | ||
| entry['root_max_age'] = '20' | ||
| if 'hold_time' not in entry: | ||
| entry['hold_time'] = '6' | ||
| if 'vlan@' not in entry: | ||
| entry['vlan@'] = '' | ||
| if 'bridge_priority' not in entry: | ||
| entry['bridge_priority'] = '32768' | ||
|
|
||
| return entry | ||
|
|
||
|
|
||
| def get_mst_port_entry(mst_id, ifname): | ||
| """Get MST port entry from application database""" | ||
| entry = stp_get_all_from_pattern(g_stp_appl_db, g_stp_appl_db.APPL_DB, | ||
| "*STP_MST_PORT_TABLE:{}:{}".format(mst_id, ifname)) | ||
| if not entry: | ||
| return None | ||
|
|
||
| # Set defaults for missing fields | ||
| if 'port_number' not in entry: | ||
| entry['port_number'] = 'NA' | ||
| if 'priority' not in entry: | ||
| entry['priority'] = '128' | ||
| if 'path_cost' not in entry: | ||
| entry['path_cost'] = '0' | ||
| if 'port_state' not in entry: | ||
| entry['port_state'] = 'NA' | ||
| if 'role' not in entry: | ||
| entry['role'] = 'NA' | ||
|
|
||
| return entry | ||
|
|
||
|
|
||
| @spanning_tree.command('mstp') | ||
| @click.pass_context | ||
| def show_mstp_summary(ctx): | ||
| """Show MSTP spanning tree information in detailed format""" | ||
|
|
||
| # Print mode | ||
| click.echo("Spanning-tree Mode: MSTP") | ||
|
|
||
| # Get all MST instances | ||
| keys = g_stp_appl_db.keys(g_stp_appl_db.APPL_DB, "*STP_MST_INST_TABLE:*") | ||
| if not keys: | ||
| return | ||
|
|
||
| mst_list = [] | ||
| for key in keys: | ||
| result = re.search(r'STP_MST_INST_TABLE:(\d+)', key) | ||
| if result: | ||
| mst_id = int(result.group(1)) | ||
| mst_list.append(mst_id) | ||
|
|
||
| mst_list.sort() | ||
|
|
||
| # Display each MST instance | ||
| for mst_id in mst_list: | ||
| show_mst_instance_detail(mst_id) | ||
|
|
||
| # Display MST region information at the end | ||
| show_mst_region_info() | ||
|
|
||
|
|
||
| def show_mst_instance_detail(mst_id): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I compare the implemented show output to the show information in HLD, I see that the information shown here is for the summary. i..e, the interface information is shown in single row for each interface. Should we rename this function to "show_mst_instance_summary()" to make the name approriately mean what is done in the function?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will take care in next PR. |
||
| """Display detailed information for a specific MST instance""" | ||
|
|
||
| mst_entry = get_mst_instance_entry(mst_id) | ||
| if not mst_entry: | ||
| return | ||
|
|
||
| mst_global = get_mst_global_info() | ||
|
|
||
| # Print instance header | ||
| vlan_str = mst_entry.get('vlan@', '') | ||
| vlan_list = format_vlan_list(vlan_str) | ||
| click.echo("") | ||
| click.echo("####### MST{:<8} Vlans mapped : {}".format(mst_id, vlan_list)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the instance is 0 we can add the word (CIST) as propsed in the HLD
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will take care in next PR. |
||
|
|
||
| # Bridge information | ||
| bridge_addr = format_bridge_id(mst_entry['bridge_address']) | ||
| bridge_priority = mst_entry.get('bridge_priority', '32768') | ||
| click.echo("Bridge Address {}".format(bridge_addr)) | ||
|
|
||
| # Root information | ||
| root_addr = format_bridge_id(mst_entry['root_address']) | ||
| click.echo("Root Address {}".format(root_addr)) | ||
|
|
||
| root_port = mst_entry.get('root_port', '') | ||
| root_path_cost = mst_entry.get('root_path_cost', '0') | ||
|
|
||
| if root_port: | ||
| click.echo(" Port {} Path cost {}".format( | ||
| root_port, root_path_cost)) | ||
| else: | ||
| click.echo(" Port Root Path cost {}".format( | ||
| root_path_cost)) | ||
|
|
||
| # Regional Root (for CIST) | ||
| if mst_id == 0: | ||
| reg_root_addr = format_bridge_id(mst_entry['regional_root_address']) | ||
| click.echo("Regional Root Address {}".format(reg_root_addr)) | ||
|
|
||
| reg_root_cost = mst_entry.get('regional_root_cost', '0') | ||
| rem_hops = mst_entry.get('remaining_hops', '20') | ||
| click.echo(" Internal cost {} Rem hops {}".format( | ||
| reg_root_cost, rem_hops)) | ||
|
|
||
| # Operational parameters | ||
| hello_time = mst_entry.get('root_hello_time', mst_global['hello_time']) | ||
| fwd_delay = mst_entry.get('root_forward_delay', mst_global['forward_delay']) | ||
| max_age = mst_entry.get('root_max_age', mst_global['max_age']) | ||
| hold_count = mst_entry.get('hold_time', mst_global['hold_count']) | ||
|
|
||
| click.echo("Operational Hello Time {}, Forward Delay {}, Max Age {}, Txholdcount {}".format( | ||
| hello_time, fwd_delay, max_age, hold_count)) | ||
|
|
||
| # Configured parameters | ||
| click.echo("Configured Hello Time {}, Forward Delay {}, Max Age {}, Max Hops {}".format( | ||
| mst_global['hello_time'], mst_global['forward_delay'], | ||
| mst_global['max_age'], mst_global['max_hops'])) | ||
| else: | ||
| # For non-CIST instances | ||
| rem_hops = mst_entry.get('remaining_hops', '20') | ||
| click.echo(" Port Root Path cost {} Rem Hops {}".format( | ||
| root_path_cost, rem_hops)) | ||
|
|
||
| click.echo("") | ||
|
|
||
| # Port information table header | ||
| click.echo("Interface Role State Cost Prio.Nbr Type") | ||
| click.echo("--------------- -------- ---------- ------- --------- -----------") | ||
|
|
||
| # Get all ports for this instance | ||
| port_keys = g_stp_appl_db.keys(g_stp_appl_db.APPL_DB, | ||
| "*STP_MST_PORT_TABLE:{}:*".format(mst_id)) | ||
| if port_keys: | ||
| intf_list = [] | ||
| for key in port_keys: | ||
| result = re.search(r'STP_MST_PORT_TABLE:\d+:(.*)', key) | ||
| if result: | ||
| ifname = result.group(1) | ||
| intf_list.append(ifname) | ||
|
|
||
| # Sort interfaces: Ethernet first, then PortChannel | ||
| eth_list = [ifname for ifname in intf_list if ifname.startswith("Ethernet")] | ||
| po_list = [ifname for ifname in intf_list if ifname.startswith("PortChannel")] | ||
|
|
||
| # Sort by numeric part | ||
| eth_list.sort(key=lambda x: int(re.search(r'\d+', x).group())) | ||
| po_list.sort(key=lambda x: int(re.search(r'\d+', x).group())) | ||
|
|
||
| for ifname in eth_list + po_list: | ||
| show_mst_port_info(mst_id, ifname) | ||
|
|
||
|
|
||
| def show_mst_port_info(mst_id, ifname): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may need another function like "show_mst_port_info_detail()" which can be used for showing MST instance detail where the each port info is displayed in deatil (as per "show spanning-tree mst detail" info in the HLD)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will take care in next PR. |
||
| """Display port information for MST instance""" | ||
|
|
||
| port_entry = get_mst_port_entry(mst_id, ifname) | ||
| if not port_entry: | ||
| return | ||
|
|
||
| role = port_entry.get('role', 'UNKNOWN').upper() | ||
| state = port_entry.get('port_state', 'UNKNOWN').upper() | ||
| cost = port_entry.get('path_cost', '0') | ||
| priority = port_entry.get('priority', '128') | ||
| port_num = port_entry.get('port_number', '0') | ||
|
|
||
| # Determine link type (typically P2P for point-to-point) | ||
| link_type = 'P2P' | ||
|
|
||
| # Format priority.port number | ||
| prio_nbr = "{}.{}".format(priority, port_num) | ||
|
|
||
| click.echo("{:<19}{:<13}{:<16}{:<11}{:<12}{}".format( | ||
| ifname, role, state, cost, prio_nbr, link_type)) | ||
|
|
||
|
|
||
| def show_mst_region_info(): | ||
| """Display MST region configuration information""" | ||
|
|
||
| mst_global = get_mst_global_info() | ||
|
|
||
| # Get CIST information for some global stats | ||
| cist_entry = get_mst_instance_entry(0) | ||
|
|
||
| click.echo("") | ||
| click.echo("Region Name : {}".format(mst_global['name'])) | ||
| click.echo("Revision : {}".format(mst_global['revision'])) | ||
|
|
||
| if cist_entry: | ||
| bridge_id = cist_entry.get('bridge_address', 'NA') | ||
| root_id = cist_entry.get('root_address', 'NA') | ||
| ext_cost = cist_entry.get('root_path_cost', '0') | ||
|
|
||
| # Remove dots and format as continuous hex | ||
| cist_bridge_id = bridge_id.replace('.', '') if bridge_id != 'NA' else '0' | ||
| cist_root_id = root_id.replace('.', '') if root_id != 'NA' else '0' | ||
|
|
||
| click.echo("CIST Bridge Identifier : {}".format(cist_bridge_id)) | ||
| click.echo("CIST Root Identifier : {}".format(cist_root_id)) | ||
| click.echo("CIST External Path Cost : {}".format(ext_cost)) | ||
|
|
||
| # Count configured instances (excluding CIST) | ||
| keys = g_stp_appl_db.keys(g_stp_appl_db.APPL_DB, "*STP_MST_INST_TABLE:*") | ||
| instance_count = len([k for k in keys if not k.endswith(':0')]) if keys else 0 | ||
|
|
||
| click.echo("Instances configured : {}".format(instance_count)) | ||
|
|
||
| # Topology change information | ||
| click.echo("Last Topology Change : 0s") | ||
| click.echo("Number of Topology Changes : 0") | ||
|
|
||
| # Bridge timers | ||
| click.echo("Bridge Timers : MaxAge {}s Hello {}s FwdDly {}s MaxHops {}".format( | ||
| mst_global['max_age'], mst_global['hello_time'], | ||
| mst_global['forward_delay'], mst_global['max_hops'])) | ||
|
|
||
| # CIST Root timers (typically same as bridge timers) | ||
| click.echo("CIST Root Timers : MaxAge {}s Hello {}s FwdDly {}s MaxHops {}".format( | ||
| mst_global['max_age'], mst_global['hello_time'], | ||
| mst_global['forward_delay'], mst_global['max_hops'])) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recommendation: It is better to implement the "show spanning-tree statistics [mst ]" as part of this PR. We already have some statistics (such as bpd rx, bpdu tx and num fwd transitions) in the STP_MST_PORT_TABLE entries. We can also implement "show spanning-tree mst interface " as part of the PR to meet all the show commands specified in HLD
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems the MSTP show commands are missed during implementation. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion for alternate approach:
If we can do this similar to how it is done for PVST we can show spanning tree for all MST instances and also for a specific instance (for "show spanning-tree mst instance command) using the same function show_mst_instnace_detail().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take care in next PR.