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

Fix issue: bulk counter feature cannot compile on platforms having no sai_bulk_object_get_stats/sai_bulk_object_clear_stats #1105

Merged
merged 3 commits into from
Sep 6, 2022
Merged

Fix issue: bulk counter feature cannot compile on platforms having no sai_bulk_object_get_stats/sai_bulk_object_clear_stats #1105

merged 3 commits into from
Sep 6, 2022

Conversation

Junchao-Mellanox
Copy link
Contributor

Why I did this?

bulk counter feature uses two new SAI API sai_bulk_object_get_stats/sai_bulk_object_clear_stats. Some vendors have no such API implemented in their SAI code. Need a way to make compilation pass.

How I fix this?

  1. Add AC_CHECK_FUNCS for sai_bulk_object_get_stats/sai_bulk_object_clear_stats which generates macro HAVE_SAI_BULK_OBJECT_CLEAR_STATS/HAVE_SAI_BULK_OBJECT_GET_STATS in config.h.
  2. Use ifdef to avoid compilation failure.

@Junchao-Mellanox
Fix issue: bulk counter feature cannot compile on platforms having no…
1a77240 
… sai_bulk_object_get_stats/sai_bulk_object_clear_stats
jimmyzhai
jimmyzhai previously approved these changes Aug 17, 2022
View reviewed changes
@jimmyzhai jimmyzhai requested a review from kcudnik August 17, 2022 09:53
@Junchao-Mellanox
Copy link
Contributor Author

/azpw run Azure.sonic-sairedis

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Contributor Author

Hi @kcudnik , can we ignore the coverage checker? I don't think we can cover that.

@kcudnik
Copy link
Collaborator

kcudnik commented Aug 22, 2022

no we can't merge without coverage fail, since it will show up on monthly report and i will get into trouble based on that, this test must pass

@kcudnik
Copy link
Collaborator

kcudnik commented Aug 22, 2022

why you need to add ifdef for those stats ?

@Junchao-Mellanox
Copy link
Contributor Author

why you need to add ifdef for those stats ?

Some vendors do not have sai_bulk_object_get_stats and sai_bulk_object_clear_stats, it would cause compile issue. So, I have to use ifdef to make those vendor pass the compilation. Could you suggest a way to cover the #ifdef?

@kcudnik
Copy link
Collaborator

kcudnik commented Aug 23, 2022

Some vendors do not have sai_bulk_object_get_stats and sai_bulk_object_clear_stats, it would cause compile issue. So, I have to use ifdef to make those vendor pass the compilation. Could you suggest a way to cover the #ifdef?

but why this is vendor specific? you are adding this in virutal switch not for specific vendor are you?
if you want to test specific fidef, then extra tests would need to be added for example in unitests/vslib/ and new executable would need to be added where you specify extra defines to c compiler

@Junchao-Mellanox
Copy link
Contributor Author

/azpw run Azure.sonic-sairedis

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Contributor Author

/azpw run Azure.sonic-sairedis

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Contributor Author

/azpw run Azure.sonic-sairedis

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Contributor Author

/azpw run Azure.sonic-sairedis

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

kcudnik
kcudnik previously approved these changes Aug 31, 2022
View reviewed changes
@Junchao-Mellanox
Copy link
Contributor Author

/azpw run Azure.sonic-sairedis

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@Junchao-Mellanox could you please check the LGTM failure?

@Junchao-Mellanox
Copy link
Contributor Author

@Junchao-Mellanox could you please check the LGTM failure?

Not an issue introduced by this PR, I have sent email to sonic admin add cc'ed you.

@kcudnik
Copy link
Collaborator

kcudnik commented Sep 5, 2022 •
edited
Loading

its probably missing library for swss-common on lgtm container build docker needs to be updated

In file included from events.cpp:1:
[2022-08-25 05:47:25] [build-stderr] events_pi.h:10:10: fatal error: uuid/uuid.h: No such file or directory
[2022-08-25 05:47:25] [build-stderr]    10 | #include <uuid/uuid.h>

the missing lib is uuid-dev lib, which was added in swss-common in this PR:

https://github.com/sonic-net/sonic-swss-common/pull/667/files

this lib would need to be added to this repo lgtm.yml

probably "uuid" and "uuid-dev" packages will be needed together

I added PR for that : #1119

@kcudnik
Copy link
Collaborator

kcudnik commented Sep 6, 2022

i merged my fix, so if you want to retrigger you would need to submit some dummy code change since re-run from is not working somehow

@Junchao-Mellanox
Copy link
Contributor Author

Thanks, will re-trigger

@kcudnik kcudnik merged commit 8140c22 into sonic-net:master Sep 6, 2022
@Junchao-Mellanox Junchao-Mellanox deleted the add-bulk-macro branch September 7, 2022 03:30
@mlorrillere mlorrillere mentioned this pull request Sep 7, 2022
Closed
mlorrillere added a commit to mlorrillere/sonic-buildimage that referenced this pull request Sep 8, 2022
@mlorrillere
[submodule] update sonic-sairedis
d0bf948 
* 660a920 [Chassis] Create fabric ports for switch_type fabric. (sonic-net/sonic-sairedis#1114)
* 8140c22 Fix issue: bulk counter feature cannot compile on platforms having no sai_bulk_object_get_stats/sai_bulk_object_clear_stats (sonic-net/sonic-sairedis#1105)
* 0aa60f5 [lgtm] Add uuid library (sonic-net/sonic-sairedis#1119)
* e8a01a8 Add retry on zmq functions if fail with EINTR. (sonic-net/sonic-sairedis#1109)
* 594b242 Add SAI_PORT_ATTR_OPER_SPEED support (sonic-net/sonic-sairedis#1107)
* 4c9e048 Add Xsight specific syncd start options (sonic-net/sonic-sairedis#1112)
* da26ace Run 20 vs tests at a time. (sonic-net/sonic-sairedis#1111)
* ffc4109 [asan] suppress the static variable leaks (sonic-net/sonic-sairedis#1085)
* bfd37e3 [sonic-sairedis] Support bulk counter (sonic-net/sonic-sairedis#1094)
* 90ba09a Transfer organization from Azure to sonic-net (sonic-net/sonic-sairedis#1095)
* 4853881 [BFN] Provide unified approach to select P4 profile based on chip family (sonic-net/sonic-sairedis#1089)
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
@Junchao-Mellanox
Fix issue: bulk counter feature cannot compile on platforms having no…
5582b11 
… sai_bulk_object_get_stats/sai_bulk_object_clear_stats (sonic-net#1105)

Why I did this?

bulk counter feature uses two new SAI API sai_bulk_object_get_stats/sai_bulk_object_clear_stats. Some vendors have no such API implemented in their SAI code. Need a way to make compilation pass.

How I fix this?

Add AC_CHECK_FUNCS for sai_bulk_object_get_stats/sai_bulk_object_clear_stats which generates macro HAVE_SAI_BULK_OBJECT_CLEAR_STATS/HAVE_SAI_BULK_OBJECT_GET_STATS in config.h.
Use ifdef to avoid compilation failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kcudnik kcudnik kcudnik approved these changes

@jimmyzhai jimmyzhai jimmyzhai left review comments

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

Successfully merging this pull request may close these issues.
5 participants
@Junchao-Mellanox @mssonicbld @kcudnik @liat-grozovik @jimmyzhai