[pmon daemon regression test] Correct the log messages with right daemon name and fix term_and_start_status failure #3922

sujinmkang · 2021-07-30T20:41:58Z

Description of PR

Summary:
Fixes #(sonic-net/sonic-buildimage#8239)

Type of change

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

Back port request

201911
202012

Approach

What is the motivation for this PR?

Needs to fix pmon daemon regression test issues

How did you do it?

Use daemon_name instead of the specific daemon name for log message
Support for both old/new db key support for checking the db value.
Skip term_and_start_status test cases for 201911 and 201811 since it behaves differently
Fix the expected daemon status after sending sig_term based on latest daemon behavior for term_and_start_status
Remove disable_and_enable_autorestart to make sure the daemon regression test doesn't affect any other containers.

How did you verify/test it?

Run test_ledd.py on arista 7050 platform which reports issue.

Any platform specific information?

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

Documentation

unexpected exit

lguohan · 2021-08-04T08:25:05Z

can you update your pr description and title to reflect the update?

tests/platform_tests/daemon/test_pcied.py

antonptashnik · 2021-08-13T14:46:10Z

@sujinmkang could you please specify why is it needed to disable all features' autostart before running these tests ?

sujinmkang · 2021-08-13T17:13:58Z

@antonptashnik originally I don't want to make the PMON container restart during this test since this regression test of some daemons in the critical_process can make the restart the PMON container. I just want to focus on the daemon behavior when it has its running status change. Do you have any concern or suggestion?

antonptashnik · 2021-08-16T11:37:31Z

@antonptashnik originally I don't want to make the PMON container restart during this test since this regression test of some daemons in the critical_process can make the restart the PMON container. I just want to focus on the daemon behavior when it has its running status change. Do you have any concern or suggestion?

So you say some of these tests can cause PMON to restart and for the tests to notice this moment you disable PMON autorestart for it to stay stopped, right ? If so I understand why we disable autorestart for PMON but I suppose autorestart for other features can be left as is

since we want to make sure that pmon and any other containers are not restarted during this test.

prgeor · 2021-08-18T15:13:56Z

tests/platform_tests/daemon/test_ledd.py

