Xcvrd should restart if any child thread crashes #326

mihirpat1 · 2022-12-28T10:56:08Z

Description

Make all task workers as child thread of main process.
The main process should watch for any child thread crash and do suicide to restart itself. Also ensure the backtrace of crashing child thread is shown

Motivation and Context

Overall, the change set now adheres to a template for creating thread and handles exceptions followed by restarting xcvrd.
With this change set, 3 threads will be created:

CmisManagerTask (This was formerly a process)
DomInfoUpdateTask (This already exists as a thread)
SfpStateUpdateTask (This was formerly a process)

All the above threads have now been named accordingly.
Once an exception is raised, the run() method will generate a traceback and will set the main_thread_stop event.
The xcvrd daemon keeps waiting until main_thread_stop event is generated. Once this event is generated, we first check if any thread is dead. If so, we capture the exception, evaluate if other threads have an exception generated as well and then force the xcvrd process to kill itself.
In case all threads are alive, we allow xcvrd daemon to gracefully exit.

Note - In order to kill SfpStateUpdateTask thread gracefully, we are manually raising an exception since SfpStateUpdateTask has a call to an API which could potentially sleep in the order of seconds and hence, could block the xcvrd daemon graceful shutdown process for a prolonged time.

How Has This Been Tested?

Please find the summary below.
1. Kill xcvrd
2. Restart xcvrd
3. Restart pmon
4. Kill 1 thread and ensure all threads are killed and xcvrd is respawned
5. Ensure backtrace is generated with a manual exception - CmisManagerTask (Please refer to the below note for details)
6. Ensure backtrace is generated with a manual exception - DomInfoUpdateTask (Please refer to the below note for details)
7. Ensure backtrace is generated with a manual exception - SfpStateUpdateTask (Please refer to the below note for details)
8. Ensure on Mellanox, no side effect is seen since they do not spawn CMIS thread

For a detailed UT, please refer to the below enclosure.
xcvrd_thread_support.txt

Additional Information (Optional)

Signed-off-by: Mihir Patel patelmi@microsoft.com

Signed-off-by: Mihir Patel <patelmi@microsoft.com>

prgeor · 2023-01-03T22:57:22Z

@keboliu can you review

prgeor · 2023-01-03T23:13:54Z

sonic-xcvrd/xcvrd/xcvrd.py

+            self.exc = e
+            self.main_thread_stop_event.set()
+
+    def raise_exception(self):


can you add comment here why this exception is raised here?

I have added a comment now.

prgeor · 2023-01-03T23:23:15Z

@mihirpat1 can you check the behavior if you kill the threads with kill -9

prgeor · 2023-01-03T23:34:00Z

@jaganbal-a @shyam77git please review

Signed-off-by: Mihir Patel <patelmi@microsoft.com>

mihirpat1 · 2023-01-04T07:52:04Z

@mihirpat1 can you check the behavior if you kill the threads with kill -9

It seems we won't be able to kill threads using kill -9 since kill requires process ID as a parameter. I have attached UT for killing xcvrd process though.

keboliu · 2023-01-05T07:35:40Z

What if "CmisManagerTask " is disabled with intention? On the Mellanox platforms "CmisManagerTask" is disabled by setting the "skip_xcvrd_cmis_mgr" to "true".

mihirpat1 · 2023-01-05T08:16:29Z

What if "CmisManagerTask " is disabled with intention? On the Mellanox platforms "CmisManagerTask" is disabled by setting the "skip_xcvrd_cmis_mgr" to "true".

