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.

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

[Mclagsyncd] Ignore select event if its return value is error. #2776

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Minkang-Tsai
Copy link
Contributor

What I did
Ignore select event if its return value is error.

Why I did it
In order to prevent the mclagsyncd terminated.

@Minkang-Tsai Minkang-Tsai requested a review from prsunny as a code owner May 15, 2023 07:58
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 15, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Minkang-Tsai (489fc92)
  • ✅ login: prsunny / name: Prince Sunny (78707dd)

@Minkang-Tsai
Copy link
Contributor Author

/easycla

@Minkang-Tsai
Copy link
Contributor Author

/easycla

@prsunny prsunny requested a review from gechiang September 18, 2023 22:03
@gechiang
Copy link
Contributor

@Minkang-Tsai ,
I have a few asks for you:
1, Is there an issue filed for this issue that you are trying to fix? If so, can you link it to this PR?
2. Can you add Unit test testcase for this change. I don't see any test code added/changed to cover your changes.

@Praveen-Brcm can you please help review this?
Thanks!


if (ret == Select::ERROR)
{
SWSS_LOG_NOTICE("Error: %s!", strerror(errno));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is error case, should the log level be elevated to WARNING or ERROR?

@Minkang-Tsai
Copy link
Contributor Author

Minkang-Tsai commented Sep 27, 2023

@Minkang-Tsai , I have a few asks for you: 1, Is there an issue filed for this issue that you are trying to fix? If so, can you link it to this PR? 2. Can you add Unit test testcase for this change. I don't see any test code added/changed to cover your changes.

@Praveen-Brcm can you please help review this? Thanks!

Hi @gechiang
We try to kill docker database directly. Under this situation, select return value in mclagsyncd is ERROR, but it can continue to do the following steps to possibly cause the segment fault. Do you think that we need to create a PR to record that issue?

@gechiang
Copy link
Contributor

@Minkang-Tsai , I have a few asks for you: 1, Is there an issue filed for this issue that you are trying to fix? If so, can you link it to this PR? 2. Can you add Unit test testcase for this change. I don't see any test code added/changed to cover your changes.
@Praveen-Brcm can you please help review this? Thanks!

Hi @gechiang Our customer try to kill docker database directly. Under this situation, select return value in mclagsyncd is ERROR, but it can continue to do the following steps to possibly cause the segment fault. Do you think that we need to create a PR to record that issue?

You have two choices:

  1. Raise a GIT hub issue under sonic-net/sonic-buildimage for what your customer tried and describes the steps, and what was observed, and what is the desired outcome after the trigger was made. and share the issue link in this PR.
  2. Fully describe the issue as I described above without filing a git hub issue in this PR description section.

Reason for this ask is we need to clearly describe the scenario involved so it helps the reviewer to see what is happening and what your changes will help to resolve the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants