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

[test gap] Add a new test case to verify lldp entry after restart syncd and orchagent #15911

Merged
merged 6 commits into from
Dec 18, 2024
Merged

[test gap] Add a new test case to verify lldp entry after restart syncd and orchagent #15911

merged 6 commits into from
Dec 18, 2024

Conversation

ZhaohuiS
Copy link
Contributor

@ZhaohuiS ZhaohuiS commented Dec 5, 2024

…cd and orchagent

Description of PR

Summary:
Fixes # (issue)
Address #15889

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

Approach

What is the motivation for this PR?

Add a new test case to address #15889

How did you do it?

Restart syncd and orchagent, then check lldp entries in APPL_DB

How did you verify/test it?

run test_lldp_syncd.py

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@ZhaohuiS
[test gap] Add a new test case to verify lldp entry after restart syn…
b46a1ce 
…cd and orchagent

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@ZhaohuiS ZhaohuiS changed the title [test gap] Add a new test case to verify lldp entry after restart syn… [test gap] Add a new test case to verify lldp entry after restart syncd and orchagent Dec 6, 2024
@ZhaohuiS
Modify wait untill check logic
9f997ed 
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@ZhaohuiS
skip test_lldp_entry_table_after_syncd_orchagent for vs testbed
040ee85 
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@ZhaohuiS ZhaohuiS requested a review from Copilot December 6, 2024 13:41
Copilot
Copilot AI reviewed Dec 6, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.
@ZhaohuiS
Copy link
Contributor Author

ZhaohuiS commented Dec 7, 2024

Hold on the merge until sonic-net/sonic-buildimage#21065 is merged, which is the image fix.

@ZhaohuiS ZhaohuiS requested a review from StormLiangMS December 9, 2024 01:55
@ZhaohuiS
Copy link
Contributor Author

ZhaohuiS commented Dec 9, 2024

@StormLiangMS Since image fix was merged in sonic-net/sonic-buildimage#21065, could you please review and approve this test gap PR?

@ZhaohuiS
address comment and move same code to the common function
0b195ef 
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Pull request contains merge conflicts.

@ZhaohuiS ZhaohuiS force-pushed the fix/lldp_test_gap_repopulate branch from 0b195ef to 3264234 Compare December 16, 2024 07:30
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ZhaohuiS
Fix wrong parameter
89d0237 
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@ZhaohuiS ZhaohuiS force-pushed the fix/lldp_test_gap_repopulate branch from 3264234 to 89d0237 Compare December 17, 2024 03:23
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Pull request contains merge conflicts.

@ZhaohuiS
solve conflict
12859e9 
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

StormLiangMS
StormLiangMS approved these changes Dec 18, 2024
View reviewed changes
Copy link
Collaborator

@StormLiangMS StormLiangMS left a comment

Choose a reason for hiding this comment

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

LGTM

@StormLiangMS StormLiangMS merged commit 9450127 into sonic-net:master Dec 18, 2024
16 checks passed
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Dec 18, 2024
@ZhaohuiS @mssonicbld
[test gap] Add a new test case to verify lldp entry after restart syn…
a9df857 
…cd and orchagent (sonic-net#15911)

What is the motivation for this PR?
Add a new test case to address sonic-net#15889

How did you do it?
Restart syncd and orchagent, then check lldp entries in APPL_DB

How did you verify/test it?
run test_lldp_syncd.py
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Dec 19, 2024
@ZhaohuiS @mssonicbld
[test gap] Add a new test case to verify lldp entry after restart syn…
dbb5db9 
…cd and orchagent (sonic-net#15911)

What is the motivation for this PR?
Add a new test case to address sonic-net#15889

How did you do it?
Restart syncd and orchagent, then check lldp entries in APPL_DB

How did you verify/test it?
run test_lldp_syncd.py
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202411: #16131

@mssonicbld mssonicbld mentioned this pull request Dec 19, 2024
Merged
8 tasks
mssonicbld pushed a commit that referenced this pull request Dec 19, 2024
@ZhaohuiS @mssonicbld
[test gap] Add a new test case to verify lldp entry after restart syn…
ca7816a 
…cd and orchagent (#15911)

What is the motivation for this PR?
Add a new test case to address #15889

How did you do it?
Restart syncd and orchagent, then check lldp entries in APPL_DB

How did you verify/test it?
run test_lldp_syncd.py
@cyw233
Copy link
Contributor

cyw233 commented Dec 26, 2024

Hey @ZhaohuiS, have we confirmed this change is compatible with multi-asic devices (e.g. T2), please? From my observation, the docker restart syncd or docker restart swss command might have issues on multi-asic devices because these devices are having containers like syncd0, syncd1, swss0, swss1...

@ZhaohuiS
Copy link
Contributor Author

Hey @ZhaohuiS, have we confirmed this change is compatible with multi-asic devices (e.g. T2), please? From my observation, the docker restart syncd or docker restart swss command might have issues on multi-asic devices because these devices are having containers like syncd0, syncd1, swss0, swss1...

Sorry, no, I don't have resource to verify it on T2, if there is some issue on T2, please help fix it.

@cyw233
Copy link
Contributor

cyw233 commented Jan 2, 2025

Hey @ZhaohuiS, have we confirmed this change is compatible with multi-asic devices (e.g. T2), please? From my observation, the docker restart syncd or docker restart swss command might have issues on multi-asic devices because these devices are having containers like syncd0, syncd1, swss0, swss1...

Sorry, no, I don't have resource to verify it on T2, if there is some issue on T2, please help fix it.

No worries, Microsoft ADO 30648046 created for this.

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

Copilot code review Copilot Copilot left review comments

@StormLiangMS StormLiangMS StormLiangMS approved these changes

Assignees
No one assigned
Labels
Approved for 202411 branch Included in 202411 branch Request for 202411 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ZhaohuiS @mssonicbld @cyw233 @StormLiangMS @liushilongbuaa