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

Add libsai RedisInterface for link event damping. #1331

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

Add libsai RedisInterface for link event damping. #1331

wants to merge 1 commit into from

Conversation

Ashish1805
Copy link
Contributor

  • This supports the link event damping config and algorithm set API.
  • Sends the link event damping config from libsai to syncd main thread.

HLD: sonic-net/SONiC#1071

@Ashish1805 Ashish1805 mentioned this pull request Dec 7, 2023
Merged
@Ashish1805
Copy link
Contributor Author

@Junchao-Mellanox for review.

@Ashish1805
Copy link
Contributor Author

@kcudnik looks like swss pytest is failing - it seems unrelated to my changes. Any idea how to fix the swss pytest?

@kcudnik
Copy link
Collaborator

kcudnik commented Dec 13, 2023

those vstests are sometime flaky so dont worry about that

@Ashish1805
Copy link
Contributor Author

@kcudnik I have updated the PR as per review comment. can you please review it when you get a chance? Thank you.

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 16, 2024

please fix build erros and code coverage

@Ashish1805 Ashish1805 force-pushed the ledamping_sairedis branch 2 times, most recently from 2399261 to ec5cf30 Compare May 10, 2024 23:02
@Ashish1805
Copy link
Contributor Author

@kcudnik Added the code coverage but I see PR is failing for 2 tests during Update rsyslog.conf test. The test failure log doesn't give much info, can you suggest where can I find the helpful debug log to figure out what exactly failed and why?

@kcudnik
Copy link
Collaborator

kcudnik commented May 13, 2024

@kcudnik Added the code coverage but I see PR is failing for 2 tests during Update rsyslog.conf test. The test failure log doesn't give much info, can you suggest where can I find the helpful debug log to figure out what exactly failed and why?

rsyslogd: pidfile '/run/rsyslogd.pid' and pid 18837 already exist.
If you want to run multiple instances of rsyslog, you need to specify

in ~/sonic-sairedis/.azure-pipelines/build-template.yml

68   - script: |
169       sudo cp azsyslog.conf /etc/rsyslog.conf
170       sudo service rsyslog restart
171     displayName: "Update rsyslog.conf"

seems like syslog service restart failed but why, no clue, we would need to look into syslog on pipeline but i dont know how :D maybe update buitld template to do "tail -n 100 /var/log/syslog ? after restaret command ?

@Ashish1805 Ashish1805 force-pushed the ledamping_sairedis branch 2 times, most recently from 6aa7b5a to 5dfe7bf Compare May 20, 2024 23:59
@Ashish1805
Copy link
Contributor Author

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 1331 in repo sonic-net/sonic-sairedis

@kcudnik
Copy link
Collaborator

kcudnik commented May 22, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Add libsai RedisInterface for link event damping.
6f5b983 
- This supports the link event damping config and algorithm set API.
- Sends the link event damping config from libsai to syncd main thread.

HLD: sonic-net/SONiC#1071

Signed-off-by: Ashish Singh <[email protected]>
@Ashish1805 Ashish1805 force-pushed the ledamping_sairedis branch from 5dfe7bf to 6f5b983 Compare May 28, 2024 20:01
kcudnik
kcudnik reviewed May 28, 2024
View reviewed changes
lib/RedisRemoteSaiInterface.cpp
Comment on lines +1883 to +1888
if ((objectType != SAI_OBJECT_TYPE_PORT) || (attr == nullptr) || (attr->id < SAI_PORT_ATTR_CUSTOM_RANGE_START))
{
return false;
}

return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can turn this to 1 single line by reversing condition

kcudnik
kcudnik reviewed May 28, 2024
View reviewed changes
lib/RedisRemoteSaiInterface.cpp
Comment on lines +507 to +515
if (m_syncMode)
{
swss::KeyOpFieldsValuesTuple kco;
auto status = m_communicationChannel->wait(REDIS_ASIC_STATE_COMMAND_DAMPING_CONFIG_SET, kco);

m_recorder->recordGenericSetResponse(status);

return status;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

make this api always synchronous, async mode is just for create/remove/set operations all other apis should be synchronous

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

@kcudnik kcudnik kcudnik left review comments

At least 1 approving review is required 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.
2 participants
@Ashish1805 @kcudnik