Skip to content

Commit

Permalink
Calulcate subport values and fix key error in hwsku.json (#18499)
Browse files Browse the repository at this point in the history
Automatically populate the correct subport value for an interface and insert into config_db.json

Why I did it
Problem 1
Breakouts of optic CMIS modules are broken. For example when in breakout mode of 4x100G on a port the /3, /5, /7 are not correctly associating with lanes (3,4), (5,6) and (7,8) of the transceiver respectively. The /3, /5, /7 are all trying to reprogram lanes (1,2).

In the xcvrd logs we see the following lane masks for all 4 interfaces for a port in 4x100G-2 mode:

EthernetX/X: Setting host_lanemask=0x3
EthernetX/X: Setting media_lanemask=0x1
As a result, we only see the /1 interface linking up correctly, the other 3 interfaces do not have the correct application advertisement + lanemasks applied. This is what we should expect to see:

lanes (1,2)
EthernetX/1: Setting host_lanemask=0x3
EthernetX/1: Setting media_lanemask=0x1

lanes (3,4)
EthernetX/3: Setting host_lanemask=0xc
EthernetX/3: Setting media_lanemask=0x2

lanes (5,6)
EthernetX/5: Setting host_lanemask=0x30
EthernetX/5: Setting media_lanemask=0x4

lanes (7,8)
EthernetX/7: Setting host_lanemask=0xc0
EthernetX/7: Setting media_lanemask=0x8
The problem is that “subport” is expected and used in xcvrd, xcvrd unit tests and explained in the Port Breakout HLD ; it was not implemented and does not end up in DB as expected, which defaults this value to 0.

(Pdb) self.port_dict["Ethernet194"]
{'index': 25, 'speed': '100000', 'lanes': '451,452', 'admin_status': 'up', 'cmis_state': 'INSERTED', 'cmis_retries': 0, 'cmis_expired': None}
Problem 2
This Pull Request does not play well with all HWSKUs that define breakouts. It does a lookup for every child port in the hwsku.json file - There should be 1 default breakout mode for each front panel port - this change expects every child port in a front panel port to have an entry - which causes an exception. Right now as far as I can tell this will break a lot of HWSKUs in a breakout config.

For example this is what the Dynamic Port Breakout feature uses on an Arista SKU to define 2x200G on the first front panel port:

"Ethernet0": {
     "default_brkout_mode": "2x200G[100G,50G,40G,25G,10G]"
},        
but it currently expects:

"Ethernet0": {
    ...
},    
"Ethernet4": {
    ...
},     
How I did it
Calculate subport value automatically based on the number of child ports used per front panel port and insert it into CONFIG_DB for the interface. If that number is 1 for the front panel, subport 0 is used. If has more than 1 I.e. is in breakout mode use a 1 based subport number as the feature expects. For example:

1x400G-8:

Ethernet0: subport 0
4x100G-2:

Ethernet0: subport 1
Ethernet2: subport 2
Ethernet4: subport 3
Ethernet6: subport 4
I have kept it compatible with Mellanox change - if you want to define a different subport number in your hwsku.json that is fine and it will override the dynamically generated default.

How to verify it
I verified this by checking the output of sonic-cfggen, updating config_db. and checking that both the host and media lane masks were correct for a 400GBASE-DR4 xcvr in both 2x200G-4 mode and 4x100G-2 mode.

All links came up and xcvrd prints the correct lanemasks in logs.

I also made sure that portconfig.py no longer causes an exception; it only checks for the optional HWSKU value if childports are present in hwsku.json file (this is to make sure we don't break Mellanox hwsku's)

I also changed the unit test for config_db generation and ensured the correct values match. The test passes.
  • Loading branch information
bobbymcgonigle committed May 17, 2024
1 parent 53966e3 commit 2c69277
Show file tree
Hide file tree
Showing 5 changed files with 179 additions and 51 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,217 +4,249 @@
"alias": "fortyGigE0/2",
"lanes": "27,28",
"speed": "50000",
"index": "0"
"index": "0",
"subport": "2"
},
"Ethernet0": {
"alias": "fortyGigE0/0",
"lanes": "25,26",
"speed": "50000",
"index": "0"
"index": "0",
"subport": "1"
}
},
"Ethernet12_1x50G_2x25G": {
"Ethernet12": {
"alias": "fortyGigE0/12",
"lanes": "37,38",
"speed": "50000",
"index": "3"
"index": "3",
"subport": "1"
},
"Ethernet14": {
"alias": "fortyGigE0/14",
"lanes": "39",
"speed": "25000",
"index": "3"
"index": "3",
"subport": "2"
},
"Ethernet15": {
"alias": "fortyGigE0/15",
"lanes": "40",
"speed": "25000",
"index": "3"
"index": "3",
"subport": "3"
}
},
"Ethernet0_2x50G": {
"Ethernet2": {
"alias": "fortyGigE0/2",
"lanes": "27,28",
"speed": "50000",
"index": "0"
"index": "0",
"subport": "2"
},
"Ethernet0": {
"alias": "fortyGigE0/0",
"lanes": "25,26",
"speed": "50000",
"index": "0"
"index": "0",
"subport": "1"
}
},
"Ethernet0_1x100G": {
"Ethernet0": {
"alias": "fortyGigE0/0",
"lanes": "25,26,27,28",
"speed": "100000",
"index": "0"
"index": "0",
"subport": "1"
}
},
"Ethernet0_4x25G": {
"Ethernet2": {
"alias": "fortyGigE0/2",
"lanes": "27",
"speed": "25000",
"index": "0"
"index": "0",
"subport": "3"
},
"Ethernet3": {
"alias": "fortyGigE0/3",
"lanes": "28",
"speed": "25000",
"index": "0"
"index": "0",
"subport": "4"
},
"Ethernet0": {
"alias": "fortyGigE0/0",
"lanes": "25",
"speed": "25000",
"index": "0"
"index": "0",
"subport": "1"
},
"Ethernet1": {
"alias": "fortyGigE0/1",
"lanes": "26",
"speed": "25000",
"index": "0"
"index": "0",
"subport": "2"
}
},
"Ethernet0_2x25G_1x50G": {
"Ethernet2": {
"alias": "fortyGigE0/2",
"lanes": "27,28",
"speed": "50000",
"index": "0"
"index": "0",
"subport": "3"
},
"Ethernet0": {
"alias": "fortyGigE0/0",
"lanes": "25",
"speed": "25000",
"index": "0"
"index": "0",
"subport": "1"
},
"Ethernet1": {
"alias": "fortyGigE0/1",
"lanes": "26",
"speed": "25000",
"index": "0"
"index": "0",
"subport": "2"
}
},
"Ethernet0_1x50G_2x25G": {
"Ethernet2": {
"alias": "fortyGigE0/2",
"lanes": "27",
"speed": "25000",
"index": "0"
"index": "0",
"subport": "2"
},
"Ethernet3": {
"alias": "fortyGigE0/3",
"lanes": "28",
"speed": "25000",
"index": "0"
"index": "0",
"subport": "3"
},
"Ethernet0": {
"alias": "fortyGigE0/0",
"lanes": "25,26",
"speed": "50000",
"index": "0"
"index": "0",
"subport": "1"
}
},
"Ethernet4_4x25G": {
"Ethernet6": {
"alias": "fortyGigE0/6",
"lanes": "31",
"speed": "25000",
"index": "1"
"index": "1",
"subport": "3"
},
"Ethernet7": {
"alias": "fortyGigE0/7",
"lanes": "32",
"speed": "25000",
"index": "1"
"index": "1",
"subport": "4"
},
"Ethernet4": {
"alias": "fortyGigE0/4",
"lanes": "29",
"speed": "25000",
"index": "1"
"index": "1",
"subport": "1"
},
"Ethernet5": {
"alias": "fortyGigE0/5",
"lanes": "30",
"speed": "25000",
"index": "1"
"index": "1",
"subport": "2"
}
},
"Ethernet4_2x50G": {
"Ethernet6": {
"alias": "fortyGigE0/6",
"lanes": "31,32",
"speed": "50000",
"index": "1"
"index": "1",
"subport": "2"
},
"Ethernet4": {
"alias": "fortyGigE0/4",
"lanes": "29,30",
"speed": "50000",
"index": "1"
"index": "1",
"subport": "1"
}
},
"Ethernet8_2x50G": {
"Ethernet8": {
"alias": "fortyGigE0/8",
"lanes": "33,34",
"speed": "50000",
"index": "2"
"index": "2",
"subport": "1"
},
"Ethernet10": {
"alias": "fortyGigE0/10",
"lanes": "35,36",
"speed": "50000",
"index": "2"
"index": "2",
"subport": "2"
}
},
"Ethernet8_1x50G_2x25G": {
"Ethernet10": {
"alias": "fortyGigE0/10",
"lanes": "35",
"speed": "25000",
"index": "2"
"index": "2",
"subport": "1"
},
"Ethernet11": {
"alias": "fortyGigE0/11",
"lanes": "36",
"speed": "25000",
"index": "2"
"index": "2",
"subport": "2"
}
},
"Ethernet8_2x25G_1x50G": {
"Ethernet8": {
"alias": "fortyGigE0/8",
"lanes": "33",
"speed": "25000",
"index": "2"
"index": "2",
"subport": "1"
},
"Ethernet9": {
"alias": "fortyGigE0/9",
"lanes": "34",
"speed": "25000",
"index": "2"
"index": "2",
"subport": "2"
},
"Ethernet10": {
"alias": "fortyGigE0/10",
"lanes": "35,36",
"speed": "50000",
"index": "2"
"index": "2",
"subport": "3"
}
},
"Ethernet8_1x100G": {
"Ethernet8": {
"alias": "fortyGigE0/8",
"lanes": "33,34,35,36",
"speed": "100000",
"index": "2"
"index": "2",
"subport": "0"
}
}
}
8 changes: 4 additions & 4 deletions platform/vs/tests/breakout/test_breakout_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,22 +74,22 @@ def test_breakout_modes(self, dvs):
print("**** Breakout Cli test Starts ****")
output_dict = self.breakout(dvs, 'Ethernet0', '2x50G')
expected_dict = expected["Ethernet0_2x50G"]
assert output_dict == expected_dict
assert output_dict == expected_dict, "output: {} != expected: {}".format(output_dict, expected_dict)
print("**** 1X100G --> 2x50G passed ****")

