System Ready #10479

sg893052 · 2022-04-06T08:58:12Z

Why I did it

At present, there is no mechanism in an event driven model to know that the system is up with all the essential sonic services and also, all the docker apps are ready along with port ready status to start the network traffic. With the asynchronous architecture of SONiC, we will not be able to verify if the config has been applied all the way down to the HW. But we can get the closest up status of each app and arrive at the system readiness.

How I did it

A new python based system monitor tool is introduced under system-health framework to monitor all the essential system host services including docker wrapper services on an event based model and declare the system is ready. This framework gives provision for docker apps to notify its closest up status. CLIs are provided to fetch the current system status and also service running status and its app ready status along with failure reason if any.

How to verify it

"show system-health sysready-status" click CLI
Syslogs for system ready

Which release branch to backport (provide reason below if selected)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

lgtm-com · 2022-04-06T09:33:08Z

This pull request introduces 15 alerts when merging 590ee50a9547bf9d9fd88b478aeefdca37e0f571 into b152f2a - view on LGTM.com

new alerts:

9 for Unused import
2 for Variable defined multiple times
1 for Wrong name for an argument in a call
1 for Testing equality to None
1 for Unused local variable
1 for Unreachable code

Junchao-Mellanox · 2022-04-08T05:02:27Z

Could you please add comments for each new function/class?

Junchao-Mellanox · 2022-04-08T05:02:40Z

Could you please fix the LGTM issues?

Junchao-Mellanox · 2022-04-08T05:05:33Z

Could please re-format files? I see many format issues, for example:

SYSLOG_IDENTIFIER="system#monitor"

should be

SYSLOG_IDENTIFIER = "system#monitor"

Junchao-Mellanox · 2022-04-08T05:07:00Z

Could you please remove unused comments?

Junchao-Mellanox · 2022-04-08T03:59:41Z

files/build_templates/init_cfg.json.j2

@@ -56,6 +56,9 @@
            "has_global_scope": {% if feature + '.service' in installer_services.split(' ') %}true{% else %}false{% endif %},
            "has_per_asic_scope": {% if feature + '@.service' in installer_services.split(' ') %}true{% else %}false{% endif %},
            "auto_restart": "{{autorestart}}",
+{%- if feature in ["bgp", "swss", "pmon"] %}


could you add comment to explain the purpose here?

Junchao-Mellanox · 2022-04-08T04:02:17Z

src/system-health/health_checker/sysmonitor.py

+import multiprocessing
+from collections import OrderedDict
+import tempfile
+from swsssdk import ConfigDBConnector


Please use ConfigDBConnector in swsscommon.

Junchao-Mellanox · 2022-04-08T04:04:04Z

src/system-health/health_checker/sysmonitor.py

+mpmgr = multiprocessing.Manager()
+
+#Logger class  for syslog
+class Logger(object):


Why not use sonic_py_common.logger.Logger?

Junchao-Mellanox · 2022-04-08T04:14:03Z

src/system-health/health_checker/sysmonitor.py

+#Initalise the syslog infrastructure
+logger = Logger(SYSLOG_IDENTIFIER)
+
+class Dict2Obj(object):


Is there a special reason to use this class instead of directly using dict?

Junchao-Mellanox · 2022-04-08T04:18:25Z

src/system-health/health_checker/sysmonitor.py

+
+   def unlock(self):
+      fcntl.flock(self.f, fcntl.LOCK_UN)
+      #self.f.close()


When will the file being closed?

Removed lock as it is not need here.

Junchao-Mellanox · 2022-04-08T06:20:38Z

src/system-health/health_checker/sysmonitor.py

+        if self.task_stopping_event.is_set():
+            return
+        self.task_queue.put(msg)
+        time.sleep(0.001)


what is the purpose of this sleep?

Junchao-Mellanox · 2022-04-08T06:20:50Z

src/system-health/health_checker/sysmonitor.py

+        time.sleep(0.001)
+
+    def task_stop(self):
+        self.task_stopping_event.set()


maybe you need stop the event loop?

when the event loop runs, as ProcessTaskBase in src/sonic-py-common/sonic_py_common/task_base.py has task_stop which actually kills the subprocess if not joined(exited) on time.

