SAI PTF 2: sai_base_test infra #1335

aljer · 2021-11-02T20:19:21Z

The changes introduce SAI PTF framework base tests infrastructure. Following files have been added:

sai_rpc_frontend.cpp - RPC server frontend file - there is a set of manually written helper functions that converts/parses SAI and thrift attributes and also here we have autogenerated SAI RPC server functions included
sai_base_test.py - python module with base test classes with common tests configuration
sai_utils.py - python module with some helper function used in SAI PTF tests
saitest.py - first test module with simple framework test

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-11-02T20:26:14Z

This pull request introduces 36 alerts when merging 855a461 into 422fc84 - view on LGTM.com

new alerts:

32 for Imprecise assert
3 for 'import *' may pollute namespace
1 for Module is imported with 'import' and 'import from'

ravi861 · 2021-11-02T22:15:53Z

@rlhui @richardyu-ms fyi

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-11-03T11:04:18Z

This pull request introduces 36 alerts when merging dfc9e0a into be03407 - view on LGTM.com

new alerts:

32 for Imprecise assert
3 for 'import *' may pollute namespace
1 for Module is imported with 'import' and 'import from'

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-11-03T13:34:40Z

This pull request introduces 36 alerts when merging d371ec0 into be03407 - view on LGTM.com

new alerts:

32 for Imprecise assert
3 for 'import *' may pollute namespace
1 for Module is imported with 'import' and 'import from'

kcudnik · 2021-11-03T16:37:33Z

meta/sai_rpc_frontend.cpp

