[orchagent] Init field of supported speeds while port initialization #1622

ghost · 2021-02-02T13:16:53Z

Reading of supported speed list from the ASIC and saving it inside
PORT_TABLE:EthernetXX table in APPL_DB to be able to check supported
speeds while changing a current speed through CLI commands.

Signed-off-by: Maksym Belei Maksym_Belei@jabil.com

What I did
New field "supp_speed" in each PORT_TABLE:EthernetXX table inside APPL_DB have started to initialize while initialization of Ethernet ports. The field will contain list of supported by the port speeds, like "1000,10000,50000,10000".
If there was something wrong during reading information regarding supported speeds from ASIC, the field will not be preset in the table of APPL_DB. This situation should be properly handled in sonic-net/sonic-utilities#1395.

Why I did it
To provide information to sonic-utilities regarding supported speeds of all the Ethernet ports.

How I verified it

redis-cli -n 0
KEYS *
select any key with PORT_TABLE: prefix and execute HGETALL PORT_TABLE:EthernetXX
all the fields of the key should be displayed, including "supp_speed"(if there were no errors during reading the information from ASIC), like this

127.0.0.1:6379> HGETALL PORT_TABLE:Ethernet84
 1) "admin_status"
 2) "up"
 3) "alias"
 4) "etp22"
 5) "index"
 6) "22"
 7) "lanes"
 8) "84,85,86,87"
 9) "mtu"
10) "9100"
11) "oper_status"
12) "down"
13) "supp_speed"
14) "1000,10000,25000,40000,50000,56000,100000"

Details if related
Related PRs:
sonic-net/sonic-utilities#1395
sonic-net/sonic-utilities#1391

prsunny · 2021-02-02T20:28:55Z

orchagent/portsorch.cpp

