[pytest] Test the autorestart feature of containers by yozhao101 · Pull Request #1769 · sonic-net/sonic-mgmt

yozhao101 · 2020-06-12T17:08:01Z

Description of PR

We want to test the autorestart feature of five containers swss, syncd, teamd, dhcp_relay and bgp. Specifically we want to test whether the container can be automatically restarted if one of critical process is killed. Currently I tested two test cases manually for swss container. One test case is to kill a critical process portsyncd and another is to kill a non-critical process rsyslogd.

Summary:
Fixes # (issue)

Type of change

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

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

I tested it on a virtual testbed which is installed SONiC image built from public master branch.

Any platform specific information?

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

Documentation

process portsyncd and a non-critical process rsyslogd. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

and use 'show container feature autorestart' to get the containers which were implemented with the autorestart feature. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

critical process and a non-critical process. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

a critical group. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

…critical program. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

need set the autorestart features of containers to 'enabled' if they are in the 'disabled' state and restore this 'disabled' state after testing. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

get_program_state(...) Signed-off-by: Yong Zhao <yozhao@microsoft.com>

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

of command "docker exec <command_name> supervisorctl" not "docker exec <container_name> ps -ax" since the name in critical_processes file is the program name defined in supervisord.file. At the same time, I also add functionality to reset failed flag once the container reaches the limitation of restart times which was set in service file. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

to improve the readability. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

is killed in the specified container. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

since the exit code is the execution result of 'kill' command not the output. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

'is_container_running(...)'. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque · 2020-08-04T21:25:52Z

Looks good to me. @yxieca to review again, please.

yxieca · 2020-08-14T03:45:17Z

tests/autorestart_test/test_container_autorestart.py

+        else:
+            pytest.fail("Syntax of the line '{}' in critical_processes file is incorrect.".format(line))
+
+    return critical_group_list, critical_process_list


What is the difference between this function and https://github.com/Azure/sonic-mgmt/blob/695a9a53b75c90878c8b8de022479a53af1240a0/tests/common/devices.py#L371? can you merge this into a common place?

This can come as another PR.

The difference between these two functions is all_critical_process_status(...) will call the function critical_process_status(...) which first get all the critical process list and then retrieve their running status for each containers in SONiC, get_critical_group_and_process_list(...) will only get all critical process list for a specified container.

@jleveque Can we break the function critical_process_status(...) in common/devices.py into two sub-functions? One function is used to get all critical process list of a container and another function can be created to retrieve the running status of these critical processes.

@yozhao101: Yes. I think splitting the function into two sub-functions should be fine.

I will do this by creating a separate PR.

yxieca · 2020-08-14T03:47:31Z

tests/autorestart_test/test_container_autorestart.py

+        if container_state == "disabled":
+            disabled_containers.append(container_name)
+
+    return disabled_containers


Please use common function https://github.com/Azure/sonic-mgmt/blob/695a9a53b75c90878c8b8de022479a53af1240a0/tests/common/devices.py#L747

FYI, @lguohan removed the CONTAINER_FEATURE table recently and merged that info into the FEATURE table. Thus, this common function needs to be updated to get the new data.

yxieca · 2020-08-14T03:48:34Z

tests/autorestart_test/test_container_autorestart.py

+        if container_state in ["enabled", "disabled"]:
+            container_autorestart_states[container_name] = container_state
+
+    return container_autorestart_states


This function can be moved to common/devices.py as a shared function if there is no such member function yet.

Will moved this function to common/devices.py by creating a separate PR.

tests/autorestart_test/test_container_autorestart.py

yxieca · 2020-08-14T03:53:40Z

tests/autorestart_test/test_container_autorestart.py

+    container_autorestart_states = get_container_autorestart_states(duthost)
+    disabled_containers = get_disabled_container_list(duthost)
+
+    for container_name in container_autorestart_states.keys():


It would better to make container name a parameter, there should be a way to make container list a parameter fixture. So that each container would have a unique test case to mark success or failure.

This change can come as a follow up PR.

yxieca · 2020-08-14T03:54:08Z

tests/autorestart_test/test_container_autorestart.py

+                                                        program_status, program_pid)
+
+        critical_group_list, critical_process_list = get_critical_group_and_process_list(duthost, container_name)
+        for critical_process in critical_process_list:


Same as containers, this could be parameterized.

yxieca

One thing this test misses, is that you should have a clean up method in the end to check if all critical processes are running, if not, then you need to recover the DUT so that next test case will have a clean run. I think you should address this issue, the other issues can come back as follow up PR.

tests/autorestart_test/test_container_autorestart.py

'get_critical_group_and_process_list(...)' into common/devices.py. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

tests/autorestart_test/test_container_autorestart.py

"get_critical_group_and_process_lists(...)" Signed-off-by: Yong Zhao <yozhao@microsoft.com>

