sysready #8889

sg893052 · 2021-10-01T16:40:46Z

Why I did it

System Ready Feature Implementation as per https://github.com/Azure/SONiC/pull/875/files
At present, there is no mechanism 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 framework is introduced 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 status core" click CLI
"show system status all" click CLI
Syslogs for system ready

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

Description for the changelog

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

ghost · 2021-10-01T16:40:58Z

All CLA requirements met.

lgtm-com · 2021-10-01T17:04:40Z

This pull request introduces 12 alerts when merging 3068490182bd487229aa451538852924019e1815 into 3e397ce - view on LGTM.com

new alerts:

4 for Unused import
2 for Unused local variable
2 for Unreachable code
2 for Variable defined multiple times
1 for Unnecessary pass
1 for Module is imported with 'import' and 'import from'

lgtm-com · 2021-10-04T11:59:33Z

This pull request introduces 2 alerts when merging 5c28c6d4c91ce8c4a76ce3bd5b8d6f8fd9a453ba into df6361f - view on LGTM.com

new alerts:

2 for Unreachable code

lgtm-com · 2021-10-04T12:56:46Z

This pull request introduces 1 alert when merging e5bd0e9ee56db3a56abf110651a3c3b81d202606 into df6361f - view on LGTM.com

new alerts:

1 for Unreachable code

lgtm-com · 2021-10-04T14:12:46Z

This pull request introduces 1 alert when merging d820b985ef04b9922eaae828595743cef224e015 into df6361f - view on LGTM.com

new alerts:

1 for Unreachable code

lgtm-com · 2021-10-11T09:55:57Z

This pull request introduces 1 alert when merging 4053ebbe5e6a4d1887723762ddd22675a1cb7640 into 7d40384 - view on LGTM.com

new alerts:

1 for Unreachable code

lgtm-com · 2021-10-14T09:54:27Z

This pull request introduces 1 alert when merging 36f9aad2a24d409f9dfc743b55b039a9c81777b2 into b9366f3 - view on LGTM.com

new alerts:

1 for Variable defined multiple times

Kalimuthu-Velappan · 2021-10-27T15:05:34Z

retest this please

Kalimuthu-Velappan · 2021-10-28T05:50:41Z

/azpw run

lgtm-com · 2021-10-28T17:19:00Z

This pull request introduces 1 alert when merging 46cbef94a90d3e19214ff13f75ab6d8d33608f40 into 3788294 - view on LGTM.com

new alerts:

1 for Variable defined multiple times

lgtm-com · 2021-10-29T07:19:18Z

This pull request introduces 1 alert when merging fd11a1e4804cb8d49a2f854b6c40ae54c8349ff6 into 51c9c98 - view on LGTM.com

new alerts:

1 for Variable defined multiple times

venkatmahalingam · 2021-11-02T05:40:17Z

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

