Skip to content

[syncd] Move logSet logGet under mutex to prevent race condition #1505

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
Merged
yejianquan merged 8 commits into from
Feb 6, 2025
Merged

[syncd] Move logSet logGet under mutex to prevent race condition #1505

yejianquan merged 8 commits into from
Feb 6, 2025

Conversation

@kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Jan 22, 2025 •
edited
Loading

Fixes: sonic-net/sonic-buildimage#21180

Mutex is added to protect m_logLevelMap when doing logSet from multiple thread

@kcudnik kcudnik added the Bug label Jan 22, 2025
@mssonicbld
Copy link
Collaborator

mssonicbld commented Jan 22, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 22, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@kcudnik kcudnik mentioned this pull request Jan 22, 2025
Closed
@rlhui rlhui requested a review from abdosi January 22, 2025 18:11
@bingwang-ms
Copy link
Contributor

bingwang-ms commented Jan 22, 2025

@kcudnik Can you raise a PR to 202405 to address conflict?

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 22, 2025

@kcudnik Can you raise a PR to 202405 to address conflict?

after this will pass

@stephenxs
Copy link
Contributor

stephenxs commented Jan 22, 2025

@kcudnik Would you please elaborate on what issue you saw without the fix? We saw an issue which I'm not sure whether it's the same

@stephenxs
Copy link
Contributor

stephenxs commented Jan 22, 2025

@kcudnik Would you please elaborate on what issue you saw without the fix? We saw an issue which I'm not sure whether it's the same

Saw the referenced ticket:)

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 23, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 23, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 23, 2025

arm fail becaue of syslog pipeline, not related to PR

@abdosi
Copy link
Contributor

abdosi commented Jan 25, 2025

/azp run Azure.sonic-sairedis

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 25, 2025

Azure Pipelines successfully started running 1 pipeline(s).

abdosi
abdosi previously approved these changes Jan 25, 2025
View reviewed changes
@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 25, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 25, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

mssonicbld commented Jan 27, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 27, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@abdosi
Copy link
Contributor

abdosi commented Jan 29, 2025

@kcudnik : can you create PR for 202405 ?

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 2, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@yejianquan
Copy link
Contributor

yejianquan commented Feb 5, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 5, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@yejianquan
Copy link
Contributor

yejianquan commented Feb 5, 2025

Hi @kcudnik , the PR checking is reporting out-of-date, could you rebase to the newest master branch?

@yejianquan yejianquan mentioned this pull request Feb 5, 2025
Open
@kcudnik
Copy link
Collaborator Author

kcudnik commented Feb 5, 2025

also for 202405: #1520

@mssonicbld
Copy link
Collaborator

mssonicbld commented Feb 5, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 5, 2025

Azure Pipelines successfully started running 1 pipeline(s).

yejianquan
yejianquan approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@yejianquan yejianquan left a comment

Choose a reason for hiding this comment

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

LGTM

@mssonicbld
Copy link
Collaborator

mssonicbld commented Feb 5, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 5, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@yejianquan yejianquan merged commit b664f08 into sonic-net:master Feb 6, 2025
15 checks passed
@kcudnik kcudnik deleted the fix branch February 6, 2025 07:51
@dprital
Copy link
Collaborator

dprital commented Feb 11, 2025

@kcudnik , Do you have dedicate PR for 202411 as this one got conflicts ?

@kcudnik
Copy link
Collaborator Author

kcudnik commented Feb 11, 2025

#1520 here is for 202405 if you need for 11,maybe i could be cherrypicked from 202405 without conflicts

@dprital
Copy link
Collaborator

dprital commented Feb 12, 2025

#1520 here is for 202405 if you need for 11,maybe i could be cherrypicked from 202405 without conflicts

Thanks, please do.

@kperumalbfn
Copy link

kperumalbfn commented Feb 19, 2025

@kcudnik please let me know when you have a PR for 202411

@kcudnik
Copy link
Collaborator Author

kcudnik commented Feb 19, 2025

@kcudnik please let me know when you have a PR for 202411

here: #1538

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

Reviewers

@yejianquan yejianquan yejianquan approved these changes

@abdosi abdosi abdosi left review comments

Assignees

@kcudnik kcudnik

Labels

Bug Cherry Pick Conflict_202405 Cherry Pick Conflict_202411 Included in 202411 Branch Request for 202405 Branch Request for 202411 Branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

syncd crash in syncd::VendorSai::logSet() during docker startup

9 participants

@kcudnik @mssonicbld @bingwang-ms @stephenxs @abdosi @yejianquan @dprital @kperumalbfn @rlhui