@@ -145,26 +113,27 @@ def test_pmon_ledd_term_and_start_status(check_daemon_status, duthosts, rand_one
    @summary: This test case is to check the ledd terminated and restarted status
    """
    duthost = duthosts[rand_one_dut_hostname]
+
+    if "201811" in duthost.os_version or "201911" in duthost.os_version:


@sujinmkang what is the difference in behavior for SIGTERM across different version?

@prgeor master and 202012 daemon has the signal_handler implemented to return the exitcode (128 + signal_number) but previous version just return exit code 0 for sigterm.

Do you really want to skip this test for 201811 and 201911? If not, it would be better we handle them differently here or make the exitcode same across 201811, 201911. What about 202012?

@prgeor this change was based on the discussion during 202012 issue triage. If there would be any back port happening, then we will need another changes for this.

prgeor · 2021-08-18T15:19:05Z

tests/platform_tests/daemon/test_pcied.py

-                          "Pcied expected terminated status is {} but is {}".format(expected_exited_status, daemon_status))
-    pytest_assert(daemon_pid == -1,
-                          "Pcied expected pid is -1 but is {}".format(daemon_pid))
+    logger.info("{} daemon got killed unexpectedly and it is in {} with pid {}".format(daemon_name, daemon_status, daemon_pid))


Why are you logging the info without checking the status if the daemon actually killed or not?

The daemon_status check is followed right after but this is just added for debugging purpose. I can remove this part.

prgeor · 2021-08-18T15:20:30Z

tests/platform_tests/daemon/test_ledd.py

@@ -180,16 +149,17 @@ def test_pmon_ledd_kill_and_start_status(check_daemon_status, duthosts, rand_one
    duthost.stop_pmon_daemon(daemon_name, SIG_KILL, pre_daemon_pid)

    daemon_status, daemon_pid = duthost.get_pmon_daemon_status(daemon_name)
-    logger.info("{} daemon got killed unexpectedly and it is {} with pid {}".format(daemon_name, daemon_status, daemon_pid))
+    logger.info("{} daemon got killed unexpectedly and it is in {} with pid {}".format(daemon_name, daemon_status, daemon_pid))


check the daemon_status before logging "daemon got killed"

The daemon_status check is followed right after but this is just added for debugging purpose. I can remove this part.

better put a log "sending SIG_KILL to daemon {}" before calling stop_pmon_daemon() for debugging and remove this

The test name(test_pmon_ledd_kill_and_start_status) is already indicating this is sending SIG_KILL command.

prgeor · 2021-08-18T15:25:58Z

tests/platform_tests/daemon/test_pcied.py

@@ -154,26 +121,27 @@ def test_pmon_pcied_term_and_start_status(check_daemon_status, duthosts, rand_on
    @summary: This test case is to check the pcied terminated and restarted status
    """
    duthost = duthosts[rand_one_dut_hostname]
+
+    if "201811" in duthost.os_version or "201911" in duthost.os_version:


SIGTERM signal is standard signal and the behavior should be same for all POSIX compliant systems. Can you elaborate what different behavior are you seeing across different os version?

@prgeor please see the previous comment.

…mon name and fix term_and_start_status failure (sonic-net#3922) What is the motivation for this PR? Needs to fix pmon daemon regression test issues How did you do it? Use daemon_name instead of the specific daemon name for log message Support for both old/new db key support for checking the db value. Skip term_and_start_status test cases for 201911 and 201811 since it behaves differently Fix the expected daemon status after sending sig_term based on latest daemon behavior for term_and_start_status Remove disable_and_enable_autorestart to make sure the daemon regression test doesn't affect any other containers. How did you verify/test it? Run test_ledd.py on arista 7050 platform which reports issue.

sujinmkang added 2 commits July 30, 2021 11:06

Fix wrong daemon names for platform daemon regression test

eab7b47

Restart the daemon if the autorestart is set to false in case of

7c6b588

unexpected exit

sujinmkang marked this pull request as ready for review August 3, 2021 15:00

sujinmkang requested review from bingwang-ms and prgeor August 3, 2021 15:01

Fix based on the autorestart enabled with buildimage PR sonic-net#8326

1e3b74f

lguohan reviewed Aug 4, 2021

View reviewed changes

tests/platform_tests/daemon/test_pcied.py Outdated Show resolved Hide resolved

sujinmkang changed the title ~~[ledd regression test] Add restart of the daemon when autorestart is not set~~ [ledd regression test] Correct the log messages with right daemon name Aug 4, 2021

sujinmkang changed the title ~~[ledd regression test] Correct the log messages with right daemon name~~ [pmon daemon regression test] Correct the log messages with right daemon name Aug 4, 2021

sujinmkang added 2 commits August 6, 2021 10:53

The pcied db fix is in other PR.

fab9aa1

Fix test_pmon_pcied/ledd_term_and_start_status failure

e2ff20c

sujinmkang changed the title ~~[pmon daemon regression test] Correct the log messages with right daemon name~~ [pmon daemon regression test] Correct the log messages with right daemon name and fix term_and_start_status failure Aug 11, 2021

sujinmkang requested a review from lguohan August 13, 2021 00:33

sujinmkang mentioned this pull request Aug 13, 2021

[202012] test_pmon_ledd_kill_and_start_status is consistently failing sonic-net/sonic-buildimage#8239

Closed

sujinmkang requested a review from vdahiya12 August 13, 2021 04:09

sujinmkang added 2 commits August 16, 2021 19:54

disable only pmon auto restart

01e359e

remove the disable_and_enable_autorestart_containers

5087487

since we want to make sure that pmon and any other containers are not restarted during this test.

prgeor reviewed Aug 18, 2021

View reviewed changes

review comments

352f0cb

prgeor approved these changes Aug 19, 2021

View reviewed changes

vdahiya12 approved these changes Aug 19, 2021

View reviewed changes

sujinmkang merged commit 42a3a81 into sonic-net:master Aug 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pmon daemon regression test] Correct the log messages with right daemon name and fix term_and_start_status failure #3922

[pmon daemon regression test] Correct the log messages with right daemon name and fix term_and_start_status failure #3922

sujinmkang commented Jul 30, 2021 •

edited

Loading

lguohan commented Aug 4, 2021

antonptashnik commented Aug 13, 2021

sujinmkang commented Aug 13, 2021

antonptashnik commented Aug 16, 2021

prgeor Aug 18, 2021

sujinmkang Aug 18, 2021 •

edited

Loading

prgeor Aug 18, 2021

sujinmkang Aug 18, 2021

prgeor Aug 18, 2021

sujinmkang Aug 18, 2021

prgeor Aug 18, 2021

sujinmkang Aug 18, 2021

prgeor Aug 18, 2021

sujinmkang Aug 18, 2021

prgeor Aug 18, 2021

sujinmkang Aug 18, 2021

[pmon daemon regression test] Correct the log messages with right daemon name and fix term_and_start_status failure #3922

[pmon daemon regression test] Correct the log messages with right daemon name and fix term_and_start_status failure #3922

Conversation

sujinmkang commented Jul 30, 2021 • edited Loading

Description of PR

Type of change

Back port request

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

lguohan commented Aug 4, 2021

antonptashnik commented Aug 13, 2021

sujinmkang commented Aug 13, 2021

antonptashnik commented Aug 16, 2021

Choose a reason for hiding this comment

sujinmkang Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sujinmkang commented Jul 30, 2021 •

edited

Loading

sujinmkang Aug 18, 2021 •

edited

Loading