def task_stop(self): # Signal the process to stop self.task_stopping_event.set() # Wait for the process to exit self._task_process.join(self._stop_timeout_secs) # If the process didn't exit, attempt to kill it if self._task_process.is_alive(): os.kill(self._task_process.pid, signal.SIGKILL) if self._task_process.is_alive(): return False

Junchao-Mellanox · 2022-04-08T06:21:58Z

src/system-health/health_checker/sysmonitor.py

+
+        update_proc_name('sysmonitor')
+        event = 'SERVICE_EVENT'
+        sysready_lock.lock()


what is the lock for?

Removed lock as it is not needed here.

Junchao-Mellanox · 2022-04-08T06:23:46Z

src/system-health/health_checker/sysmonitor.py

+        # Queue to receive the STATEDB and Systemd state change event
+        while True:
+            try:
+                msg = myQ.get(timeout=15)


Please use constant value instead of magic number

Junchao-Mellanox · 2022-04-08T06:28:25Z

src/system-health/scripts/healthd

@@ -72,6 +73,8 @@ class HealthDaemon(DaemonBase):
            import sonic_platform.platform
            chassis = sonic_platform.platform.Platform().get_chassis()
            manager = HealthCheckerManager()
+            sysmon = Sysmonitor()
+            sysmon.system_service()


sysmon.system_service is a infinite loop, the rest of the code in this function will never run.

Taken care.

Junchao-Mellanox · 2022-04-08T06:29:50Z

could you please add unit test?

lgtm-com · 2022-04-24T08:11:26Z

This pull request introduces 4 alerts when merging 9388306a2644f61c903aa5b918f7ba5564af4af0 into 5cd6bc4 - view on LGTM.com

new alerts:

2 for Unused import
1 for Use of the return value of a procedure
1 for Unused local variable

Junchao-Mellanox · 2022-04-24T08:37:20Z

src/system-health/health_checker/sysmonitor.py

+SYSTEM_READY_TABLE = 'SYSTEM_READY'
+SYSTEM_ALLSRV_STATE="DOWN"
+SYSREADY_LOCKFILE="/var/run/sysready.lock"
+dnsrvs_name_list=[]


Could you elaborate the reason to keep it as a list? Query/remove an element from set is faster than list.

Junchao-Mellanox · 2022-04-24T08:39:59Z

src/system-health/health_checker/sysmonitor.py

+
+    #add the services from the below targets
+    path= ["/etc/systemd/system/multi-user.target.wants", "/etc/systemd/system/sonic.target.wants"]
+    for p in path:


Not sure I understand.

Junchao-Mellanox · 2022-04-24T08:41:06Z

src/system-health/health_checker/sysmonitor.py

+    global dnsrvs_name_list
+    global allsrvs_dict
+    global allsrvs_status
+    global spl_srv_list


spl_srv_list not used.

Junchao-Mellanox · 2022-04-24T08:57:35Z

src/system-health/health_checker/sysmonitor.py

+def run_systemctl_show(service):
+    command = ('systemctl show {} --property=Id,LoadState,UnitFileState,Type,ActiveState,SubState,Result'.format(service))
+    output = utils.run_command(command)
+    srv_properties = output.split('\n')


Maybe use output.splitlines()

Also, used glob in place of listdir and modified dnsrvs_name from list to set as per suggestion.

Junchao-Mellanox · 2022-04-24T09:02:32Z

src/system-health/health_checker/sysmonitor.py

+
+def post_unit_status(st_db, srv_name, srv_status, app_status, fail_reason, update_time):
+    key = 'ALL_SERVICE_STATUS|{}'.format(srv_name)
+    st_db.set(st_db.STATE_DB, key, 'service_status', srv_status)


could you consider using hmset?

lgtm-com · 2022-04-25T10:53:08Z

This pull request introduces 1 alert when merging ee0818c6f8ddc19ae926e7a128016717bd515f84 into 3fc3259 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2022-04-29T02:16:55Z

This pull request introduces 1 alert when merging 001b7ed92156e6ff9099f8a254a0c711ddee122c into e0f5333 - view on LGTM.com

new alerts:

1 for Unused import

sg893052 · 2022-05-05T13:05:38Z

Could please re-format files? I see many format issues, for example:
SYSLOG_IDENTIFIER="system#monitor"
should be
SYSLOG_IDENTIFIER = "system#monitor"

