Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sonic-sairedis : Wred stats feature changes on Sai-redis and Syncd #1234

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sonic-sairedis : Wred stats feature changes on Sai-redis and Syncd #1234

wants to merge 1 commit into from

Conversation

rpmarvell
Copy link

@rpmarvell rpmarvell commented Apr 24, 2023 •
edited
Loading

  • Stats capability query API support is added

Details :

  • Changes are done to support the stats capability query from Swss to SAI.
  • This was implemented as part of WRED and ECN statistics
  • Statistics capability query is implemented in sairedis/syncd

HLD : https://github.com/sonic-net/SONiC/blob/ebcd2a4a987f1d6027cd57677dc6806b8a9adcdb/doc/qos/ECN_and_WRED_statistics_HLD.md#show-interface-counters-cli-output-on-a-wred-drop-statistics-supported-platform

The Build is failing in this review because of the swss-common-dependency at PR : sonic-net/sonic-swss-common#777

Expected order of pull-request commits :

  1. sonic-swss common pull request : swss-common: WRED and ECN statistics specific changes sonic-swss-common#777
  2. sonic-swss : pull request : sonic-swss: Code changes for WRED and ECN statistics sonic-swss#2750
  3. sonic-yang-model pull requests : sonic-yang-models: WRED statistics yang sonic-buildimage#14758
  4. sonic-utilities pull request : sonic-utilities: WRED stats feature changes on sonic-utilities sonic-utilities#2807
  5. sonic-sairedis pull request : sonic-sairedis : Wred stats feature changes on Sai-redis and Syncd #1234

This was referenced Apr 25, 2023
Merged
Open
@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch from d8bbe32 to 5da8294 Compare April 25, 2023 16:40
@rpmarvell rpmarvell mentioned this pull request Apr 26, 2023
Merged
10 tasks
@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch from 5da8294 to 838bc2e Compare May 1, 2023 08:06
@kcudnik
Copy link
Collaborator

kcudnik commented May 4, 2023

please fix errors

@rpmarvell
Copy link
Author

This PR is dependent on swss-common PR 777 as i have mentioned in the description. Hence the errors. Could you please review and approve the swss-common PR if possible..! Thank you

msosyak
msosyak previously approved these changes May 8, 2023
View reviewed changes
@rpmarvell rpmarvell mentioned this pull request May 9, 2023
Merged
stephenxs
stephenxs reviewed May 25, 2023
View reviewed changes
lib/ClientSai.cpp Outdated Show resolved Hide resolved
lib/ClientSai.cpp Outdated Show resolved Hide resolved
lib/RedisRemoteSaiInterface.cpp Outdated Show resolved Hide resolved
@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch from 838bc2e to 8d57f14 Compare May 29, 2023 10:15
@kcudnik
Copy link
Collaborator

kcudnik commented May 29, 2023

please fix build errors

@rpmarvell
Copy link
Author

please fix build errors

sonic-sairedis is dependent on sonic-swss-common PR https://github.com/sonic-net/sonic-swss-common/pull/777 .
This sonic-swss-common PR has already been approved by maintainer but we are facing issues in merging this PR as the CI is unstable.
There is an issue raised for this in the SONiC community by Myron Sosyak, Link for this issue : sonic-net/sonic-swss-common#784

@stephenxs
Copy link
Contributor

Hi @rpmarvell
Please refer to my latest comment. I'm not able to unresolve a comment
Thanks

@akokhan
Copy link
Contributor

akokhan commented May 31, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@akokhan
Copy link
Contributor

akokhan commented Jun 2, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@akokhan
Copy link
Contributor

akokhan commented Jun 2, 2023

@rpmarvell , even swss-common changes are merged, sairedis CI is still failing. Please check.

@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch from 8d57f14 to 6b6d297 Compare June 3, 2023 17:37
@rpmarvell
Copy link
Author

Hi @rpmarvell Please refer to my latest comment. I'm not able to unresolve a comment Thanks

I have addressed your review comment. Thank you!

@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch 8 times, most recently from 68b4d42 to 380553f Compare June 4, 2023 10:31
@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch 8 times, most recently from fc7a15e to 77c47d4 Compare May 31, 2024 08:02
@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch 6 times, most recently from 58ada88 to 1628881 Compare June 27, 2024 05:28
@rpmarvell
Copy link
Author

@kcudnik : the vs tests are failing due to the following permission issue, could you help resolve this issue.

sh: 1: ./scripts/pahole-version.sh: Permission denied
init/Kconfig:97: syntax error
init/Kconfig:96: invalid statement
make[1]: *** [scripts/kconfig/Makefile:77: allmodconfig] Error 1
make: *** [Makefile:630: allmodconfig] Error 2

@kcudnik
Copy link
Collaborator

kcudnik commented Jul 24, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch 2 times, most recently from 6ad4575 to e945f69 Compare August 1, 2024 19:44
@kperumalbfn
Copy link

@rpmarvell Could you rebase to master and fix the build issues.

@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch 2 times, most recently from f8ef4db to 295d7b1 Compare August 2, 2024 20:43
kcudnik
kcudnik reviewed Aug 3, 2024
View reviewed changes
meta/Meta.h
@@ -459,10 +459,6 @@ namespace saimeta
_Out_ uint64_t *counters,
_In_ sai_stats_mode_t mode);

sai_status_t meta_validate_query_stats_capability(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is removed ?

kcudnik
kcudnik reviewed Aug 3, 2024
View reviewed changes
vslib/SwitchStateBase.cpp
return SAI_STATUS_BUFFER_OVERFLOW;
}

stats_capability->count = 51;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use std vector with declared values of that and then assign in loop

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have picked only the Ether/If/port in/out and wred stat_enums from the sai_port_stat_t. Hence we need to have unique assignment of these statistics. Please note that i would prefer this way unless it is mandatory to add all the stats in sai_port_stat_t in the capability list.

@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch from 295d7b1 to e684dbc Compare August 8, 2024 06:30
@rpmarvell
sonic-sairedis : Wred stats feature changes on Sai-redis and Syncd
eb9d0fd 
* Stats capability query API support is added
* Marvell VS support for query stats capability added

Signed-off-by: rpmarvell <rperumal@marvell.com>
@rpmarvell rpmarvell force-pushed the rpmarvell_wredstats_sairedis branch from e684dbc to eb9d0fd Compare August 8, 2024 06:49
@kperumalbfn
Copy link

@rpmarvell could you check the code coverage pipeline failure?

@kperumalbfn
Copy link

@rpmarvell Could you take care of code coverage by adding more tests for the changes? We could merge all WRED stats related changes for 202411 release.

image

@kperumalbfn
Copy link

@rpmarvell Could you resolve the conflicts and check the code coverage. We need to merge this PR first before sonic-swss PRs to avoid build breakage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kcudnik kcudnik kcudnik requested changes

@stephenxs stephenxs stephenxs approved these changes

@msosyak msosyak Awaiting requested review from msosyak

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rpmarvell @kcudnik @stephenxs @akokhan @zhangyanzhao @kperumalbfn @msosyak