[vstest]VS test support for VOQ system ports #696

vganesan-nokia · 2020-11-09T03:21:50Z

Signed-off-by: vedganes vedavinayagam.ganesan@nokia.com

SAI emulation support added for VOQ system ports. The changes include:
(1) Switch create initialization for VOQ switch objects
(2) Associating local port id to sytems ports that correspond to the
local ports. In real SAI, mapping between system port and local port
is done by matching the system ports's <switch_id, core index, core port
index> tuple. In VOQ switches, each port blongs to a switch core and is
assigned an index. This mapping is done in hardware configurations. For
VS, the mapping is emulated via a new file coreportindexmap.ini. This
file is made available in /usr/share/sonic/hwsku in the same way how
lanemap.ini is made avaialble. The loading and parsing of
coreportindexmap.ini is done in similar way as it is done for
lanemap.ini. While emulating configuration system ports, if a system
port is found to be a local port (determined using its switch_id), the
local port oid corresponding system port is retrieved from m_port_list
and local port atribute is set in the system port object. This is used
by orchagent (portsorch) for system port initialization.

Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com> SAI emulation support added for VOQ system ports. The changes include: (1) Switch create initialization for VOQ switch objects (2) Associating local port id to sytems ports that correspond ot the local ports. In real SAI, mapping between system port and local port is done by matching the system ports's <switch_id, core index, core port index> tuple. In VOQ switches, each port blongs to a switch core and is assigned an index. This mapping is done in hardware configurations. For VS, the mapping is emulated via a new file coreportindexmap.ini. This file is made available in /usr/share/sonic/hwsku in the same way how lanemap.ini is made avaialble. The loading and and parsing of coreportindexmap.ini is done in similar way as it is done for lanemap.ini. While emulating configuration system ports, if a system port is found to be a local port (determined using its switch_id), the local port oid corresponding system port is retrieved from m_port_list and local port atribute is set in the system port object. This is used by orchagent (portsorch) for system port initialization.

kcudnik

Don't ever create such huge PR's, create small prs preferred single class per PR, no need to add this new class to Makefile, you can make this on separate pr after all files will be in place
Address comments

kcudnik · 2020-11-09T08:56:08Z

vslib/inc/CorePortIndexMap.h

+
+            std::map<std::vector<uint32_t>, std::string> m_coreportindex_to_ifname;
+
+            std::map<std::string, std::vector<uint32_t>> m_ifname_to_coreportindex;
+


keep naming convention as other names like m_corePortIndexToIfName

Fixed in PR #698

vslib/inc/SwitchStateBase.h

kcudnik · 2020-11-09T08:57:34Z

vslib/inc/saivs.h

+/**
+ * @def SAI_KEY_VS_CORE_PORT_INDEX_MAP_FILE
+ *
+ * For VOQ systems if specified in profile.ini it should point to eth interface to
+ * core and core port index map as port name:core_index,core_port_index
+ *
+ * Example:
+ * eth1:0,1
+ * eth17:1,1
+ *
+ */
+#define SAI_KEY_VS_CORE_PORT_INDEX_MAP_FILE  "SAI_VS_CORE_PORT_INDEX_MAP_FILE"
+


move this UP, where SAI_KEY_* are declared

Fixed in PR #703

kcudnik · 2020-11-09T08:58:40Z

vslib/src/CorePortIndexMap.cpp

+    if (n != 2)
+    {
+        SWSS_LOG_ERROR("Invalid corePortIndex. Core port index must have core and core port index %d, %s", n, ifname.c_str());
+        return false;
+    }


if this is is alwasy 2, should it be better to have 2 input parameters in add method instead of vector ?

For now it is going to be only 2 fields. But not sure if this will change in the future. We may have switch_id (not switch index) as part of the mapping later.

kcudnik · 2020-11-09T09:00:26Z

vslib/src/CorePortIndexMap.cpp