+    auto supp_speed_temp = getSupportedSpeed(port.m_alias, port.m_port_id);
+    /* Copy the vector to be able to modify it */
+    auto supp_speed(supp_speed_temp);
+    if (supp_speed.size() > 0) {


{ -> new line

prsunny · 2021-02-02T20:29:05Z

orchagent/portsorch.cpp

+        sort(supp_speed.begin(), supp_speed.end());
+
+        string tmp_supp_speed_str = "";
+        /* Add (cound - 1) elements to the string with separator*/


count - typo

prsunny · 2021-02-03T18:58:35Z

@dgsudharsan , can you review this?

dgsudharsan · 2021-02-04T16:26:46Z

We also get the list of allowed speeds from platform.json eg ("breakout_modes": "1x100G[50G,40G,25G,10G],2x50G[40G,25G,10G]") The hardware supported speeds would be generally more than the allowable speeds listed in platform.json (which are the platform validated ones). Shouldn't we be using this instead of fetching from the hardware and using it as valid speeds?

ghost · 2021-02-08T08:14:18Z

@dgsudharsan, @prsunny,

I am not sure about using of configuration of Breakout Mode feature for supported speed determination purpose. Is it a good idea to establish dependencies between those features? What if the Breakout Mode configuration parameters will not be provided in platform.json or a syntax on the parameters will be changed?
Currently, the system checks available speed from ASIC, but this makes after setting the desirable port speed to Config DB, and as there is no rollback mechanism for the speed parameter in Orchagent, we are getting invalid speed parameter, shown by command show interface status, for example. The idea is to prevent such situations, so, I decided to use information from ASIC little bit earlier, in Sonic-utilities.

prsunny · 2021-02-08T17:47:58Z

/AzurePipelines run

azure-pipelines · 2021-02-08T17:48:08Z

Azure Pipelines successfully started running 1 pipeline(s).

dgsudharsan · 2021-02-08T17:52:26Z

@dgsudharsan, @prsunny,

I am not sure about using of configuration of Breakout Mode feature for supported speed determination purpose. Is it a good idea to establish dependencies between those features? What if the Breakout Mode configuration parameters will not be provided in platform.json or a syntax on the parameters will be changed?
Currently, the system checks available speed from ASIC, but this makes after setting the desirable port speed to Config DB, and as there is no rollback mechanism for the speed parameter in Orchagent, we are getting invalid speed parameter, shown by command show interface status, for example. The idea is to prevent such situations, so, I decided to use information from ASIC little bit earlier, in Sonic-utilities.

Hi,
Please take a look at this. The speeds must be part of platform.json.
https://github.com/Azure/SONiC/blob/master/doc/dynamic-port-breakout/sonic-dynamic-port-breakout-HLD.md

A capability file for a platform with port breakout capabilities is provided. It defines the initial configurations like lanes, speed, alias etc. This file will be used for CLI later to pick the parent port and breakout mode. It can be used for speed checks based on the current mode. It (in conjunction with hwsku.json talked later) will also replace the functionality of the current existing port_config.ini.

Speeds in the breakout modes will be used for speed validation when we change the individual ports speed later. Eg, In 4x25G[10G] mode, we can change the individual port in that port group to 25G or 10G, but not some other speeds.

dgsudharsan · 2021-02-08T18:04:06Z

@zhenggen-xu Please decide if this PR is needed given that port breakout handles the speed checks

zhenggen-xu · 2021-02-08T21:54:02Z

I think we could keep both speed checks to get more protection.

orchagent check is based on ASIC SAI for ports, and breakout mode and platform.json should be more restrict since the speed was based on the platform (not just ASIC) capabilities and breakout mods. In general, breakout speed check is better. However, not all platforms have adopted the breakout design, and meanwhile having backend SAI checks won't be conflicting with that and can provide protection to some extend before all platforms are onboarded with breakout feature.

ghost · 2021-02-09T16:09:09Z

@zhenggen-xu, so, if my understanding is correct, we could use this solution as a temporary, till breakout feature will be completely integrated to all the platforms? @prsunny, could you check the comment from @zhenggen-xu?

prsunny · 2021-02-09T16:12:10Z

@zhenggen-xu, so, if my understanding is correct, we could use this solution as a temporary, till breakout feature will be completely integrated to all the platforms? @prsunny, could you check the comment from @zhenggen-xu?

Fine with me. @zhenggen-xu, can we take up breakout later?

zhenggen-xu · 2021-02-16T06:31:35Z

@zhenggen-xu, so, if my understanding is correct, we could use this solution as a temporary, till breakout feature will be completely integrated to all the platforms? @prsunny, could you check the comment from @zhenggen-xu?

Fine with me. @zhenggen-xu, can we take up breakout later?

We will make the check in CLI if dynamic breakout (DPB) is utilized, it is up to box vendor to onboard the platform with DPB feature.

ghost · 2021-03-10T13:34:47Z

@prsunny, is there any updates regarding the PR?

dprital · 2021-06-14T14:53:35Z

@prsunny - Do you have an update for this PR ?

prsunny · 2021-06-14T17:11:29Z

@prsunny - Do you have an update for this PR ?

can you please rebase? Also @zhenggen-xu to signoff

dgsudharsan · 2021-10-13T05:43:34Z

@maksymbelei95 Is there any update on this PR?

Rebase sonic-net#1622 to master Signed-off-by: Mykola Horb <mykola_horb@jabil.com>

* Reading of supported speed list from the ASIC and saving it inside PORT_TABLE:EthernetXX table in APPL_DB to be able to check supported speeds while changing a current speed through CLI commands. Signed-off-by: Maksym Belei <Maksym_Belei@jabil.com>

pshulik · 2021-10-22T04:54:36Z

/AzurePipelines run

azure-pipelines · 2021-10-22T04:54:41Z

Commenter does not have sufficient privileges for PR 1622 in repo Azure/sonic-swss

pshulik · 2021-10-22T14:40:37Z

@prsunny , @zhenggen-xu please review it

zhenggen-xu · 2021-10-25T05:10:54Z

orchagent/portsorch.cpp

@@ -1783,21 +1783,73 @@ bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip)
    return true;
 }

-bool PortsOrch::isSpeedSupported(const std::string& alias, sai_object_id_t port_id, sai_uint32_t speed)
+const PortSupportedSpeeds& PortsOrch::getSupportedSpeed(const std::string& alias, sai_object_id_t port_id)


This function seemed to share a lot code with void PortsOrch::getPortSupportedSpeeds(), can we re-use the code by introduce some common function block?

zhenggen-xu · 2021-11-03T22:28:04Z

@zhenggen-xu we made some changes regarding the use of common code cbf3621 b15a29f

Can you update the PR with the changes so they could be reviewed here?