#### Why I did it Update submodule pointer for swss to include recent changes 4f1d726 [portsorch] fix errors when moving port from one lag to another. (sonic-net#1797) ae44701 [orchagent] Put port configuration to APPL_DB according to autoneg mode (sonic-net#1769) 5295f91 Add failure handling for SAI get operations (sonic-net#1768) 7c7c451 Revert recirc port change (sonic-net#1813) 5528ebf Cleanup code (sonic-net#1814)

yozhao101 added 2 commits June 12, 2020 09:46

[pytest] Test the autorestart feature of swss container with a critical

64d7221

process portsyncd and a non-critical process rsyslogd. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Change the format of error message for assert statement.

0adc9d7

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

yozhao101 requested review from jleveque, lguohan and yxieca June 12, 2020 17:55

yozhao101 added 2 commits June 12, 2020 10:59

[pytest] Delete a comment line.

fc9dce2

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Delete a comment line.

b5c4c7b

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Jun 12, 2020

View reviewed changes

jleveque added Enhancement Pytest 🐍 labels Jun 12, 2020

jleveque suggested changes Jun 12, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

yozhao101 added 2 commits June 15, 2020 00:15

Add test functions for all containers.

c100e81

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Reorganize the comments.

c393660

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Jun 15, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

jleveque suggested changes Jun 15, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

yozhao101 added 3 commits June 15, 2020 15:02

[pytest] Change the function name to test kill a non-critical process

c179bcf

and use 'show container feature autorestart' to get the containers which were implemented with the autorestart feature. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Delete a blank line.

04575a1

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Test the autorestart feature of containers with a single

8829a01

critical process and a non-critical process. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Jun 18, 2020

View reviewed changes

yozhao101 added 4 commits June 24, 2020 14:36

[pytest] Add a functionality to test the autorestart feature by killing

6ca1fc2

a critical group. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Skip testing the restapi.

b5dafe4

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] For each critical group, currently we should test its first …

e20a7d1

…critical program. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Delete the blank lines.

aa898e9

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Jun 24, 2020

View reviewed changes

yozhao101 added 3 commits June 24, 2020 17:09

[pytest] Add some comments in test_autorestart_container(...). We only

7e8a4cc

need set the autorestart features of containers to 'enabled' if they are in the 'disabled' state and restore this 'disabled' state after testing. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Fix a typo for the comment in the function

4c2210c

get_program_state(...) Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Fix a syntax error in the comment.

545829b

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Jun 29, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

yozhao101 added 2 commits July 2, 2020 15:59

[pytest] Delete the debugging code.

9203139

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

yozhao101 added 3 commits July 31, 2020 15:51

[pytest] Fix the typos and the sequence of imports.

2da480a

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Delete extra blank line and trailing whitespace.

c9817ea

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Fix grammar error.

46f1006

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Aug 2, 2020

View reviewed changes

yozhao101 added 2 commits August 3, 2020 12:03

[pytest] Move the function 'wait_until(...)' out of 'pytest_assert(...)'

b66f8bf

to improve the readability. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[pytest] Use the exit code of 'kill' command to decide whether a process

d38dc2b

is killed in the specified container. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Aug 4, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

[pytest] Use the name 'kill_cmd_result' instead of 'kill_cmd_output'

cea9bb3

since the exit code is the execution result of 'kill' command not the output. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Aug 4, 2020

View reviewed changes

[pytest] Reorganize the log message and simplify the return statement of

66e8212

'is_container_running(...)'. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Aug 4, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

[[pytest] Delete the empty line at the end of file.

a276f48

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Aug 4, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

[pytest] Use 'restore_disabled_state' instead of 'need_restore_state'.

36544d2

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

yxieca reviewed Aug 14, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

yxieca reviewed Aug 14, 2020

View reviewed changes

jleveque suggested changes Aug 14, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

yozhao101 added 2 commits September 9, 2020 13:11

Merge branch 'master' into test_autorestart_feature_containers

4151804

[pytest] Move functions 'get_container_autorestart_states(...)' and

a078d6c

'get_critical_group_and_process_list(...)' into common/devices.py. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque suggested changes Sep 9, 2020

View reviewed changes

tests/autorestart_test/test_container_autorestart.py Outdated Show resolved Hide resolved

[pytest] Add an "s" to the end of function name

357a70e

"get_critical_group_and_process_lists(...)" Signed-off-by: Yong Zhao <yozhao@microsoft.com>

jleveque approved these changes Sep 14, 2020

View reviewed changes

yxieca merged commit 5d40492 into sonic-net:master Sep 14, 2020

Conversation

yozhao101 commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Approach

What is the motivation for this PR?

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jleveque commented Aug 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yozhao101 Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yozhao101 commented Jun 12, 2020 •

edited

Loading

yozhao101 Sep 8, 2020 •

edited

Loading

yxieca left a comment •

edited

Loading