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

fix for voq fabric related tests #16068

Merged
merged 4 commits into from
Dec 30, 2024

Conversation

rawal01
Copy link
Contributor

@rawal01 rawal01 commented Dec 13, 2024

Description of PR

Summary:
Fixes # 16067
#16067

Type of change

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

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • [X ] 202405

Approach

What is the motivation for this PR?

Fixing issue 16067.

How did you do it?

Changed to not re-write originalIsolateStatus but instead write to isolateStatus.
Also added try/catch block to both tests as today if the test fails it exits without restoring the state of the port.

I also added check to tests/voq/test_fabric_reach.py::test_fabric_reach_supervisor to run against only if asic is present. Right not the tests get the max asics for the platform and assumes they are all present.

How did you verify/test it?

Ran against t2 Nokia chassis

Any platform specific information?

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

Documentation

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rawal01
Copy link
Contributor Author

rawal01 commented Dec 16, 2024

@mssonicbld I checked the test failed because of host unreachable in posttest not related to any changes I made please rerun

@arlakshm
Copy link
Contributor

@jfeng-arista, can you review this change

@rawal01 rawal01 force-pushed the fix_enhance_voq_fabric branch from 10cbc86 to 9265ef8 Compare December 18, 2024 21:32
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@kenneth-arista kenneth-arista left a comment

Choose a reason for hiding this comment

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

Besides the minor comments, this change looks good to me.

@rawal01 rawal01 force-pushed the fix_enhance_voq_fabric branch from 85bae55 to a2bbe4d Compare December 19, 2024 14:42
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rawal01
Copy link
Contributor Author

rawal01 commented Dec 19, 2024

@arlakshm can you help merge this PR ?

@jfeng-arista
Copy link
Contributor

jfeng-arista commented Dec 20, 2024

The change looks good to me, also quickly tested on one of our systems and they are running ok. We can merge this up after Kenneth's comments get addressed.

@rawal01
Copy link
Contributor Author

rawal01 commented Dec 20, 2024

The change looks good to me, also quickly tested on one of our systems and they are running ok. We can merge this up after Kenneth's comments get addressed.

@jfeng-arista I already addressed all comments in latest commit please check

@arlakshm arlakshm merged commit 1574885 into sonic-net:master Dec 30, 2024
16 checks passed
yaqiangz pushed a commit to yaqiangz/sonic-mgmt that referenced this pull request Dec 31, 2024
What is the motivation for this PR?
Fixing issue 16067.

How did you do it?
Changed to not re-write originalIsolateStatus but instead write to isolateStatus.
Also added try/catch block to both tests as today if the test fails it exits without restoring the state of the port.

I also added check to tests/voq/test_fabric_reach.py::test_fabric_reach_supervisor to run against only if asic is present. Right not the tests get the max asics for the platform and assumes they are all present.

How did you verify/test it?
Ran against t2 Nokia chassis
@rawal01 rawal01 deleted the fix_enhance_voq_fabric branch January 3, 2025 17:30
@rlhui
Copy link

rlhui commented Jan 8, 2025

@bingwang-ms please approve? thanks.

mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Jan 8, 2025
What is the motivation for this PR?
Fixing issue 16067.

How did you do it?
Changed to not re-write originalIsolateStatus but instead write to isolateStatus.
Also added try/catch block to both tests as today if the test fails it exits without restoring the state of the port.

I also added check to tests/voq/test_fabric_reach.py::test_fabric_reach_supervisor to run against only if asic is present. Right not the tests get the max asics for the platform and assumes they are all present.

How did you verify/test it?
Ran against t2 Nokia chassis
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202405: #16393

@jfeng-arista
Copy link
Contributor

The change looks good to me, also quickly tested on one of our systems and they are running ok. We can merge this up after Kenneth's comments get addressed.

@jfeng-arista I already addressed all comments in latest commit please check

great ! thank you

mssonicbld pushed a commit that referenced this pull request Jan 30, 2025
What is the motivation for this PR?
Fixing issue 16067.

How did you do it?
Changed to not re-write originalIsolateStatus but instead write to isolateStatus.
Also added try/catch block to both tests as today if the test fails it exits without restoring the state of the port.

I also added check to tests/voq/test_fabric_reach.py::test_fabric_reach_supervisor to run against only if asic is present. Right not the tests get the max asics for the platform and assumes they are all present.

How did you verify/test it?
Ran against t2 Nokia chassis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants