[sonic-config-engine]: Improve comparison between default and supported breakout modes. by oleksandrivantsiv · Pull Request #9278 · sonic-net/sonic-buildimage

oleksandrivantsiv · 2021-11-16T20:34:20Z

Why I did it

The previous implementation of sonic-cfggen did a simple comparison between default breakout mode in
hwsku.json and supported modes in platform.json. To set a different default speed in hwsku.json
it was required to add one more entry to supported modes in platfrom.json file:

1x10G[100G,50G] vs 1x100G[50G,10G]

The new implementation does more intelligent parsing and analysis of supported and default modes. It
allows changing default speed without adding a new entry to platform.json.

How I did it

Add more intelligent parsing and analysis of supported and default modes.

How to verify it

Run sonic-config-engine unit tests from sonic-config-engine/tests directory:

sonic-config-engine/tests$ pytest ./test_cfggen_platformJson.py -vvs

test_cfggen_platformJson.py::TestCfgGenPlatformJson::test_dummy_run PASSED                                                                                                    [ 16%]
test_cfggen_platformJson.py::TestCfgGenPlatformJson::test_platform_json_all_ethernet_interfaces PASSED                                                                        [ 33%]
test_cfggen_platformJson.py::TestCfgGenPlatformJson::test_platform_json_interfaces_keys PASSED                                                                                [ 50%]
test_cfggen_platformJson.py::TestCfgGenPlatformJson::test_platform_json_no_interfaces PASSED                                                                                  [ 66%]
test_cfggen_platformJson.py::TestCfgGenPlatformJson::test_platform_json_specific_ethernet_interfaces PASSED                                                                   [ 83%]
test_cfggen_platformJson.py::TestCfgGenPlatformJson::test_print_data PASSED                                                                                                   [100%]

============================================================================= 6 passed in 2.46 seconds ==============================================================================

Unit tests are extended to cover changes in breakout mode parsing added in this PR

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

…ed breakout modes. The previous implementation of sonic-cfggen did a simple comparison between default breakout mode in hwsku.json and supported modes in platform.json. To set a different default speed in hwsku.json it was required to add one more entry to supported modes in platfrom.json file: 1x10G[100G,50G] vs 1x100G[50G,10G] The new implementation does more intelligent parsing and analysis of supported and default modes. It allows changing default speed without adding a new entry to platform.json.

dgsudharsan · 2021-11-16T20:57:40Z

@praveen-li @zhenggen-xu Can you please review?

zhenggen-xu · 2021-11-17T05:57:24Z

@samaity Please review.

oleksandrivantsiv · 2021-11-19T09:50:49Z

@samaity Can you please review this?

samaity · 2021-11-19T18:02:49Z

@samaity Can you please review this?

checking now!

liat-grozovik · 2021-11-24T09:08:42Z

/azp run

azure-pipelines · 2021-11-24T09:08:47Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

liat-grozovik · 2021-11-24T09:09:04Z

@samaity kindly reminder. can you please provide your inputs?

liat-grozovik · 2021-11-24T09:19:34Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2021-11-24T09:19:53Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2021-12-08T17:18:05Z

@oleksandrivantsiv could you please check the lgtm issue so we can now mege now that all approved the change?

lgtm-com · 2021-12-09T13:08:18Z

This pull request introduces 2 alerts when merging 04be47c into fc91dae - view on LGTM.com

new alerts:

1 for Inconsistent equality and inequality
1 for Inconsistent equality and hashing

liat-grozovik · 2021-12-14T14:30:18Z

@samaity kindly reminder, can you please review ?

samaity · 2021-12-15T01:19:43Z

src/sonic-config-engine/portconfig.py

-            alias_position += 1
-            lanes_start += step
+        def __hash__(self):
+            return hash((self.num_ports, tuple(self.supported_speed), self.num_assigned_lanes))


what exactly are we checking here?

BreakoutModeEntry objects are used to compare breakout modes based on their capabilities with __eq__ and __ne__ operators.
In Python when the class implements __eq__ operator it is recommended to implement __hash__ operator as well. __hash__ operator returns integer hash id of the object that can be used to put an object in a dictionary (as a key) or set. Here hash id is calculated based on capabilities that define breakout mode.

