Notification NOS running SAI and SDK, about HW/FW failures #1777

amandyuk · 2023-03-09T12:02:18Z

Health event is a way to inform NOS about some HW issue in the switch. When some error on switch occur, SAI should in some way inform a NOS about the status. In order to provide flexibility depending on different types issues SAI generates health event with several parameters that describes that event. Hence, to inform NOS - sai_switch_asic_sdk_health_event_notification_fn callback is invoked.

kcudnik · 2023-03-09T18:18:19Z

inc/saiswitch.h

+        _In_ sai_timespec_t timestamp,
+        _In_ sai_switch_asic_sdk_health_category_t category,
+        _In_ sai_switch_health_data_t data,
+        _In_ sai_u8_list_t description);


this probably should be const since contains pointer to data

Right now data is passed as value not as pointer
In sai_switch_health_data_t data,
Do you suggest changing to
In const sai_switch_health_data_t *data
?

i was talking about sai_u8_list_t, since u8 list has pointer inside, which without const can be modified

itaibaz · 2023-03-09T21:42:54Z

@JaiOCP
Can you please review
We added the ability to extend and provide per event data, so
#1307
for example, can be implemented with the health event
Please review the suggested extension in the .MD
Thanks

JaiOCP · 2023-03-09T22:39:23Z

@JaiOCP
Can you please review
We added the ability to extend and provide per event data, so
#1307
for example, can be implemented with the health event
Please review the suggested extension in the .MD
Thanks

Thanks Itai

rlhui · 2023-03-12T23:04:43Z

health_event.md

+
+    /** @passparam data_type */
+    sai_health_data_t data;
+} sai_health_t;


don't see this defined in header files

It is not defined in the headers since SER is a separate PR
The intention is to show the health event can be used as infrastructure for additional future event
For each event, we will need separate PR. The struct with specific details will be defined in the PR, and will added to the union.
I am not trying to address every possible health event at the moment, just create an extendable infrastructure which can easily be extended

rlhui · 2023-03-12T23:18:56Z

inc/saiswitch.h

+        _In_ sai_switch_asic_sdk_health_severity_t severity,
+        _In_ sai_timespec_t timestamp,
+        _In_ sai_switch_asic_sdk_health_category_t category,
+        _In_ sai_switch_health_data_t data,


from definitions so far, this "data" is actually a data type, and only one type of data SAI_HEALTH_DATA_TYPE_GENERAL can be passed.

yes currently we don't need to use data for the general type
but we created extendable infrastructure, per Jai's request, so additional events can be added

rlhui · 2023-03-12T23:20:45Z

health_event.md

