[Link Event Damping] Add per port link event damper class. #1334
Conversation
- Adding per port link event damper class that manages the internal behavior and workings of link event damping logic on a port where link event damping config is enabled. - This class keeps track of damping timer, current port state, post damping advertised port state and relevant debug counters per port. HLD: sonic-net/SONiC#1071
| if (m_penaltyCeiling - m_accumulatedPenalty > m_flapPenalty) | ||
| { | ||
| m_accumulatedPenalty += m_flapPenalty; | ||
| } | ||
| else | ||
| { | ||
| m_accumulatedPenalty = m_penaltyCeiling; | ||
| } |
| SWSS_LOG_ENTER(); | ||
|
|
||
| m_timer.setInterval( | ||
| /*interval=*/{.tv_sec = (time_us / int64_t(MICRO_SECS_PER_SEC)), |
There was a problem hiding this comment.
comment in weird place, please remove we know it's setInterval function
| m_maxSuppressTimeUsec = | ||
| uint64_t(config.max_suppress_time) * MICRO_SECS_PER_MILLI_SEC; |
There was a problem hiding this comment.
join those lines, and why you need to explicitly cast to 64?
|
|
||
| namespace syncd | ||
| { | ||
|
|
| inline uint64_t getCurrentTimeUsecs() | ||
| { | ||
| SWSS_LOG_ENTER(); | ||
|
|
||
| struct timespec ts; | ||
| clock_gettime(CLOCK_MONOTONIC, &ts); | ||
|
|
||
| return (ts.tv_sec * MICRO_SECS_PER_SEC) + | ||
| (ts.tv_nsec / NANO_SECS_PER_MICRO_SEC); | ||
| } |
| struct DampingStats | ||
| { |
There was a problem hiding this comment.
each struct and class should have it's own file cpp and h
| // Number of link UP events received. | ||
| uint64_t pre_damping_up_events; | ||
| // Number of link DOWN events received. | ||
| uint64_t pre_damping_down_events; | ||
| // Number of link UP events advertised post damping. | ||
| uint64_t post_damping_up_events; | ||
| // Number of link DOWN events advertised post damping. | ||
| uint64_t post_damping_down_events; | ||
| // Timestamp for last advertised link up event post damping. | ||
| std::string last_advertised_up_event_timestamp; | ||
| // Timestamp for last advertised link down event post damping. | ||
| std::string last_advertised_down_event_timestamp; |
There was a problem hiding this comment.
add empty line before each line of comment for readibility or put all comments on the right side
| struct DampingStats m_stats; | ||
|
|
||
| friend class PortLinkEventDamperPeer; | ||
| friend class LinkEventDamperPeer; |
There was a problem hiding this comment.
in entire project we don't use friend class, please explain in details why this is exactly what you need and it cant be done in other way?
| { | ||
|
|
||
| // Peer class to access private state and APIs. | ||
| class PortLinkEventDamperPeer |
There was a problem hiding this comment.
should this be Mock class?
There was a problem hiding this comment.
classes in tests should be in separated files
| sai_serialize_object_id(data.port_id).c_str(), | ||
| sai_serialize_port_oper_status(data.port_state).c_str()); | ||
|
|
||
| m_notificationHandler->onPortStateChangePostLinkEventDamping(/*count=*/1, &data); |
There was a problem hiding this comment.
this is not right, you are manually generating port notification, this breaks SAI design, @lguohan we need to discuss this whether this is allowed
There was a problem hiding this comment.
This is not manual generation of port event notification exactly rather delayed generation of port events as it is intended with link event damping feature - for the link event damping, the actual port events will be trapped here (https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/NotificationHandler.cpp#L164-L172) (NotificationHandler changes will be added in subsequent PRs to do that) and sent to link event damping logic running in syncd main thread where events will be processed as per the damping algo and config. If port events are not needed to be damped or damping duration have elapsed and damped port event needs to be advertised to OA (and other applications), corresponding port event notifications will be generated as the case here.
Since we are doing review of a portion of a code at a time, full logic is not self explanatory here.
Targeted final onPortStateChange() will look like following where port events are trapped and sends for processing. Here m_portStateChangeHandler is of type PortStateChangeHandler (added in #1310):
void NotificationHandler::onPortStateChange(
In uint32_t count,
In const sai_port_oper_status_notification_t *data)
{
SWSS_LOG_ENTER();
if (m_portStateChangeHandler == nullptr) {
auto s = sai_serialize_port_oper_status_ntf(count, data);
enqueueNotification(SAI_SWITCH_NOTIFICATION_NAME_PORT_STATE_CHANGE, s);
} else {
// Send notification to port state change handler for further processing.
m_portStateChangeHandler->HandlePortStateChangeNotification(count, data);
}
}
cc: @mikeberesford.
There was a problem hiding this comment.
it seems like manual generate, since this code is creating notification data and manually calling onPortStateChangePostLinkEventDamping, which all notifications should come from vendor SAI and not be generated on demand
There was a problem hiding this comment.
That is the intention of the feature. SAI generated link event is trapped by link event damping feature and it will be sent to application as per the damping logic. For example:
- if damping logic says SAI generated event notification needs to damped, that notification will be suppressed in the syncd and will not be sent to application.
- if damping logic says no damping needed, then the notification needs to be sent to application using onPortStateChangePostLinkEventDamping(). Since original SAI notification is trapped and processed in the damper logic, we need to manually create the corresponding notification data and send it to application.
Since the original notification from SAI is trapped and processed by damper, we will need to create notification data when notification needs to be sent.
There was a problem hiding this comment.
Not fully understand this logic, can you make some kind of flow diagram ? So e I'm not convinced still why we generate manually this notification
HLD: sonic-net/SONiC#1071