output_dict = self.breakout(dvs, 'Ethernet4', '4x25G[10G]')
expected_dict = expected["Ethernet4_4x25G"]
assert output_dict == expected_dict
assert output_dict == expected_dict, "output: {} != expected: {}".format(output_dict, expected_dict)
print("**** 1X100G --> 4x25G[10G] passed ****")

output_dict = self.breakout(dvs, 'Ethernet8', '2x25G(2)+1x50G(2)')
expected_dict = expected["Ethernet8_2x25G_1x50G"]
assert output_dict == expected_dict
assert output_dict == expected_dict, "output: {} != expected: {}".format(output_dict, expected_dict)
print("**** 1X100G --> 2x25G(2)+1x50G(2) passed ****")

output_dict = self.breakout(dvs, 'Ethernet12', '1x50G(2)+2x25G(2)')
expected_dict = expected["Ethernet12_1x50G_2x25G"]
assert output_dict == expected_dict
assert output_dict == expected_dict, "output: {} != expected: {}".format(output_dict, expected_dict)
print("**** 1X100G --> 1x50G(2)+2x25G(2) passed ****")

# TODOFIX: remove comments once #4442 PR got merged and
Expand Down
15 changes: 10 additions & 5 deletions src/sonic-config-engine/portconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
BRKOUT_MODE = "default_brkout_mode"
CUR_BRKOUT_MODE = "brkout_mode"
INTF_KEY = "interfaces"
OPTIONAL_HWSKU_ATTRIBUTES = ["fec", "autoneg", "subport", "role"]
OPTIONAL_HWSKU_ATTRIBUTES = ["fec", "autoneg", "role"]