+
+    for (uint32_t idx = 0; idx < defaultPortCount; idx++)
+    {
+        auto ifname = "eth" + std::to_string(idx);


move "eth" to some define in top of file or constexpr

Fixed in PR #698

vslib/src/SwitchStateBase.cpp

kcudnik · 2020-11-09T09:08:16Z

vslib/src/VirtualSwitchSaiInterface.cpp

@@ -659,6 +659,12 @@ sai_status_t VirtualSwitchSaiInterface::create(
            return SAI_STATUS_FAILURE;
        }

+        //Initialize switch for VOQ attributes


space after //

Fixed in PR #702

kcudnik · 2020-11-09T09:09:49Z

vslib/src/VirtualSwitchSaiInterface.cpp

@@ -659,6 +659,12 @@ sai_status_t VirtualSwitchSaiInterface::create(
            return SAI_STATUS_FAILURE;
        }

+        //Initialize switch for VOQ attributes
+        if ((ss->initialize_voq_switch_objects(attr_count, attr_list)) != SAI_STATUS_SUCCESS)


this seems wrong to me, this should not be here, since there can be warm boot executed, and voq objects may be different than cold boot, remove

The system ports are one time static configuration done at the start of the system. Unlike m_port_list, no configuration will change system ports and other voq switch objects during lifetime of the system. As of now, irrespective of whether the cold boot or warm boot, the system ports config is required to be always freshly configured. And since attr_count and attr_list are needed to initialize voq switch objects, voq swith object initialization is done from here. In the future, in case, dynamic configuration of system ports are implemented, we can always check for the warmboot state here and take action appropriately.

kcudnik · 2020-11-09T09:10:14Z

vslib/src/VirtualSwitchSaiInterface.cpp

+        //Initialize switch for VOQ attributes
+        if ((ss->initialize_voq_switch_objects(attr_count, attr_list)) != SAI_STATUS_SUCCESS)
+        {
+            SWSS_LOG_ERROR("VOQ switch initialization failed!");


init failed but you continue, throw here

Fixed in PR #702

vganesan-nokia · 2020-11-09T16:45:11Z

Don't ever create such huge PR's, create small prs preferred single class per PR, no need to add this new class to Makefile, you can make this on separate pr after all files will be in place
Address comments

I'll raise following multiple PRs

(1) PR for CorePortIndexMap.h and CorePortIndexMap.cpp
(2) PR for CorePortIndexMapContainer.h and CorePortIndexMapContainer.cpp
(3) PR for CorePortIndexMapFileParser.h and CorePortIndexMapFileParser.cpp
(4) PR for SwitchStateBase (.h and .cpp) changes without reference to coreportindxmap
(5) PR for VirtualSwitchSaiInterface.cpp changes to create VOQ switch objects
(6) PR for Makefile and Sai (.h and .cpp), SwitchConfig.h, saivs.h changes

I"ll also address other review comments in these PRs.

Thanks for the comments.

vganesan-nokia · 2020-11-10T01:27:32Z

Request re-review of this on the 6 small PRs (#698, #699, #700, #701, #702 and #703). I'll close this PR once these 6 PRs are merged.
Thanks

kcudnik · 2020-11-13T21:08:13Z

closing this since it was split to multiple PR

This was referenced Nov 9, 2020

[vstest]VS test for VOQ System ports sonic-net/sonic-buildimage#5862

Merged

Implementation of System ports initialization, Interface & Neighbor Synchronization... sonic-net/sonic-swss#1431

Merged

kcudnik requested changes Nov 9, 2020

View reviewed changes

vganesan-nokia requested a review from kcudnik November 10, 2020 01:24

kcudnik marked this pull request as draft November 13, 2020 11:51

kcudnik closed this Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vstest]VS test support for VOQ system ports #696

[vstest]VS test support for VOQ system ports #696

vganesan-nokia commented Nov 9, 2020

kcudnik left a comment

kcudnik Nov 9, 2020

vganesan-nokia Nov 10, 2020

kcudnik Nov 9, 2020

vganesan-nokia Nov 10, 2020

kcudnik Nov 9, 2020

vganesan-nokia Nov 10, 2020 •

edited

Loading

kcudnik Nov 9, 2020

vganesan-nokia Nov 10, 2020

kcudnik Nov 9, 2020

vganesan-nokia Nov 10, 2020

kcudnik Nov 9, 2020

vganesan-nokia Nov 10, 2020 •

edited

Loading

kcudnik Nov 9, 2020

vganesan-nokia Nov 10, 2020

vganesan-nokia commented Nov 9, 2020

vganesan-nokia commented Nov 10, 2020

kcudnik commented Nov 13, 2020


		std::map<std::vector<uint32_t>, std::string> m_coreportindex_to_ifname;

		std::map<std::string, std::vector<uint32_t>> m_ifname_to_coreportindex;

[vstest]VS test support for VOQ system ports #696

[vstest]VS test support for VOQ system ports #696

Conversation

vganesan-nokia commented Nov 9, 2020

kcudnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vganesan-nokia Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vganesan-nokia Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vganesan-nokia commented Nov 9, 2020

vganesan-nokia commented Nov 10, 2020

kcudnik commented Nov 13, 2020

vganesan-nokia Nov 10, 2020 •

edited

Loading

vganesan-nokia Nov 10, 2020 •

edited

Loading