+                key "name";
+
+                leaf name {
+                    description "host feature name in host Feature table";


Can you give some examples of host features like hostcfgd, caclmgrd...etc?

Included the example in description now.

venkatmahalingam · 2021-11-02T05:42:54Z

files/build_templates/init_cfg.json.j2

-            "high_mem_alert": "disabled"
+            "high_mem_alert": "disabled",
+{%- if feature in ["bgp", "swss", "pmon", "nat", "teamd", "dhcp_relay", "sflow", "l2mcd", "udld", "stp", "snmp", "lldp", "radv", "iccpd", "syncd", "vrrp", "mgmt-framework", "tam"] %}
+            "check_up_status" : "false"


Why do we need this initialization, cant we assume if check_up_status field is not present, it's false?

This is part of framework provision for dockers to set "true" against "check_up_status" flag.
For now, all these dockers check_up_status is set to 'false'. Later once these dockers implement the logic of marking the up_status flag in STATE_DB, can set it to 'true' eventually.

venkatmahalingam · 2021-11-02T05:55:07Z

files/image_config/sysmonitor/sysmonitor.py

+            sysready_lock.unlock()
+        except Exception as e:
+            logger.log_error( str(e))
+            time.sleep(2)


Do we need to periodically check the status every 2 secs? what's the CPU usage for this service?

sleep(2) is invoked only in case of exception.
There is no periodic check.
As the design is on event based framework, the logic is to check the status of a service unit that was received in the queue. It will not hog the cpu.

sg893052 · 2021-11-05T13:44:03Z

system_ready_ut.pdf

System Ready UT log file uploaded.

lgtm-com · 2021-11-05T18:49:37Z

This pull request introduces 1 alert when merging 3135d67 into 0290207 - view on LGTM.com

new alerts:

1 for Variable defined multiple times

zhangyanzhao · 2021-11-10T22:52:43Z

Venkat approved and all build check passed. Merge it

This reverts commit d7e5372.

lguohan · 2021-11-10T23:36:57Z

to note, this pr has been reverted. per our discussion, i think this pr need mellanox folks to review and approve.

liat-grozovik · 2021-11-11T06:25:42Z

@zhangyanzhao as we have comments on the HLD itself i suggest to revert this PR as well.
it should not be merged without agreeing on the HLD.

zhangyanzhao · 2021-11-11T18:20:37Z

Add yang label since it has yang related change

lguohan · 2021-11-16T05:31:18Z

files/image_config/sysmonitor/sysmonitor.py

@@ -0,0 +1,691 @@
+#!/usr/bin/python3
+
+from datetime import datetime


need to package it into a proper python package, need unit test to validate the service.

lguohan · 2021-11-16T05:32:51Z

files/image_config/sysmonitor/sysmonitor.py

+spl_srv_list= ['database-chassis', 'gbsyncd']
+core_srv_list = [
+        'swss.service',
+        'bgp.service',


i am not sure bgp.service is a core service, we need to check how this interack with application extension framework.

We will probably need to define whether a docker is a core or not in the docker's manifest.

lguohan · 2021-11-16T05:33:34Z

files/image_config/sysmonitor/sysmonitor.py

+import argparse
+import multiprocessing as mp
+from swsssdk import ConfigDBConnector
+from swsssdk import SonicV2Connector


swsssdk is on the deprecation path, it cannot be used any more.

lguohan · 2021-11-16T05:45:41Z

files/image_config/sysmonitor/sysmonitor.py

+
+def db_connect():
+    try:
+        st_db = SonicV2Connector()


we should all use the swsscommon library.

lguohan · 2021-11-16T05:49:18Z

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

+                }
+            }
+        }
+        container HOST_FEATURE {


i am not sure we should introduce such yang model, why not in the feature table, we add a flag to indicate if it is host or container feature.

stepanblyschak · 2021-11-17T01:47:50Z

files/build_templates/init_cfg.json.j2

@@ -51,7 +51,27 @@
 {%- if feature in ["lldp", "pmon", "radv", "snmp", "telemetry"] %}
            "set_owner": "kube", {% else %}
            "set_owner": "local", {% endif %} {% endif %}
-            "high_mem_alert": "disabled"
+            "high_mem_alert": "disabled",
+{%- if feature in ["bgp", "swss", "pmon", "nat", "teamd", "dhcp_relay", "sflow", "l2mcd", "udld", "stp", "snmp", "lldp", "radv", "iccpd", "syncd", "vrrp", "mgmt-framework", "tam"] %}


dhcp_relay is now an application extension package. We should not have it hardcoded as we may have images with dhcp_relay and without it.
We need to add this new field here - https://github.com/Azure/sonic-utilities/blob/master/sonic_package_manager/service_creator/feature.py#L34.
And we need to add a new manifest variable to control whether "check_up_status" should be up true or false which will also be an indication whether docker implements marking the up_status flag in STATE_DB.

stepanblyschak · 2021-11-17T01:52:51Z

files/image_config/sysmonitor/sysmonitor.py

+spl_srv_list= ['database-chassis', 'gbsyncd']
+core_srv_list = [
+        'swss.service',
+        'bgp.service',


We will probably need to define whether a docker is a core or not in the docker's manifest.

sg893052 requested a review from lguohan as a code owner October 1, 2021 16:40

sg893052 mentioned this pull request Oct 1, 2021

SYSTEM READY sonic-net/SONiC#875

Merged

sg893052 force-pushed the SYSTEM_READY branch from 3068490 to 5c28c6d Compare October 4, 2021 11:35

sg893052 force-pushed the SYSTEM_READY branch from 5c28c6d to e5bd0e9 Compare October 4, 2021 12:34

sg893052 closed this Oct 4, 2021

sg893052 reopened this Oct 4, 2021

sg893052 force-pushed the SYSTEM_READY branch from e5bd0e9 to d820b98 Compare October 4, 2021 13:50

sg893052 force-pushed the SYSTEM_READY branch from d820b98 to 4053ebb Compare October 11, 2021 08:13

sg893052 force-pushed the SYSTEM_READY branch from 4053ebb to 36f9aad Compare October 14, 2021 08:19

sg893052 force-pushed the SYSTEM_READY branch from 36f9aad to c5ed18d Compare October 14, 2021 10:03

sg893052 force-pushed the SYSTEM_READY branch from c5ed18d to 0286c09 Compare October 27, 2021 08:16

sg893052 force-pushed the SYSTEM_READY branch from 0286c09 to 46cbef9 Compare October 28, 2021 14:42

sg893052 force-pushed the SYSTEM_READY branch from 46cbef9 to fd11a1e Compare October 29, 2021 05:45

venkatmahalingam reviewed Nov 2, 2021

View reviewed changes

sysready

3135d67

sg893052 force-pushed the SYSTEM_READY branch from fd11a1e to 3135d67 Compare November 5, 2021 17:14

venkatmahalingam approved these changes Nov 9, 2021

View reviewed changes

zhangyanzhao merged commit d7e5372 into sonic-net:master Nov 10, 2021

lguohan added a commit that referenced this pull request Nov 10, 2021

Revert "sysready (#8889)"

5f11eb3

This reverts commit d7e5372.

zhangyanzhao added the YANG YANG model related changes label Nov 11, 2021

lguohan reviewed Nov 16, 2021

View reviewed changes

stepanblyschak reviewed Nov 17, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sysready #8889

sysready #8889

sg893052 commented Oct 1, 2021

ghost commented Oct 1, 2021 •

edited by ghost

Loading

lgtm-com bot commented Oct 1, 2021

lgtm-com bot commented Oct 4, 2021

lgtm-com bot commented Oct 4, 2021

lgtm-com bot commented Oct 4, 2021

lgtm-com bot commented Oct 11, 2021

lgtm-com bot commented Oct 14, 2021

Kalimuthu-Velappan commented Oct 27, 2021

Kalimuthu-Velappan commented Oct 28, 2021

lgtm-com bot commented Oct 28, 2021

lgtm-com bot commented Oct 29, 2021

venkatmahalingam Nov 2, 2021

sg893052 Nov 4, 2021 •

edited

Loading

venkatmahalingam Nov 2, 2021

sg893052 Nov 4, 2021

venkatmahalingam Nov 2, 2021

sg893052 Nov 4, 2021

sg893052 commented Nov 5, 2021 •

edited

Loading

lgtm-com bot commented Nov 5, 2021

zhangyanzhao commented Nov 10, 2021

lguohan commented Nov 10, 2021

liat-grozovik commented Nov 11, 2021

zhangyanzhao commented Nov 11, 2021

lguohan Nov 16, 2021

lguohan Nov 16, 2021

stepanblyschak Nov 17, 2021

lguohan Nov 16, 2021

lguohan Nov 16, 2021

lguohan Nov 16, 2021

stepanblyschak Nov 17, 2021

stepanblyschak Nov 17, 2021

		@@ -0,0 +1,691 @@
		#!/usr/bin/python3

		from datetime import datetime

sysready #8889

sysready #8889

Conversation

sg893052 commented Oct 1, 2021

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

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

ghost commented Oct 1, 2021 • edited by ghost Loading

lgtm-com bot commented Oct 1, 2021

lgtm-com bot commented Oct 4, 2021

lgtm-com bot commented Oct 4, 2021

lgtm-com bot commented Oct 4, 2021

lgtm-com bot commented Oct 11, 2021

lgtm-com bot commented Oct 14, 2021

Kalimuthu-Velappan commented Oct 27, 2021

Kalimuthu-Velappan commented Oct 28, 2021

lgtm-com bot commented Oct 28, 2021

lgtm-com bot commented Oct 29, 2021

Choose a reason for hiding this comment

sg893052 Nov 4, 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

sg893052 commented Nov 5, 2021 • edited Loading

lgtm-com bot commented Nov 5, 2021

zhangyanzhao commented Nov 10, 2021

lguohan commented Nov 10, 2021

liat-grozovik commented Nov 11, 2021

zhangyanzhao commented Nov 11, 2021

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

ghost commented Oct 1, 2021 •

edited by ghost

Loading

sg893052 Nov 4, 2021 •

edited

Loading

sg893052 commented Nov 5, 2021 •

edited

Loading