If "CmisManagerTask" is disabled with intention, we will not spawn thread for it. Also, I have attached the UT for it (#8 in xcvrd_thread_support.txt)

jaganbal-a · 2023-01-05T17:53:25Z

sonic-xcvrd/xcvrd/xcvrd.py

@@ -2199,9 +2247,10 @@ class DaemonXcvrd(daemon_base.DaemonBase):
    def __init__(self, log_identifier, skip_cmis_mgr=False):
        super(DaemonXcvrd, self).__init__(log_identifier)
        self.stop_event = threading.Event()
-        self.sfp_error_event = multiprocessing.Event()
+        self.sfp_error_event = threading.Event()


sfp_error_event event seems not handled.

It seems we are using it at the below:
https://github.com/sonic-net/sonic-platform-daemons/blob/master/sonic-xcvrd/xcvrd/xcvrd.py#L2391-L2392
if self.sfp_error_event.is_set():
sys.exit(SFP_SYSTEM_ERROR)

But that code will never get hit, when the event get set due to error.

Agreed, since we are killing the parent process (xcvrd) if next_state is STATE_EXIT, the above code will never get hit. This seems like a day 1 behavior though.

jaganbal-a · 2023-01-05T19:20:03Z

sonic-xcvrd/xcvrd/xcvrd.py

+        threading.Thread.__init__(self)
+        self.name = "CmisManagerTask"
+        self.exc = None
+        self.task_stopping_event = threading.Event()


Can you please explain how this event created inside the thread will be used?

This event will be used to stop the handling the task_worker routine (in case of exception handling or graceful shutdown of the thread). The task_worker routine will be running in a while loop waiting for task_stopping_event to be set in addition to performing necessary actions.
Hence, once we want the thread to exit, we must stop the execution of task_worker routine so that it comes out of the while loop and allows us to proceed with exception handling or graceful shutdown.

jaganbal-a · 2023-01-06T18:03:03Z

sonic-xcvrd/xcvrd/xcvrd.py


+        # Stop the sfp state info update thread
+        if sfp_state_update.is_alive():
+            sfp_state_update.raise_exception()


Will it be good to join(1) with timeout and raise exception if timeout expires and the thread is still alive?

I feel it's better to raise exception right away since based on my current analysis on Arista testbed, it takes about ~5s for the vendor API to come out of sleep. This timeout I assume could be dynamic based on the transceivers present on the device so I think having 1s delay will not give us a much advantage for join function with 1s timeout.

sonic-xcvrd/xcvrd/xcvrd.py

…daemons into xcvr_thread_support

…sed to be skipped. Also, moidified join function to handle accordingly. Added logs for showing names of threads spawned Signed-off-by: Mihir Patel <patelmi@microsoft.com>

* Xcvrd should restart if any child thread crashes Signed-off-by: Mihir Patel <patelmi@microsoft.com> * Resolved test_SfpStateUpdateTask_task_run_stop test failure * Added comment for raise_exception Signed-off-by: Mihir Patel <patelmi@microsoft.com> * Added check for avoiding cmis_manager.start() if CMIS thread is supposed to be skipped. Also, moidified join function to handle accordingly. Added logs for showing names of threads spawned Signed-off-by: Mihir Patel <patelmi@microsoft.com> Signed-off-by: Mihir Patel <patelmi@microsoft.com>

prgeor · 2023-02-02T19:23:56Z

@StormLiangMS please pick to 202211

* Xcvrd should restart if any child thread crashes Signed-off-by: Mihir Patel <patelmi@microsoft.com> * Resolved test_SfpStateUpdateTask_task_run_stop test failure * Added comment for raise_exception Signed-off-by: Mihir Patel <patelmi@microsoft.com> * Added check for avoiding cmis_manager.start() if CMIS thread is supposed to be skipped. Also, moidified join function to handle accordingly. Added logs for showing names of threads spawned Signed-off-by: Mihir Patel <patelmi@microsoft.com> Signed-off-by: Mihir Patel <patelmi@microsoft.com>

mihirpat1 added 2 commits December 28, 2022 10:50

Xcvrd should restart if any child thread crashes

350bfe2

Signed-off-by: Mihir Patel <patelmi@microsoft.com>

Resolved test_SfpStateUpdateTask_task_run_stop test failure

896a585

mihirpat1 marked this pull request as ready for review December 28, 2022 11:43

mihirpat1 requested a review from prgeor December 28, 2022 11:43

prgeor added the xcvrd label Jan 3, 2023

prgeor reviewed Jan 3, 2023

View reviewed changes

Added comment for raise_exception

64107dd

Signed-off-by: Mihir Patel <patelmi@microsoft.com>

jaganbal-a reviewed Jan 5, 2023

View reviewed changes

jaganbal-a reviewed Jan 6, 2023

View reviewed changes

jaganbal-a previously approved these changes Jan 9, 2023

View reviewed changes

keboliu reviewed Jan 10, 2023

View reviewed changes

sonic-xcvrd/xcvrd/xcvrd.py Show resolved Hide resolved

keboliu reviewed Jan 10, 2023

View reviewed changes

sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved

vdahiya12 reviewed Jan 12, 2023

View reviewed changes

sonic-xcvrd/xcvrd/xcvrd.py Show resolved Hide resolved

vdahiya12 previously approved these changes Jan 12, 2023

View reviewed changes

mihirpat1 added 2 commits January 13, 2023 20:08

Merge branch 'master' of https://github.com/sonic-net/sonic-platform-…

3b13cca

…daemons into xcvr_thread_support

Added check for avoiding cmis_manager.start() if CMIS thread is suppo…

311ed90

…sed to be skipped. Also, moidified join function to handle accordingly. Added logs for showing names of threads spawned Signed-off-by: Mihir Patel <patelmi@microsoft.com>

mihirpat1 dismissed stale reviews from vdahiya12 and jaganbal-a via 311ed90 January 13, 2023 20:29

keboliu approved these changes Jan 18, 2023

View reviewed changes

prgeor approved these changes Jan 19, 2023

View reviewed changes

prgeor merged commit 2211b7e into sonic-net:master Jan 19, 2023

prgeor added the Enhancement label Jan 19, 2023

mihirpat1 added Request for 202205 Branch Request for 202211 Branch labels Jan 19, 2023

yxieca added Cherry Pick Conflict_202205 Included in 202205 Branch and removed Cherry Pick Conflict_202205 labels Jan 19, 2023

StormLiangMS added the Included in 202211 Branch label Feb 4, 2023

Junchao-Mellanox mentioned this pull request Sep 5, 2023

[Mellanox] set select timeout to no more than 1 sec to make sure fast shutdown sonic-net/sonic-buildimage#13611

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xcvrd should restart if any child thread crashes #326

Xcvrd should restart if any child thread crashes #326

mihirpat1 commented Dec 28, 2022 •

edited

Loading

prgeor commented Jan 3, 2023

prgeor Jan 3, 2023

mihirpat1 Jan 4, 2023

prgeor commented Jan 3, 2023

prgeor commented Jan 3, 2023

mihirpat1 commented Jan 4, 2023

keboliu commented Jan 5, 2023

mihirpat1 commented Jan 5, 2023

jaganbal-a Jan 5, 2023 •

edited

Loading

mihirpat1 Jan 6, 2023

jaganbal-a Jan 6, 2023

mihirpat1 Jan 6, 2023

jaganbal-a Jan 5, 2023 •

edited

Loading

mihirpat1 Jan 6, 2023 •

edited

Loading

jaganbal-a Jan 6, 2023

mihirpat1 Jan 10, 2023 •

edited

Loading

prgeor commented Feb 2, 2023

Xcvrd should restart if any child thread crashes #326

Xcvrd should restart if any child thread crashes #326

Conversation

mihirpat1 commented Dec 28, 2022 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

prgeor commented Jan 3, 2023

prgeor Jan 3, 2023

Choose a reason for hiding this comment

mihirpat1 Jan 4, 2023

Choose a reason for hiding this comment

prgeor commented Jan 3, 2023

prgeor commented Jan 3, 2023

mihirpat1 commented Jan 4, 2023

keboliu commented Jan 5, 2023

mihirpat1 commented Jan 5, 2023

jaganbal-a Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

mihirpat1 Jan 6, 2023

Choose a reason for hiding this comment

jaganbal-a Jan 6, 2023

Choose a reason for hiding this comment

mihirpat1 Jan 6, 2023

Choose a reason for hiding this comment

jaganbal-a Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

mihirpat1 Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

jaganbal-a Jan 6, 2023

Choose a reason for hiding this comment

mihirpat1 Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

prgeor commented Feb 2, 2023

mihirpat1 commented Dec 28, 2022 •

edited

Loading

jaganbal-a Jan 5, 2023 •

edited

Loading

jaganbal-a Jan 5, 2023 •

edited

Loading

mihirpat1 Jan 6, 2023 •

edited

Loading

mihirpat1 Jan 10, 2023 •

edited

Loading