BRKOUT_PATTERN = r'(\d{1,6})x(\d{1,6}G?)(\[(\d{1,6}G?,?)*\])?(\((\d{1,6})\))?'
BRKOUT_PATTERN_GROUPS = 6
Expand Down Expand Up @@ -353,8 +353,10 @@ def _str_to_entries(self, bmode):
def get_config(self):
# Ensure that we have corret number of configured lanes
lanes_used = 0
total_num_ports = 0
for entry in self._breakout_mode_entry:
lanes_used += entry.num_assigned_lanes
total_num_ports += entry.num_ports

if lanes_used > len(self._lanes):
raise RuntimeError("Assigned lines count is more that available!")
Expand All @@ -376,7 +378,8 @@ def get_config(self):
'alias': self._breakout_capabilities[alias_id],
'lanes': ','.join(lanes),
'speed': str(entry.default_speed),
'index': self._indexes[lane_id]
'index': self._indexes[lane_id],
'subport': "0" if total_num_ports == 1 else str(alias_id + 1)
}

lane_id += lanes_per_port
Expand Down Expand Up @@ -422,10 +425,12 @@ def parse_platform_json_file(hwsku_json_file, platform_json_file):
child_ports = get_child_ports(intf, brkout_mode, platform_json_file)

# take optional fields from hwsku.json
hwsku_entry = hwsku_dict[INTF_KEY]
for child_port in child_ports:
for key, item in hwsku_dict[INTF_KEY][child_port].items():
if key in OPTIONAL_HWSKU_ATTRIBUTES:
child_ports.get(child_port)[key] = item
if child_port in hwsku_entry:
for key, item in hwsku_entry[child_port].items():
if key in OPTIONAL_HWSKU_ATTRIBUTES:
child_ports.get(child_port)[key] = item

ports.update(child_ports)

Expand Down
Loading

0 comments on commit 2c69277

Please sign in to comment.