My initial implementation didn't include this method implementation. I added to fix LGTM alerts:

This pull request introduces 2 alerts when merging 04be47c into fc91dae - view on LGTM.com new alerts: 1 for Inconsistent equality and inequality 1 for Inconsistent equality and hashing

Thanks for the clarification, @oleksandrivantsiv.

samaity · 2021-12-15T01:22:24Z

src/sonic-config-engine/portconfig.py

+        for supported_mode in self._properties['breakout_modes']:
+            if self._breakout_mode_entry == self._str_to_entries(supported_mode):
+                self._breakout_capabilities = self._properties['breakout_modes'][supported_mode]
+                break


why did you use break here?
what will be case when self._breakout_mode_entry != self._str_to_entries(supported_mode)? Should we address such cases?

Here we are trying to find whether the given breakout mode is supported. We are iterating over supported modes and checking if the given breakout mode is equal to one of them. If the breakout mode is not equal to any of the supported modes we will raise an exception with an appropriated message at line 250.

samaity · 2021-12-15T01:27:19Z

src/sonic-config-engine/portconfig.py

+            raise RuntimeError("Unsupported breakout mode {}!".format(bmode))
+
+    def _re_group_to_entry(self, group):
+        if len(group) != 6:


can we make 6 as global var in the beginning of this file and use it here and also in BRKOUT_PATTERN?

I added a new BRKOUT_PATTERN_GROUPS=6 global variable to replace the magic number used here.
However, I didn't change the value used in BRKOUT_PATTERN because it has a different meaning.
Here be indicating how many groups should have as a result of regexp matching.
In BRKOUT_PATTERN pattern 6 indicates up to how many digits can be in each group.
It's just a coincidence that these values are the same :)

Aah, makes sense! 👍

samaity · 2021-12-15T01:28:08Z

src/sonic-config-engine/portconfig.py