pshulik · 2021-11-12T06:48:08Z

@zhenggen-xu, Could you please review the latest changes?

zhenggen-xu · 2021-11-12T16:28:08Z

@zhenggen-xu, Could you please review the latest changes?

@pshulik @KonstiantynHalushka As mentioned above, please update the PR itself instead of sharing commits from different places so we can review the finial code here.

KonstiantynHalushka · 2021-11-16T17:04:25Z

@zhenggen-xu @prsunny Sorry for the wrong update of the PR. Please, review the finial code.

zhenggen-xu · 2021-11-17T06:35:07Z

orchagent/portsorch.cpp

+    if (!m_portSupportedSpeeds.count(port_id))
+    {
+    	PortSupportedSpeeds supported_speeds;
+    	getPortSupportedSpeeds(alias, port_id, &supported_speeds);


should be getPortSupportedSpeeds(alias, port_id, supported_speeds);?

zhenggen-xu · 2021-11-17T06:37:04Z

orchagent/portsorch.cpp

+    // This method will return false if we get a list of supported speeds and the requested speed
+    // is not supported
+    // Otherwise the method will return true (even if did not receive list of supported speed)
+


Since we removed initPortSupportedSpeeds in this function, just want to make sure initPortSupportedSpeeds was called always before calling this function.

… function, since when calling isSpeedSupported, you need to initialize m_portStateTable if it has not already been done Signed-off-by: KonstiantynHalushka <Konstiantyn_Halushka@gabil.com>

KonstiantynHalushka · 2021-11-17T20:52:57Z

@zhenggen-xu I accepted your comments

zhenggen-xu · 2021-11-18T06:37:40Z

Just checked the PR again, I failed to understand the reason we introduce the new functions and code to collect the speeds. Isn't that done in the existing function initPortSupportedSpeeds() where the speeds were saved to stateDB? see: https://github.com/Azure/sonic-swss/blob/master/orchagent/portsorch.cpp#L1864 . Can we just leverage that for CLI check? Any reason we need move them to appDB? Even if we need to do so, why don't we just put the same info to appDB instead of all the new code?

Sorry for being late for these findings, or maybe I am missing something here?

pshulik · 2021-11-26T07:38:43Z

Hi zhenggen-xu
The initPortSupportedSpeeds() is called one time during initialization. Issue that optical transceiver can be replaced during operation time and supported speeds might be different, so, information obtained via initPortSupportedSpeeds() might be not actual.

zhenggen-xu · 2021-11-30T07:16:37Z

Hi zhenggen-xu The initPortSupportedSpeeds() is called one time during initialization. Issue that optical transceiver can be replaced during operation time and supported speeds might be different, so, information obtained via initPortSupportedSpeeds() might be not actual.

The port speeds were obtained by SAI API CALLs which should not care about optical transceivers. This is purely ASIC capabilities, and we can just reuse the same support speeds as got during init time?

The latest version of pyroute2 introduce breaking change

ghost mentioned this pull request Feb 2, 2021

[portconfig] Check supported speeds before setting port speed sonic-net/sonic-utilities#1395

Open

ghost marked this pull request as ready for review February 2, 2021 13:55

ghost mentioned this pull request Feb 2, 2021

Config utility allows incorrect values for interface speed sonic-net/sonic-buildimage#6424

Open

prsunny reviewed Feb 2, 2021

View reviewed changes

ghost requested a review from prsunny February 8, 2021 08:14

prsunny requested a review from zhenggen-xu February 8, 2021 18:48

NGorb-jabil added a commit to NGorb-jabil/sonic-swss that referenced this pull request Oct 21, 2021

[orchagent] Init field of supported speeds while port initialization

dc5d0ef

Rebase sonic-net#1622 to master Signed-off-by: Mykola Horb <mykola_horb@jabil.com>

zhenggen-xu reviewed Oct 25, 2021

View reviewed changes

KonstiantynHalushka mentioned this pull request Nov 4, 2021

[orchagent] Init field of supported speeds while port initialization, refactoring #2007

Open

[orchagent/portsorch] Refactoring, remove duplicate code

ad7de0a

zhenggen-xu suggested changes Nov 17, 2021

View reviewed changes

KonstiantynHalushka added 2 commits November 17, 2021 10:32

[orchagent/portsorch] Fixed transfer by reference

addfe2c

[orchagent/portsorch] Return of a call to the initPortSupportedSpeeds…

144b6c1

… function, since when calling isSpeedSupported, you need to initialize m_portStateTable if it has not already been done Signed-off-by: KonstiantynHalushka <Konstiantyn_Halushka@gabil.com>

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022

Limit pyroute2 version range due to upstream broken (sonic-net#1622)

00bd0ce

The latest version of pyroute2 introduce breaking change

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[orchagent] Init field of supported speeds while port initialization #1622

[orchagent] Init field of supported speeds while port initialization #1622

ghost commented Feb 2, 2021 •

edited by ghost

Loading

prsunny Feb 2, 2021

ghost Feb 3, 2021

prsunny Feb 2, 2021

ghost Feb 3, 2021

prsunny commented Feb 3, 2021

dgsudharsan commented Feb 4, 2021

ghost commented Feb 8, 2021

prsunny commented Feb 8, 2021

azure-pipelines bot commented Feb 8, 2021

dgsudharsan commented Feb 8, 2021

dgsudharsan commented Feb 8, 2021

zhenggen-xu commented Feb 8, 2021

ghost commented Feb 9, 2021

prsunny commented Feb 9, 2021

zhenggen-xu commented Feb 16, 2021

ghost commented Mar 10, 2021

dprital commented Jun 14, 2021

prsunny commented Jun 14, 2021

dgsudharsan commented Oct 13, 2021

pshulik commented Oct 22, 2021

azure-pipelines bot commented Oct 22, 2021

pshulik commented Oct 22, 2021

zhenggen-xu Oct 25, 2021

zhenggen-xu commented Nov 3, 2021

pshulik commented Nov 12, 2021

zhenggen-xu commented Nov 12, 2021

KonstiantynHalushka commented Nov 16, 2021

zhenggen-xu Nov 17, 2021

zhenggen-xu Nov 17, 2021

KonstiantynHalushka commented Nov 17, 2021

zhenggen-xu commented Nov 18, 2021 •

edited

Loading

pshulik commented Nov 26, 2021

zhenggen-xu commented Nov 30, 2021

[orchagent] Init field of supported speeds while port initialization #1622

Are you sure you want to change the base?

[orchagent] Init field of supported speeds while port initialization #1622

Conversation

ghost commented Feb 2, 2021 • edited by ghost Loading

prsunny Feb 2, 2021

Choose a reason for hiding this comment

ghost Feb 3, 2021

Choose a reason for hiding this comment

prsunny Feb 2, 2021

Choose a reason for hiding this comment

ghost Feb 3, 2021

Choose a reason for hiding this comment

prsunny commented Feb 3, 2021

dgsudharsan commented Feb 4, 2021

ghost commented Feb 8, 2021

prsunny commented Feb 8, 2021

azure-pipelines bot commented Feb 8, 2021

dgsudharsan commented Feb 8, 2021

dgsudharsan commented Feb 8, 2021

zhenggen-xu commented Feb 8, 2021

ghost commented Feb 9, 2021

prsunny commented Feb 9, 2021

zhenggen-xu commented Feb 16, 2021

ghost commented Mar 10, 2021

dprital commented Jun 14, 2021

prsunny commented Jun 14, 2021

dgsudharsan commented Oct 13, 2021

pshulik commented Oct 22, 2021

azure-pipelines bot commented Oct 22, 2021

pshulik commented Oct 22, 2021

zhenggen-xu Oct 25, 2021

Choose a reason for hiding this comment

zhenggen-xu commented Nov 3, 2021

pshulik commented Nov 12, 2021

zhenggen-xu commented Nov 12, 2021

KonstiantynHalushka commented Nov 16, 2021

zhenggen-xu Nov 17, 2021

Choose a reason for hiding this comment

zhenggen-xu Nov 17, 2021

Choose a reason for hiding this comment

KonstiantynHalushka commented Nov 17, 2021

zhenggen-xu commented Nov 18, 2021 • edited Loading

pshulik commented Nov 26, 2021

zhenggen-xu commented Nov 30, 2021

ghost commented Feb 2, 2021 •

edited by ghost

Loading

zhenggen-xu commented Nov 18, 2021 •

edited

Loading