Taken care of all the format issues

sg893052 · 2022-05-05T13:06:41Z

Could you please add comments for each new function/class?

Added comments for all functions and classes

sg893052 · 2022-05-05T13:19:35Z

could you please add unit test?

Have added unit test coverage for the system ready code in test_system_health.py
Also, attaching the manual UT performed runtime on a target.

Manual UT runtime - system ready.txt

adyeung · 2022-05-06T17:18:19Z

/azpw run Azure.sonic-buildimage

sg893052 · 2022-05-07T06:41:42Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2022-05-07T06:41:44Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-05-07T06:41:55Z

Azure Pipelines successfully started running 1 pipeline(s).

Bring in all global functions under Sysmonitor class Add unit test code coverage for sysmonitor code Formatted the code

zhangyanzhao · 2022-05-13T16:33:00Z

/azpw run Azure.sonic-buildimage

sg893052 · 2022-05-14T05:15:12Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2022-05-14T05:15:13Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-05-14T05:15:27Z

Azure Pipelines successfully started running 1 pipeline(s).

sg893052 · 2022-05-15T02:07:34Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2022-05-15T02:07:35Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-05-15T02:07:47Z

Azure Pipelines successfully started running 1 pipeline(s).

sg893052 · 2022-05-16T04:47:29Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2022-05-16T04:47:31Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-05-16T04:47:45Z

Azure Pipelines successfully started running 1 pipeline(s).

sg893052 · 2022-05-16T16:11:04Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-05-16T16:11:09Z

Commenter does not have sufficient privileges for PR 10479 in repo Azure/sonic-buildimage

sg893052 · 2022-05-17T04:13:18Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-05-17T04:13:23Z

Commenter does not have sufficient privileges for PR 10479 in repo Azure/sonic-buildimage

liushilongbuaa · 2022-05-17T11:03:12Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2022-05-17T11:03:24Z

Azure Pipelines successfully started running 1 pipeline(s).

zhangyanzhao · 2022-05-18T05:29:55Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2022-05-18T05:30:08Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2022-06-10T20:34:48Z

src/sonic-yang-models/yang-models/sonic-feature.yang

+                    description "This configuration controls the system ready tool to check
+                        the app ready/up status";
+                    type boolean;
+                    default false;


Could you add yang model unit test?
We recently fixed a bug #10986

…anch Related work items: #52, #71, #73, #75, #77, sonic-net#1306, sonic-net#1588, sonic-net#1991, sonic-net#2031, sonic-net#2040, sonic-net#2053, sonic-net#2066, sonic-net#2069, sonic-net#2087, sonic-net#2107, sonic-net#2110, sonic-net#2112, sonic-net#2113, sonic-net#2117, sonic-net#2124, sonic-net#2125, sonic-net#2126, sonic-net#2128, sonic-net#2130, sonic-net#2131, sonic-net#2132, sonic-net#2133, sonic-net#2134, sonic-net#2135, sonic-net#2136, sonic-net#2137, sonic-net#2138, sonic-net#2139, sonic-net#2140, sonic-net#2143, sonic-net#2158, sonic-net#2161, sonic-net#2233, sonic-net#2243, sonic-net#2250, sonic-net#2254, sonic-net#2260, sonic-net#2261, sonic-net#2267, sonic-net#2278, sonic-net#2282, sonic-net#2285, sonic-net#2288, sonic-net#2289, sonic-net#2292, sonic-net#2294, sonic-net#8887, sonic-net#9279, sonic-net#9390, sonic-net#9511, sonic-net#9700, sonic-net#10025, sonic-net#10322, sonic-net#10479, sonic-net#10484, sonic-net#10493, sonic-net#10500, sonic-net#10580, sonic-net#10595, sonic-net#10628, sonic-net#10634, sonic-net#10635, sonic-net#10644, sonic-net#10670, sonic-net#10691, sonic-net#10716, sonic-net#10731, sonic-net#10750, sonic-net#10751, sonic-net#10752, sonic-net#10761, sonic-net#10769, sonic-net#10775, sonic-net#10776, sonic-net#10779, sonic-net#10786, sonic-net#10792, sonic-net#10793, sonic-net#10800, sonic-net#10806, sonic-net#10826, sonic-net#10839, sonic-net#10840, sonic-net#10842, sonic-net#10844, sonic-net#10847, sonic-net#10849, sonic-net#10852, sonic-net#10865, sonic-net#10872, sonic-net#10877, sonic-net#10886, sonic-net#10889, sonic-net#10903, sonic-net#10904, sonic-net#10905, sonic-net#10913, sonic-net#10914, sonic-net#10916, sonic-net#10919, sonic-net#10925, sonic-net#10926, sonic-net#10929, sonic-net#10933, sonic-net#10934, sonic-net#10937, sonic-net#10941, sonic-net#10947, sonic-net#10952, sonic-net#10953, sonic-net#10957, sonic-net#10959, sonic-net#10971, sonic-net#10972, sonic-net#10980