@@ -0,0 +1,870 @@
+/**
+ * Copyright (c) 2021 Microsoft Open Technologies, Inc.


can this file be located in ptf directory ?

This file make use of autogenerated RPC code which also is to be placed in meta directory and its subdirectories. Please see: https://github.com/opencomputeproject/SAI/pull/1325/files

We can maybe allow the gensairpc script to allow to generate the files in a different directory than meta as an argument. Would it help if we create a top-level "thriftrpc" directory to place this and other related files?

@kcudnik - please confirm?

what should i confirm here?

@kcudnik - wanted to make sure if we're in agreement on "create a top-level "thriftrpc" directory to place this and other related files"

that should be fine

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-11-04T12:05:15Z

This pull request introduces 36 alerts when merging 6cfeabc into be03407 - view on LGTM.com

new alerts:

32 for Imprecise assert
3 for 'import *' may pollute namespace
1 for Module is imported with 'import' and 'import from'

rlhui · 2021-11-16T22:05:15Z

Adding @smaheshm to review as well.

kcudnik · 2021-12-09T09:09:55Z

new alerts:

32 for Imprecise assert

3 for 'import *' may pollute namespace

1 for Module is imported with 'import' and 'import from'

please address LGTM issues

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-12-09T10:35:39Z

This pull request introduces 2 alerts when merging acd7f69 into 05648bc - view on LGTM.com

new alerts:

2 for 'import *' may pollute namespace

aljer · 2021-12-09T10:44:19Z

This pull request introduces 2 alerts when merging acd7f69 into 05648bc - view on LGTM.com

new alerts:

2 for 'import *' may pollute namespace

@kcudnik I addressed almost all issues. Those two which remain are required from the perspective of tests infrastructure.

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-12-10T08:50:29Z

This pull request introduces 2 alerts when merging ddcba71 into 05648bc - view on LGTM.com

new alerts:

2 for 'import *' may pollute namespace

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-12-10T09:10:38Z

This pull request introduces 2 alerts when merging 6d10ea9 into 05648bc - view on LGTM.com

new alerts:

2 for 'import *' may pollute namespace

smaheshm · 2021-12-10T23:55:02Z

Regarding "sai_rpc_frontend.cpp - RPC server frontend file - there is a set of manually written helper functions that converts/parses SAI and thrift attributes and also here we have autogenerated SAI RPC server functions included"

Why not keep these two separate? why mix auto generated file with manual addition?

ravi861 · 2021-12-11T00:10:10Z

Regarding "sai_rpc_frontend.cpp - RPC server frontend file - there is a set of manually written helper functions that converts/parses SAI and thrift attributes and also here we have autogenerated SAI RPC server functions included"

Why not keep these two separate? why mix auto generated file with manual addition?

The helper functions are defined in sai_rpc_frontend.cpp and used in the generated sai_rpc_server.cpp.
The sai_rpc_server.cpp is a large class definition with all the member functions declared and defined inline and use the handwritten helper functions.
So there is a circular dependency on who includes the other. The sai_rpc_server.cpp is included in sai_rpc_frontend.cpp with a #include right after the helper function definitions, which, I agree is a little unconventional.

smaheshm · 2021-12-13T21:05:10Z

meta/sai_rpc_frontend.cpp

+
+using namespace ::sai;
+
+unsigned int sai_thrift_mac_t_parse(const std::string s, void *data) {


can you add comments to describe what the functions are doing.

Okay, I will add them

smaheshm · 2021-12-13T21:12:17Z

meta/sai_rpc_frontend.cpp

+  return (j == 12);
+}
+
+void sai_thrift_ip4_t_parse(const std::string s, unsigned int *m) {


How about adding unit tests for the APIs performing SAI thrift to SAI and vice versa.

smaheshm · 2021-12-13T21:27:20Z

meta/sai_rpc_frontend.cpp

+#include "sai_rpc_server.cpp"
+
+class sai_rpcHandlerFrontend : virtual public sai_rpcHandler {
+  int64_t sai_thrift_object_type_get_availability(


curious why this (and others) had to be written manually.

These two manually written 'sai_thrift_' functions are specific and they don't match the patterns the generator is working with. There are only two of such functions related to CRM verification.

smaheshm · 2021-12-13T21:59:58Z

ptf/sai_base_test.py

+        time.sleep(timeout + aging_interval_buffer)
+
+
+class SaiHelper(SaiHelperBase):


Does it make sense to rename this class since it sets up a specific configuration that all tests may not be able to use.

Do you mean this class should not be named "SaiHelper"? Actually this class "helps" to set designed configuration which we use in most of the tests. This is even more common than using SaiHelperBace as a parent class.

Most tests which are being upstreamed use this as the base class and use the configuration defined in this class. New tests may choose to use SaiHelperBase instead to define their own configuration.

smaheshm · 2021-12-13T22:15:38Z

Rest looks good. I think adding unit tests (gtests) to the conversion APIs will be useful to catch regression when someone changes those functions.

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-12-15T20:36:06Z

This pull request introduces 2 alerts when merging d558751 into 05648bc - view on LGTM.com

new alerts:

2 for 'import *' may pollute namespace

…tests Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-12-15T21:01:45Z

This pull request introduces 2 alerts when merging d2cbed8 into 05648bc - view on LGTM.com

new alerts:

2 for 'import *' may pollute namespace

smaheshm · 2021-12-15T23:08:43Z

ptf/lpm.py

+
+Please check the test_lpm.py file to see the details of how this class works.
+'''
+class LpmDict():


where is this object instantiated? If not used can you remove this file and add when it's being used.

This module is to be used in saiswitch tests that will be upstreamed soon. But in general it is a part of the infrastructure and may be used in any other test modules.

Code shouldn't be added with the assumption it will be used, this way we may end up with unused code lying around. Can you remove this file and add it when it's being used, i.e. along with saiswitch tests.

Okay, as you wish

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-12-16T08:20:33Z

This pull request introduces 2 alerts when merging 953dd9f into 05648bc - view on LGTM.com

new alerts:

2 for 'import *' may pollute namespace

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

lgtm-com · 2021-12-16T19:11:33Z

This pull request introduces 2 alerts when merging a5a7d94 into 05648bc - view on LGTM.com

new alerts:

2 for 'import *' may pollute namespace

smaheshm · 2021-12-16T19:59:09Z

thanks for removing the file. can you address the LGTM alerts.

aljer · 2021-12-16T20:11:01Z

thanks for removing the file. can you address the LGTM alerts.

@smaheshm As I explained here #1335 (comment) these two imports causing the alerts are required by all other test modules. This is one of a framework assumptions.

smaheshm

The changes introduce SAI PTF framework base tests infrastructure. Following files have been added: sai_rpc_frontend.cpp - RPC server frontend file - there is a set of manually written helper functions that converts/parses SAI and thrift attributes and also here we have autogenerated SAI RPC server functions included sai_base_test.py - python module with base test classes with common tests configuration sai_utils.py - python module with some helper function used in SAI PTF tests saitest.py - first test module with simple framework test Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

[Cherry-Pick]SAI PTF 2: sai_base_test infra (#1335)

The changes introduce SAI PTF framework base tests infrastructure. Following files have been added: sai_rpc_frontend.cpp - RPC server frontend file - there is a set of manually written helper functions that converts/parses SAI and thrift attributes and also here we have autogenerated SAI RPC server functions included sai_base_test.py - python module with base test classes with common tests configuration sai_utils.py - python module with some helper function used in SAI PTF tests saitest.py - first test module with simple framework test Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

The changes introduce SAI PTF framework base tests infrastructure. Following files have been added: sai_rpc_frontend.cpp - RPC server frontend file - there is a set of manually written helper functions that converts/parses SAI and thrift attributes and also here we have autogenerated SAI RPC server functions included sai_base_test.py - python module with base test classes with common tests configuration sai_utils.py - python module with some helper function used in SAI PTF tests saitest.py - first test module with simple framework test Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com> Signed-off-by: Yang Wang <yangwang1@microsoft.com>

* SAI PTF 1: Proposal (#1325) This is an autogeneration framework for PTF based SAI tests. Before in order to add a new test it is necessary to: 1. Add an entry in switch_sai.thrift. 2. Add RPC server method for new entry in switch_sai_rpc_server.cpp. 3. Add a python wrapper in switch.py (if applicable). 4. Write a new test The main goal of autogeneration framework is to generate first three steps based on SAI headers to facilitate the process of writing new tests. * Create SAI-Proposal-SAI_PTF.md * Adding generator script * New makefile target and cleanup Signed-off-by: Yang Wang <yangwang1@microsoft.com> * SAI PTF 2: sai_base_test infra (#1335) The changes introduce SAI PTF framework base tests infrastructure. Following files have been added: sai_rpc_frontend.cpp - RPC server frontend file - there is a set of manually written helper functions that converts/parses SAI and thrift attributes and also here we have autogenerated SAI RPC server functions included sai_base_test.py - python module with base test classes with common tests configuration sai_utils.py - python module with some helper function used in SAI PTF tests saitest.py - first test module with simple framework test Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com> Signed-off-by: Yang Wang <yangwang1@microsoft.com> * Proposal for init switch and destroy switch from PRC (#1387) keep the switch id in creat_switch and remove it from remove_switch 1. support to create the create_switch only once before the test run and use the same switch id within multi-round of testing 2. support remove the switch id Signed-off-by: Richard Yu <richard.yu@microsoft.com> Signed-off-by: Yang Wang <yangwang1@microsoft.com> * [SAIServer]Add new SaiServer to support ptf-SAI test structure (#1388) * [SAIServer]Add new SaiServer to support ptf-SAI test structure Add build script to generate the necessary package as saiserverv2 Add new saiserver to support new structure - without switch init and with thrift 0.13.0 Add makefile add sai_rpc_frontend simplify saiserver and remove init method add permission for copy_saithriftv2.s copy saiserver at dh builddeb ctypesgen for sai-ptf and install python-saithrift for two version Signed-off-by: richardyu-ms <richard.yu@microsoft.com> * add a package name for sai_adapter Signed-off-by: richardyu-ms <richard.yu@microsoft.com> * address review comments Signed-off-by: richardyu-ms <richard.yu@microsoft.com> * add more comments for the command in the Makefile Signed-off-by: richardyu-ms <richard.yu@microsoft.com> Signed-off-by: Yang Wang <yangwang1@microsoft.com> * Add platform support and a sanity test sample (#1390) (#1415) * Add a middle layer (strategy and factory) under the test cases which can no more if-else for platform selecting no more code injection for different platforms, just need to abstract to a method on the differences auto select the platform by the parameters in the test starter (shell, happed in run_p4_tests, not published here) easy-distinct structure for the difference from platforms prepared for future statistic Add a sanity sample, it can configure across all the ports (base on port's configuration file) simple FDB and VLAN naming according to port's number make a basic check on the status of the devices use as a sample for how to use the new middle layer fix and suppress LGTM alters Signed-off-by: Richard Yu <richard.yu@microsoft.com> * code refactor part I - add comments for class and method - reformat code - move platform related method to corresponding class code refactor Part II optimize the class hirerarchy simplify the import items add more docs Signed-off-by: richardyu-ms <richard.yu@microsoft.com> * suppress lgtm warnings fix lgtm warning Signed-off-by: richardyu-ms <richard.yu@microsoft.com> Signed-off-by: Yang Wang <yangwang1@microsoft.com> * [FIX]Fix the circular reference issue when build sai header py (#1427) Fix circular reference issue. Test done: build the saiserver and check the sai_headers.py Signed-off-by: richardyu-ms <richard.yu@microsoft.com> Signed-off-by: Yang Wang <yangwang1@microsoft.com> * Skip brcm teardown assertion (#1423) (#1428) Signed-off-by: Yang Wang <yangwang1@microsoft.com> * Add two more API as RPC for warmboot testing (#1421) (#1437) try to expose the sai api object_type_query and switch_id_query for warmboot testing Modify the template file for generate code from meta. Signed-off-by: richardyu-ms <richard.yu@microsoft.com> Signed-off-by: Yang Wang <yangwang1@microsoft.com> Co-authored-by: Patrycja Kochmanska <patrycja.kochmanska@intel.com> Co-authored-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com> Co-authored-by: Richard.Yu <richard.yu@microsoft.com>

kcudnik · 2023-08-31T12:56:03Z

meta/sai_rpc_frontend.cpp

+// including it here we never have to modify the generated file
+#include "sai_rpc_server.cpp"


this is wrong way to do it :(

kcudnik · 2023-09-08T09:32:15Z

ptf/sai_base_test.py

+        switch_resources = sai_thrift_get_switch_attribute(
+            self.client,
+            available_ipv4_route_entry=True,
+            available_ipv6_route_entry=True,
+            available_ipv4_nexthop_entry=True,
+            available_ipv6_nexthop_entry=True,
+            available_ipv4_neighbor_entry=True,
+            available_ipv6_neighbor_entry=True,
+            available_next_hop_group_entry=True,
+            available_next_hop_group_member_entry=True,
+            available_fdb_entry=True,
+            available_ipmc_entry=True,
+            available_snat_entry=True,
+            available_dnat_entry=True,
+            available_double_nat_entry=True,
+            number_of_ecmp_groups=True,
+            ecmp_members=True)


this is actually wrong approach, since not a lot of vendors has those implemented, for example current DASH implementation, and this function will throw sai thrift exception if any of those values will not be success, and this will then generate SkipTest exception if error is SAI_STATUS_NOT_SUPPORTED, which is what happens, and many test will/can be skipped if those values are not present, but they not prevent actually for test to be executed, each test should handle this independently or this function here should handle this in a way that if not supported, then set this resource to zero

@lguohan @chrispsommers fyi

@kcudnik It does seem like the PTF infrastructure makes some assumptions about base capabilities of any SAI target which has caused some issues in DASH and forced some workarounds. It's arguable whether this is a flaw in the PTF framework and its assumptions, or that the DASH behavioral model falls too short of the mark for emulating a target in all its SAI support. Recent work by some vendors in SAI made many allowances for DASH such as supporting only 2-port devices, etc. but it didn't try to accommodate all the shortcomings if the current DASH behavioral model.

If there is opportunity to establish the capabilities/resources of the target and use that information to skip tests, that would be preferred.

Aleksandra Jereczek added 4 commits November 2, 2021 19:52

Add sai_rpc_frontend.cpp file

2639bd4

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Add sai_base_test.py file

19cdcb4

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Add sai_utils.py file

e359911

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Add saitest.py file

855a461

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Correct spelling issues

dfc9e0a

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Aleksandra Jereczek added 2 commits November 3, 2021 13:20

Add MSFT license header to sai_rpc_frontend file

5261b06

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Verify initialized switch_id exists

d371ec0

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

kcudnik reviewed Nov 3, 2021

View reviewed changes

Minor docstring fix

6cfeabc

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

richardyu-ms approved these changes Nov 9, 2021

View reviewed changes

richardyu-ms mentioned this pull request Nov 10, 2021

[Cherry pick] Merge saiserver related updates to v1.9 #1338

Merged

Address LGTM checker issues

acd7f69

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Minor sai_base_test improvement

ddcba71

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Change default objects lists names

6d10ea9

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

smaheshm reviewed Dec 13, 2021

View reviewed changes

Add brief descriptions for sai_rpc_frontend functions

d558751

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Add lpm.py module as a part of tests infrastructure required by some …

d2cbed8

…tests Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

smaheshm reviewed Dec 15, 2021

View reviewed changes

Address spelling issues

953dd9f

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

Remove lpm.py module

a5a7d94

Signed-off-by: Aleksandra Jereczek <aleksandra.jereczek@intel.com>

smaheshm approved these changes Dec 16, 2021

View reviewed changes

rlhui merged commit 0a18470 into opencomputeproject:master Dec 22, 2021

aljer deleted the sai_ptf_infra branch December 22, 2021 07:23

RyoYang mentioned this pull request Jan 5, 2022

[Cherry-Pick]SAI PTF 2: sai_base_test infra (#1335) #1385

Merged

richardyu-ms added a commit that referenced this pull request Jan 6, 2022

Merge pull request #1385 from RyoYang/merge_sai_ptf_2

1455a12

[Cherry-Pick]SAI PTF 2: sai_base_test infra (#1335)

RyoYang mentioned this pull request Jan 6, 2022

[Fixed] delete conflict content from cherry-pick #1389

Merged

richardyu-ms mentioned this pull request Jan 6, 2022

Add platform support and a sanity test sample #1390

Merged

kcudnik reviewed Aug 31, 2023

View reviewed changes

kcudnik reviewed Sep 8, 2023

View reviewed changes

		@@ -0,0 +1,870 @@
		/**
		* Copyright (c) 2021 Microsoft Open Technologies, Inc.


		using namespace ::sai;

		unsigned int sai_thrift_mac_t_parse(const std::string s, void *data) {

		time.sleep(timeout + aging_interval_buffer)


		class SaiHelper(SaiHelperBase):

		// including it here we never have to modify the generated file
		#include "sai_rpc_server.cpp"

SAI PTF 2: sai_base_test infra #1335

SAI PTF 2: sai_base_test infra #1335

Conversation

aljer commented Nov 2, 2021

lgtm-com bot commented Nov 2, 2021

ravi861 commented Nov 2, 2021

lgtm-com bot commented Nov 3, 2021

lgtm-com bot commented Nov 3, 2021

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

Choose a reason for hiding this comment

lgtm-com bot commented Nov 4, 2021

rlhui commented Nov 16, 2021

kcudnik commented Dec 9, 2021

lgtm-com bot commented Dec 9, 2021

aljer commented Dec 9, 2021

lgtm-com bot commented Dec 10, 2021

lgtm-com bot commented Dec 10, 2021

smaheshm commented Dec 10, 2021

ravi861 commented Dec 11, 2021

smaheshm Dec 13, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaheshm commented Dec 13, 2021

lgtm-com bot commented Dec 15, 2021

lgtm-com bot commented Dec 15, 2021

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

lgtm-com bot commented Dec 16, 2021

lgtm-com bot commented Dec 16, 2021

smaheshm commented Dec 16, 2021

aljer commented Dec 16, 2021

smaheshm 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

smaheshm Dec 13, 2021 •

edited