+
+2) For all types that have additional data, a struct should be created. The first time a new type that has actual struct data will be created, union should be created as well. Each struct should be added to that union.
+
+For example if we want to add a SER item (https://github.com/opencomputeproject/SAI/pull/1307), the flow will be :


Example looks not complete. It should show how the attribute and parameters should be assigned and passed to function

This will be addressed in SER specific PR
It is out of scope of this PR which is the general infrastructure

rlhui · 2023-03-13T01:13:47Z

Could we please look into supporting the data defined in json? sai_json_t is available.

itaibaz · 2023-03-13T03:04:32Z

@rlhui
I am not sure I follow the requirement for json
In the mentioned PR
https://github.com/sonic-net/SONiC/blob/5677242770e3774a5eab0f951dc14642d81422ec/doc/event-alarm-framework/events-producer.md
Sonic is moving to this model, to define the contract between Sonic and external tools on top of Sonic

I don't see how it's related to SAI layer

The parameters for the health event are well typed, other than the description field which is free text and vendor specific
To generalize, we did add the category (on top of the initial severity we had), per community request
Can you please explain what exactly are you looking for ?

rlhui · 2023-03-16T05:53:32Z

@rlhui I am not sure I follow the requirement for json In the mentioned PR https://github.com/sonic-net/SONiC/blob/5677242770e3774a5eab0f951dc14642d81422ec/doc/event-alarm-framework/events-producer.md Sonic is moving to this model, to define the contract between Sonic and external tools on top of Sonic

I don't see how it's related to SAI layer

The parameters for the health event are well typed, other than the description field which is free text and vendor specific To generalize, we did add the category (on top of the initial severity we had), per community request Can you please explain what exactly are you looking for ?

Please see #1324 as an example. For the categories added, they can also be defined as part of json format.

rlhui · 2023-03-16T06:12:08Z

Can we please update PR to add description

rlhui · 2023-03-16T06:18:09Z

Can we pleae also update PR title ? Currently it's truncated.

itaibaz · 2023-03-16T08:57:23Z

Added description to the PR
Also modified the title to avoid truncation

rlhui · 2023-03-17T17:16:26Z

build is failing

kcudnik · 2023-03-17T17:25:30Z

WARNING: first line doxygen comment should be with '/**': saiswitch.h: /** 
WARNING: line ends in whitespace saiswitch.h 2875:      
WARNING: Header doesn't meet style requirements (most likely ident is not 4 or 8 spaces) saiswitch.h 2875:     
WARNING: line ends in whitespace saiswitch.h 2876:     /**

Signed-off-by: Andrii Mandiuk <[email protected]>

prsunny · 2023-03-22T17:15:43Z

inc/saiswitch.h

+    SAI_SWITCH_ASIC_SDK_HEALTH_SEVERITY_WARNING,
+
+    /** Switch event severity notice */
+    SAI_SWITCH_ASIC_SDK_HEALTH_SEVERITY_NOTICE


how do we know if a specific HEALTH issue is cleared. Also if its correctable/non-correctable?

Re if it's correctable or not, the severity field can be used as indication
If it's fatal, then it's not correctable
If it's warning or notice, then it is correctable

Re clearing of health issue
The concept is for one time event
It's not an alarm where a certain threshold is crossed and then can later be cleared when going under the threshold

prsunny

lgtm

kcudnik · 2023-06-20T07:23:38Z

inc/saitypes.h

+typedef struct _sai_switch_health_data_t
+{
+    /** Type of switch health data */
+    sai_health_data_type_t data_type;


since this struct contains only one entry, is it not better to use data_type directly in notification ?

it's an extendable placeholder
check out possible extension for SER in the .MD above
The ask in the community was to be able to extend and add info for specific features in the future

since we want to make structs constant then this would not expand any more

…teproject#1777) Health event is a way to inform NOS about some HW issue in the switch. When some error on switch occur, SAI should in some way inform a NOS about the status. In order to provide flexibility depending on different types issues SAI generates health event with several parameters that describes that event. Hence, to inform NOS - sai_switch_asic_sdk_health_event_notification_fn callback is invoked. Signed-off-by: Andrii Mandiuk <[email protected]>

kcudnik reviewed Mar 9, 2023

View reviewed changes

itaibaz mentioned this pull request Mar 9, 2023

Implement a mechanism that to enable a NOS running SAI and SDK, to detect SDK and FW failures #1615

Closed

JaiOCP approved these changes Mar 9, 2023

View reviewed changes

amandyuk force-pushed the health_event_feature branch from c72d540 to 50b21ce Compare March 10, 2023 13:50

kcudnik approved these changes Mar 10, 2023

View reviewed changes

rlhui reviewed Mar 12, 2023

View reviewed changes

amandyuk changed the title ~~Implement a mechanism that to enable a NOS running SAI and SDK, to de…~~ Notification NOS running SAI and SDK, about HW/FW failures Mar 16, 2023

amandyuk force-pushed the health_event_feature branch from 50b21ce to 07cae66 Compare March 17, 2023 13:20

amandyuk force-pushed the health_event_feature branch from 07cae66 to f6f25c3 Compare March 18, 2023 12:14

Notification NOS running SAI and SDK, about HW/FW failures

3999b41

Signed-off-by: Andrii Mandiuk <[email protected]>

amandyuk force-pushed the health_event_feature branch from c63f71f to 3999b41 Compare March 18, 2023 12:23

prsunny reviewed Mar 22, 2023

View reviewed changes

kcudnik approved these changes Mar 22, 2023

View reviewed changes

prsunny approved these changes Jun 9, 2023

View reviewed changes

rlhui merged commit e06d81e into opencomputeproject:master Jun 20, 2023

kcudnik reviewed Jun 20, 2023

View reviewed changes

JaiOCP mentioned this pull request Mar 6, 2024

SER Support #1979

Closed

JaiOCP mentioned this pull request Mar 7, 2024

SER Data #1982

Closed

JaiOCP mentioned this pull request Apr 15, 2024

SER Support #1993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification NOS running SAI and SDK, about HW/FW failures #1777

Notification NOS running SAI and SDK, about HW/FW failures #1777

amandyuk commented Mar 9, 2023 •

edited

Loading

kcudnik Mar 9, 2023

itaibaz Mar 9, 2023

kcudnik Mar 9, 2023

itaibaz commented Mar 9, 2023

JaiOCP commented Mar 9, 2023

rlhui Mar 12, 2023

itaibaz Mar 13, 2023

rlhui Mar 12, 2023

itaibaz Mar 13, 2023

rlhui Mar 12, 2023

itaibaz Mar 13, 2023

rlhui commented Mar 13, 2023

itaibaz commented Mar 13, 2023

rlhui commented Mar 16, 2023

rlhui commented Mar 16, 2023

rlhui commented Mar 16, 2023

itaibaz commented Mar 16, 2023

rlhui commented Mar 17, 2023

kcudnik commented Mar 17, 2023

prsunny Mar 22, 2023

itaibaz Mar 24, 2023

prsunny left a comment

kcudnik Jun 20, 2023

itaibaz Jun 20, 2023

kcudnik Jun 21, 2023


		2) For all types that have additional data, a struct should be created. The first time a new type that has actual struct data will be created, union should be created as well. Each struct should be added to that union.

		For example if we want to add a SER item (https://github.com/opencomputeproject/SAI/pull/1307), the flow will be :

Notification NOS running SAI and SDK, about HW/FW failures #1777

Notification NOS running SAI and SDK, about HW/FW failures #1777

Conversation

amandyuk commented Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaibaz commented Mar 9, 2023

JaiOCP commented Mar 9, 2023

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

rlhui commented Mar 13, 2023

itaibaz commented Mar 13, 2023

rlhui commented Mar 16, 2023

rlhui commented Mar 16, 2023

rlhui commented Mar 16, 2023

itaibaz commented Mar 16, 2023

rlhui commented Mar 17, 2023

kcudnik commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny left a 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

amandyuk commented Mar 9, 2023 •

edited

Loading