+            2x50G ---------------> [('2', '50G', None, None, None)]
+        """
+
+        groups_list = [re.match(BRKOUT_PATTERN, i).groups() for i in bmode.split("+")]


if regex does not match, should we do try-catch in that case?

To raise an exception with the meaningful error message I added try-except as suggested. Thanks!

samaity · 2021-12-15T01:32:52Z

@oleksandrivantsiv reviewed the code and asked few doubts.

Overall, looks good to me. Kudos to the intelligence behind the smooth parsing and analysis of supported and default modes and covering more corner cases 👍

btw a quick doubt, did you build the whole image with this PR? As I remember, there are few build test cases in sonic-utilities submodule which depend on the function you are deprecating via this PR?

oleksandrivantsiv · 2021-12-15T19:17:35Z

@samaity thanks a lot for your feedback!

Before raising a PR I build the whole images and run a CI. The changes listed here are fully backward compatible with what was implemented before. All configuration files written before should work without any changes

samaity

LGTM! 👍

qiluo-msft · 2021-12-16T21:58:15Z

src/sonic-config-engine/portconfig.py

 OPTIONAL_HWSKU_ATTRIBUTES = ["fec", "autoneg"]

-BRKOUT_PATTERN = r'(\d{1,3})x(\d{1,3}G)(\[(\d{1,3}G,?)*\])?(\((\d{1,3})\))?'
+BRKOUT_PATTERN = r'(\d{1,6})x(\d{1,6}G?)(\[(\d{1,6}G?,?)*\])?(\((\d{1,6})\))?'


G?

Why allow missing G? What is the default unit? Is there a document for the default unit? #Closed

To allow to configure speed in megabytes:

"1x100000[50G,40000,25000,10G]": ["Eth36"], "2x50G": ["Eth36/1", "Eth36/2"], "4x25G[10000]": ["Eth36/1", "Eth36/2", "Eth36/3", "Eth36/4"], "2x25000(2)+1x50G(2)": ["Eth36/1", "Eth36/2", "Eth36/3"], "1x50000(2)+2x25G(2)": ["Eth36/1", "Eth36/2", "Eth36/3"]

liat-grozovik · 2021-12-20T14:34:38Z

@qiluo-msft any further comment? can we move forward and merge?

…ed breakout modes. (#9278) Closes #7958 #### Why I did it The previous implementation of sonic-cfggen did a simple comparison between default breakout mode in hwsku.json and supported modes in platform.json. To set a different default speed in hwsku.json it was required to add one more entry to supported modes in platfrom.json file: 1x10G[100G,50G] vs 1x100G[50G,10G] The new implementation does more intelligent parsing and analysis of supported and default modes. It allows changing default speed without adding a new entry to platform.json. #### How I did it Add more intelligent parsing and analysis of supported and default modes. #### How to verify it Run sonic-config-engine unit tests from sonic-config-engine/tests directory

oleksandrivantsiv requested a review from lguohan as a code owner November 16, 2021 20:34

dgsudharsan requested a review from zhenggen-xu November 16, 2021 20:58

dgsudharsan previously approved these changes Nov 16, 2021

View reviewed changes

liat-grozovik removed the request for review from zhenggen-xu November 24, 2021 09:09

Merge remote-tracking branch 'origin/master'

04be47c

Fix LGTM allerts.

7c44f2c

oleksandrivantsiv dismissed dgsudharsan’s stale review via 7c44f2c December 9, 2021 15:15

samaity reviewed Dec 15, 2021

View reviewed changes

Fix review comments.

e745da6

samaity approved these changes Dec 15, 2021

View reviewed changes

dgsudharsan added the Request for 202111 Branch For PRs being requested for 202111 branch label Dec 15, 2021

qiluo-msft reviewed Dec 16, 2021

View reviewed changes

qiluo-msft approved these changes Dec 20, 2021

View reviewed changes

qiluo-msft merged commit 3af4a96 into sonic-net:master Dec 20, 2021

judyjoseph added the Included in 202111 Branch label Dec 27, 2021

samaity mentioned this pull request Jan 8, 2022

[DPB] swss docker crashes when doing DPB in DellEMC Z9332f #9663

Closed

This was referenced Jan 31, 2022

[mellanox] Remove redundant breakout mode for SN3800 alexrallen/sonic-buildimage#27

Closed

[mellanox] Remove redundant breakout mode for SN3800 #9912

Merged

dgsudharsan mentioned this pull request Mar 2, 2022

[202106] [Mellanox] Fix DPB supported breakout modes #10129

Merged

6 tasks

dgsudharsan added the Request for 202106 Branch label Mar 2, 2022

arlakshm added the Included in 202106 Branch label Mar 24, 2022

noaOrMlnx mentioned this pull request Jan 14, 2025

[Mellanox] Add SPC5 SN5640 platform, Mellanox-SN5640-C512S2,Mellanox-SN5640-C448O16 SKUs noaOrMlnx/sonic-buildimage#47

Closed

11 tasks

Conversation

oleksandrivantsiv commented Nov 16, 2021

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

Uh oh!

dgsudharsan commented Nov 16, 2021

Uh oh!

zhenggen-xu commented Nov 17, 2021

Uh oh!

oleksandrivantsiv commented Nov 19, 2021

Uh oh!

samaity commented Nov 19, 2021

Uh oh!

liat-grozovik commented Nov 24, 2021

Uh oh!

azure-pipelines bot commented Nov 24, 2021

Uh oh!

liat-grozovik commented Nov 24, 2021

Uh oh!

liat-grozovik commented Nov 24, 2021

Uh oh!

azure-pipelines bot commented Nov 24, 2021

Uh oh!

liat-grozovik commented Dec 8, 2021

Uh oh!

lgtm-com bot commented Dec 9, 2021

Uh oh!

liat-grozovik commented Dec 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleksandrivantsiv Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samaity commented Dec 15, 2021

Uh oh!

oleksandrivantsiv commented Dec 15, 2021

Uh oh!

samaity left a comment

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liat-grozovik commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

oleksandrivantsiv Dec 15, 2021 •

edited

Loading

qiluo-msft Dec 16, 2021 •

edited

Loading