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

Adding support to get safe_reload and check_intf_up_ports #15762

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

Conversation

vperumal
Copy link
Contributor

Description of PR

We had support for time and wait in plt_reboot_dict, Adding support to fetch safe reload and check_intf_up_status. With these parameters we can provide knobs in lab file for which tests vendors want to check critical process status and interface status. This will be particular to their profile and devices, instead of being global to all

For e.g.

plt_reboot_dict:
cold:
timeout: 450
wait: 360
safe_reboot: True
check_intf_up_ports: True
watchdog:
timeout: 300
wait: 360
safe_reboot: True
check_intf_up_ports: True
acl/test_acl.py::TestAclWithReboot:
timeout: 300
wait: 600
safe_reboot: True
check_intf_up_ports: True
platform_tests/test_reload_config.py::test_reload_configuration_checks:
timeout: 300
wait: 60
platform_tests/test_kdump.py::TestKernelPanic::test_kernel_panic:
timeout: 300
wait: 360
safe_reboot: True
check_intf_up_ports: True

Summary:
Fixes # (issue)

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?

In some testcases we want to have a provision to check container status and interface status after a reboot.

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

@vperumal
Copy link
Contributor Author

FYI @abdosi @yejianquan

@@ -254,6 +254,8 @@ def reboot(duthost, localhost, reboot_type='cold', delay=10,
# get 'wait' and 'timeout' from inventory if they are specified, otherwise use current values
wait = plt_reboot_ctrl.get('wait', wait)
timeout = plt_reboot_ctrl.get('timeout', timeout)
safe_reboot = plt_reboot_ctrl.get('safe_reboot', safe_reboot)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep using safe_reboot with explicitly parameters, the plt_reboot configuration generally is locally, if we set safe_reboot in configuration, it would cause the differentiates being huge on different platforms.
Any reason we have to set safe_reboot in configuration file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @yejianquan, It is used only in places where in the intent is to make sure that the system is fully up before continuing the test. For e.g. in acl, we do a reboot and give a larger wait time for critical process and interfaces to come up and then continue with the tests. The only place that I know of where we don't want to check if critical process is not up and interface is not up is test_reload_config.py because there we want to check almost immediately after reboot, certain conditions. But in all cases we would want to ensure after reboot everything is up. Hence I added that option for the user

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vperumal Yeah, that's what I supposed,
then let's add safe_reboot=true on the places that call the reboot:
reboot(...., safe_reboot=True)
So that the behaviors keep the same even with different device configurations.

The safe_reboot was an enhanced option.
During the writing of the test cases, it was possible that we don't have safe_reboot option so we didn't use it.
Let's enhance the out-of-date usage of reboot.

@rlhui
Copy link

rlhui commented Nov 27, 2024

@vperumal - as discussed in wg, let's create an issue first.

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