-
Notifications
You must be signed in to change notification settings - Fork 543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
swss: Dataplane telemetry support in SONiC #521
Conversation
please fix the vs test, it is broken. |
Fixed the VS test |
orchagent/orchdaemon.cpp
Outdated
m_orchList = { switch_orch, gCrmOrch, gPortsOrch, intfs_orch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, gBufferOrch, mirror_orch }; | ||
|
||
bool initialize_dtel = false; | ||
if (platform == BFN_PLATFORM_SUBSTRING) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in vs env, the platform is not BFN, then initialize_dtel is false and dtel_orch is not initialized, how can dtel vs test passing? Where do I miss here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I locally remove the platform check when I run the test. Without the check, I make sure it still passes for other platforms. Is there some other way to handle platform-specific feature tests for VS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add platform vs for this env, and you can check if platform == "vs" or platform == BFN_PLATFORM_SUBSTRING
tests/test_dtel.py
Outdated
atbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_DTEL") | ||
keys = atbl.getKeys() | ||
|
||
for k in keys: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, here the keys is null then, no assert is checked, that is why it is passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to assert len(keys) > 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check would fail if platform is not barefoot. Is there a "vs platform" check that we can add to orchdaemon.cpp to initialize dtel, in addition to barefoot?
@@ -33,6 +33,7 @@ const char config_db_key_delimiter = '|'; | |||
|
|||
#define MLNX_PLATFORM_SUBSTRING "mellanox" | |||
#define BRCM_PLATFORM_SUBSTRING "broadcom" | |||
#define BFN_PLATFORM_SUBSTRING "barefoot" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add BFN_PLATFORM_SUBSTRING "vs" here.
I will export the env in the vs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean #define VS_PLATFORM_SUBSTRING "vs"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
tests/test_dtel.py
Outdated
|
||
atbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_DTEL_REPORT_SESSION") | ||
keys = atbl.getKeys() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check keys is not empty.
tests/test_dtel.py
Outdated
atbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_DTEL_INT_SESSION") | ||
keys = atbl.getKeys() | ||
|
||
for k in keys: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check keys is not empty.
tests/test_dtel.py
Outdated
atbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_DTEL_QUEUE_REPORT") | ||
keys = atbl.getKeys() | ||
|
||
for k in keys: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check keys is not empty.
tests/test_dtel.py
Outdated
atbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_DTEL_EVENT") | ||
keys = atbl.getKeys() | ||
|
||
for k in keys: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check keys is not empty.
Enabled DTel for VS platform and fixed the test |
retest this please |
Looks like platform env variable is not set to "vs" and dtel test fails due to that in the newly added assert. sonic-buildimage seems to be at commit daf590e. How do we fix this to pick the latest? |
buildimage-vs-all build has been failing last couple of times. VS tests here are not passing because the requisite change in buildimage is not present |
platform vs is added. the dtel orch tests are passing, but it looks like it is causing mirror test to fail. it looks like it is causing two tests to fail.
@shruthi9 , ca you take a look? |
thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you explain a little bit about my questions?
@@ -13,9 +13,19 @@ def get_acl_table_id(self, dvs): | |||
assert dvs.asicdb.default_acl_table in keys | |||
|
|||
acl_tables = [k for k in keys if k not in dvs.asicdb.default_acl_table] | |||
assert len(acl_tables) == 1 | |||
assert len(acl_tables) >= 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the exact number of the ACL tables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DTel adds 2 additional ACL tables in addition to the default one
|
||
return acl_tables[0] | ||
# Filter out DTel Acl tables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this ACL table is there by default, you could add the table information in the conftest.py
file.
…onic-net#521) * Add buffer pool watermark support to watermarkstat Signed-off-by: Wenda Ni <wenni@microsoft.com> * Add buffer pool watermark to counterpoll Signed-off-by: Wenda Ni <wenni@microsoft.com> * Print N/A if the buffer pool watermark value is not present Signed-off-by: Wenda Ni <wenni@microsoft.com> * Fix syntax error; Change signature of function get_print_all_stat Signed-off-by: Wenda Ni <wenni@microsoft.com> * Address review comments Signed-off-by: Wenda Ni <wenni@microsoft.com>
Adding dataplane telemetry configuration support in SONiC
Pending: new VS tests to be added for this feature
A separate PR will be created for new community tests that will added for this feature
Design for the feature is at:
sonic-net/SONiC#182