sg893052 requested review from qiluo-msft and lguohan as code owners April 6, 2022 08:58

sg893052 mentioned this pull request Apr 6, 2022

SYSTEM READY sonic-net/SONiC#977

Merged

Junchao-Mellanox suggested changes Apr 8, 2022

View reviewed changes

sg893052 force-pushed the SYSTEM_READY branch from 590ee50 to 9388306 Compare April 24, 2022 07:48

Junchao-Mellanox suggested changes Apr 24, 2022

View reviewed changes

sg893052 force-pushed the SYSTEM_READY branch from 9388306 to ee0818c Compare April 25, 2022 10:31

sg893052 force-pushed the SYSTEM_READY branch 3 times, most recently from e6f68dd to 001b7ed Compare April 29, 2022 01:55

sg893052 force-pushed the SYSTEM_READY branch 2 times, most recently from 95e5686 to 9782dbe Compare May 5, 2022 12:59

sysready

426c6cf

Bring in all global functions under Sysmonitor class Add unit test code coverage for sysmonitor code Formatted the code

sg893052 force-pushed the SYSTEM_READY branch from 9782dbe to 426c6cf Compare May 8, 2022 06:32

Merge branch 'Azure:master' into SYSTEM_READY

c1d89f8

Merge branch 'Azure:master' into SYSTEM_READY

15dc1f2

Merge branch 'Azure:master' into SYSTEM_READY

617c94f

Merge branch 'Azure:master' into SYSTEM_READY

6e37ca5

sujinmkang approved these changes May 20, 2022

View reviewed changes

sujinmkang merged commit f37dd77 into sonic-net:master May 20, 2022

qiluo-msft reviewed Jun 10, 2022

View reviewed changes

qiluo-msft mentioned this pull request Jun 10, 2022

Add yang model unit test for check_up_status field type #11110

Closed

System Ready #10479

System Ready #10479

Conversation

sg893052 commented Apr 6, 2022

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

lgtm-com bot commented Apr 6, 2022

Junchao-Mellanox commented Apr 8, 2022

Junchao-Mellanox commented Apr 8, 2022

Junchao-Mellanox commented Apr 8, 2022

Junchao-Mellanox commented Apr 8, 2022 • 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

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

Junchao-Mellanox commented Apr 8, 2022

lgtm-com bot commented Apr 24, 2022

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

lgtm-com bot commented Apr 25, 2022

lgtm-com bot commented Apr 29, 2022

sg893052 commented May 5, 2022

sg893052 commented May 5, 2022

sg893052 commented May 5, 2022

adyeung commented May 6, 2022

sg893052 commented May 7, 2022

mssonicbld commented May 7, 2022

azure-pipelines bot commented May 7, 2022

zhangyanzhao commented May 13, 2022

sg893052 commented May 14, 2022

mssonicbld commented May 14, 2022

azure-pipelines bot commented May 14, 2022

sg893052 commented May 15, 2022

mssonicbld commented May 15, 2022

azure-pipelines bot commented May 15, 2022

sg893052 commented May 16, 2022

mssonicbld commented May 16, 2022

azure-pipelines bot commented May 16, 2022

sg893052 commented May 16, 2022

azure-pipelines bot commented May 16, 2022

sg893052 commented May 17, 2022

azure-pipelines bot commented May 17, 2022

liushilongbuaa commented May 17, 2022

azure-pipelines bot commented May 17, 2022

zhangyanzhao commented May 18, 2022

azure-pipelines bot commented May 18, 2022

Choose a reason for hiding this comment

Junchao-Mellanox commented Apr 8, 2022 •

edited

Loading