[sflow] Add Dropped Packet Notification (MOD) support. - #3970
[sflow] Add Dropped Packet Notification (MOD) support.#3970yehjunying wants to merge 11 commits into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR adds SFLOW drop monitor functionality to track and rate-limit dropped packets using TAM (Telemetry and Monitoring) infrastructure. The implementation enables drop monitoring only when SFLOW is enabled and provides configurable rate limiting through a policer.
- Introduces
SflowDropMonitorclass to manage TAM-based drop monitoring - Adds configuration support for
drop_monitor_limitparameter in SFLOW global settings - Implements comprehensive SAI object lifecycle management for TAM, policer, and hostif trap components
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| orchagent/sfloworch.h | Adds SflowDropMonitor class definition with TAM object management methods and member variable to SflowOrch |
| orchagent/sfloworch.cpp | Implements drop monitor enable/disable logic, TAM object creation/removal, and global config parsing with drop_monitor_limit |
| tests/mock_tests/portal.h | Adds accessor methods for testing drop monitor status and limit rate |
| tests/mock_tests/sfloworh_ut.cpp | Adds unit tests for drop monitor enable/disable and rate limit change scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| set_switch_capability(fvVector); | ||
| } | ||
|
|
||
| void SwitchOrch::querySwitchMirrorOnDropCapability() |
There was a problem hiding this comment.
It looks like some of the checks here are too specific to HOSTIF traps, but the capability being set is generic SWITCH_CAPABILITY_TABLE_MIRROR_ON_DROP_CAPABLE. Does it make sense to refactor this and add probably a capability specific to SFLOW/HOSTIF so that it can be reused for other MOD implementations ?
There was a problem hiding this comment.
The current capability check is based on the functions used by the sFlow drop monitor as described in the HLD. Other MOD implementations may have different requirements and fall outside the scope of this change.
There was a problem hiding this comment.
Since this is specific to sFlow, doesn't it make sense to name it as such by factoring out the non-sflow specific code from here ? Or are you thinking querySwitchMirrorOnDropCapability would evolve to add additional checks and return true if any form of MOD is supported ? Even if that is the case, I would recommend refactoring this to put sflow specific checks in another function say querySwitchSfloModCapability and use that in querySwitchMirrorOnDropCapability
There was a problem hiding this comment.
Just to confirm — your suggestion is to create a new function querySwitchSflowModCapability() that is called from querySwitchMirrorOnDropCapability(), and move the following HOSTIF-specific checks into it:
- SAI_HOSTIF_USER_DEFINED_TRAP_ATTR_TYPE (enum)
- SAI_HOSTIF_USER_DEFINED_TRAP_TYPE_TAM
- SAI_HOSTIF_USER_DEFINED_TRAP_ATTR_TRAP_GROUP
Then, querySwitchMirrorOnDropCapability() would call querySwitchSflowModCapability(), and if it returns true (along with all required TAM capabilities), it would set SWITCH_CAPABILITY_TABLE_MIRROR_ON_DROP_CAPABLE.
Please confirm if this matches your idea.
There was a problem hiding this comment.
Yes. That is what I was suggesting. I think most of the time we would call specific checks and use that. But it is ok to have an umbrella kind of check like querySwitchMirrorOnDropCapability which would set SWITCH_CAPABILITY_TABLE_MIRROR_ON_DROP_CAPABLE if any of the flavors of MOD are supported.
developfast
left a comment
developfast
left a comment
There was a problem hiding this comment.
thanks for the pr :) i left a few comments
| } | ||
|
|
||
| // Reenable drop monitor when rate limit is changed | ||
| disableDropMonitor(); |
There was a problem hiding this comment.
I see a potential memory leak if disableDropMonitor() fails to fully clean up, the recursive call to enableDropMonitor() will attempt to initialize with potentially inconsistent state. The cleanupDropMonitor() is called, but there's no verification that all resources were actually freed. Please add some explicit state validation like this:
if (!disableDropMonitor())
{
SWSS_LOG_ERROR("Failed to disable drop monitor before changing rate");
return false;
}
if (m_tam != SAI_NULL_OBJECT_ID || m_tamEvent != SAI_NULL_OBJECT_ID... // how many ever conditions
There was a problem hiding this comment.
Thanks for the suggestion. Agree — avoiding the recursive call makes sense. I will update the logic so that if disableDropMonitor() fails, the function returns immediately.
There was a problem hiding this comment.
Update: instead of returning early on a failed disableDropMonitor(), aaacf87 removes the disable/re-init path from rate changes entirely. When MOD is already enabled, enableDropMonitor() now updates CBS/CIR on the existing policer in place, so a rate change never tears down the MOD objects.
|
|
||
| bool SflowDropMonitor::initializeDropMonitor(int32_t limit_rate) | ||
| { | ||
| return (createTamReport() && |
There was a problem hiding this comment.
I am curious what happens in the incomplete cleanup scenario here. If any step fails (e.g., createTamCollector() fails), the function returns false immediately, but cleanupDropMonitor() is only called in enableDropMonitor(). However, cleanupDropMonitor() calls deletion functions in a fixed order that may not match what was actually created. For example, if createTamCollector() fails, we've already created TamReport, TamEventAction, TamTransport, Policer, HostifTrapGroup, and HostifUserDefinedTrap but we haven't set up the dependency chain properly. You may want to add logging to see which step failed for instance.
There was a problem hiding this comment.
The cleanup sequence is the exact reverse order of the creation sequence, and each remove operation checks whether the corresponding object was successfully created before attempting to delete it.
So in the scenario where createTamCollector() fails, calling cleanupDropMonitor() will only remove the objects that were actually created.
Additionally, each remove function already includes logging for any failures during deletion, so incomplete cleanup cases should be visible through those logs.
| } | ||
| } | ||
|
|
||
| m_sflowStatus = sflow_status; |
There was a problem hiding this comment.
if enableDropMonitor() or disableDropMonitor() fails, m_sflowStatus is still updated, causing state inconsistency. The actual drop monitor state might not match m_sflowStatus. You should check return values and only update state on success.
There was a problem hiding this comment.
Thanks for pointing this out. I will update the logic so that m_sflowStatus is only updated when the operation succeeds.
| extern PortsOrch* gPortsOrch; | ||
|
|
||
| // TODO: Add the value to copp_cfg.j2 | ||
| #define SFLOW_DROP_MONITOR_CPU_QUEUE 47 |
There was a problem hiding this comment.
can you add a comment on why 47 is chosen here?
There was a problem hiding this comment.
The value 47 is the last CPU queue on Broadcom ASICs.
We selected the last queue to avoid any potential conflicts with the queues already configured in copp_cfg.j2.
There was a problem hiding this comment.
Please make this a config file where vendor specific queue number can be given like copp_cfg.j2
| if (status != SAI_STATUS_SUCCESS) | ||
| { | ||
| SWSS_LOG_ERROR("Failed to disable drop monitor when unbinding the TAM object from switch, rv:%d", status); | ||
| return false; |
There was a problem hiding this comment.
if unbinding fails, we don't cleanup the resources. This leaves the system in a partially disabled state. Like this:
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to unbind TAM from switch, rv:%d, attempting cleanup anyway", status);
// Fall through to cleanup
}
cleanupDropMonitor();
return (status == SAI_STATUS_SUCCESS);
There was a problem hiding this comment.
This is difficult to handle cleanly.
If the set operation fails, calling cleanupDropMonitor() afterward still won’t be able to delete the objects. The reason is that each SAI object maintains a reference counter, and a failed unbind/set operation means the reference counter is not decremented. As a result, the objects remain referenced and cannot be removed by the cleanup path.
| } | ||
|
|
||
| // Check capability for SAI_HOSTIF_USER_DEFINED_TRAP_ATTR_TRAP_GROUP | ||
| status = sai_query_attribute_capability(gSwitchId, SAI_OBJECT_TYPE_HOSTIF, |
There was a problem hiding this comment.
is this the correct attribute? Using SAI_OBJECT_TYPE_HOSTIF instead of SAI_OBJECT_TYPE_HOSTIF_USER_DEFINED_TRAP?
There was a problem hiding this comment.
Yes, SAI_OBJECT_TYPE_HOSTIF_USER_DEFINED_TRAP is correct. I will update it.
| } | ||
|
|
||
| /* Test enabling/disabling SFLOW drop monitor */ | ||
| TEST_F(SflowOrchTest, SflowDropMonitorEnableDisable) |
There was a problem hiding this comment.
test cases are good but would suggest adding some negative test cases for robustness. For instance, the tests don't verify error conditions like:
- what happens when sai calls fail
- partial initialization failures
- state after failed operations
There was a problem hiding this comment.
Added: SAI create failure at each initialization step, bind/unbind and remove failures, policer set failure, invalid limit and config parse errors. Each test checks the resulting state and, via an object tracker, that no SAI object is leaked.
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@yehjunying gentle remainder, do you have any outstanding comments to address? Is it ready to merge? |
No, I think it is ready to merge. |
|
Retrying failed(or canceled) stages in build 1170615: ✅Stage TestAsan:
✅Stage Test:
|
|
This |
Signed-off-by: junying_yeh <junying_yeh@edge-core.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
orchagent/switchorch.cpp:2101
- The warning messages in this HOSTIF user-defined trap capability check incorrectly mention "TAM Event", which makes troubleshooting confusing when this query fails.
SWSS_LOG_WARN("Could not query TAM Event capability %d", status);
}
else
{
SWSS_LOG_WARN("TAM Event capability not implemented");
orchagent/sfloworch.cpp:582
- removeHostifTrapGroup() logs an error when the trap group was never created (SAI_NULL_OBJECT_ID) but still returns success. This will generate spurious error logs during partial-init cleanup paths.
if (m_hostifTrapGroup == SAI_NULL_OBJECT_ID)
{
SWSS_LOG_ERROR("The HOSTIF trap group does not exist or has already been removed.");
return true;
}
orchagent/sfloworch.cpp:1115
- sflowStatusSet() only updates m_sflowStatus when the drop-monitor operation succeeds. If enabling/disabling MOD fails, this can incorrectly prevent sFlow itself from being enabled/disabled (doTask() returns early when m_sflowStatus is false), contradicting the goal of keeping existing sFlow behavior unaffected.
if (is_succ)
{
m_sflowStatus = sflow_status;
}
orchagent/switchorch.cpp:2074
- Capability query uses SAI_TAM_ATTR_EVENT_OBJECTS_LIST but passes SAI_OBJECT_TYPE_TAM_EVENT; this attribute belongs to the TAM object, so the query will likely fail and incorrectly mark MIRROR_ON_DROP_CAPABLE as false.
This issue also appears on line 2097 of the same file.
// Check capability for SAI_TAM_ATTR_EVENT_OBJECTS_LIST
status = sai_query_attribute_capability(gSwitchId, SAI_OBJECT_TYPE_TAM_EVENT,
SAI_TAM_ATTR_EVENT_OBJECTS_LIST, &capability);
orchagent/sfloworch.cpp:88
- When unbinding SAI_SWITCH_ATTR_TAM_OBJECT_ID, objlist.count is set to 0 but objlist.list is set to a non-null pointer. Other code in this repo leaves list unset/null when count=0; passing a non-null list with count=0 can be rejected by some SAI implementations.
This issue also appears on line 1112 of the same file.
attr.id = SAI_SWITCH_ATTR_TAM_OBJECT_ID;
attr.value.objlist.count = 0;
attr.value.objlist.list = &null_oid;
status = sai_switch_api->set_switch_attribute(gSwitchId, &attr);
Signed-off-by: junying_yeh <junying_yeh@edge-core.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
orchagent/switchorch.cpp:2004
- querySwitchMirrorOnDropCapability() repeats the same unchecked meta->enummetadata dereference for HOSTIF_USER_DEFINED_TRAP_ATTR_TYPE. Add the same guard here to avoid a potential null dereference.
values_list.resize(meta->enummetadata->valuescount);
values.count = static_cast<uint32_t>(values_list.size());
values.list = values_list.data();
orchagent/sfloworch.cpp:1073
- sflowStatusSet() initializes sflow_status and drop_monitor_limit to false/0, so a SET that only updates one field (e.g., sample_rate) will implicitly disable sFlow and/or MOD. Initialize these variables from the current member state so unspecified fields are preserved.
bool sflow_status = false;
int32_t sflow_drop_monitor_limit = 0;
orchagent/sfloworch.cpp:1116
- m_sflowStatus is only updated when the drop-monitor operation succeeds. This couples core sFlow admin state to optional MOD enable/disable, so a MOD bind/unbind failure can prevent applying the requested sFlow state. Update m_sflowStatus independently, and treat MOD failures as best-effort (log and keep MOD disabled/enabled as appropriate).
// Enable, disable or change drop monitor limit when configuration changes
if (m_sflowStatus != sflow_status ||
m_sflowDropMonitor.getLimitRate() != sflow_drop_monitor_limit)
{
bool is_succ = true;
// Drop monitor only enabled when sFlow is enabled
if (sflow_status && sflow_drop_monitor_limit > 0)
{
is_succ = m_sflowDropMonitor.enableDropMonitor(sflow_drop_monitor_limit);
}
else
{
is_succ = m_sflowDropMonitor.disableDropMonitor();
}
if (is_succ)
{
m_sflowStatus = sflow_status;
}
}
orchagent/sfloworch.cpp:88
- When unbinding SAI_SWITCH_ATTR_TAM_OBJECT_ID, objlist.count is set to 0 but objlist.list is set to a non-null pointer. For SAI object lists, count=0 should use a nullptr list to avoid undefined behavior in implementations that validate the pointer/count pair.
attr.id = SAI_SWITCH_ATTR_TAM_OBJECT_ID;
attr.value.objlist.count = 0;
attr.value.objlist.list = &null_oid;
status = sai_switch_api->set_switch_attribute(gSwitchId, &attr);
orchagent/sfloworch.cpp:136
- createTamReport() returns false when the report is already created, but does so silently. This makes initialization failures much harder to debug if state becomes inconsistent (e.g., partial cleanup failures). Log an error before returning.
if (m_tamReport != SAI_NULL_OBJECT_ID)
{
return false;
}
orchagent/sfloworch.cpp:582
- removeHostifTrapGroup() logs an ERROR when the trap group OID is null, but this is a normal condition during cleanup after partial initialization failures. This will generate noisy/error-level logs for expected control flow; return true silently when the object doesn't exist.
if (m_hostifTrapGroup == SAI_NULL_OBJECT_ID)
{
SWSS_LOG_ERROR("The HOSTIF trap group does not exist or has already been removed.");
return true;
}
| vector<int32_t> values_list(meta->enummetadata->valuescount); | ||
| sai_s32_list_t values; | ||
| values.count = static_cast<uint32_t>(values_list.size()); | ||
| values.list = values_list.data(); |
Signed-off-by: junying_yeh <junying_yeh@edge-core.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
orchagent/switchorch.cpp:1994
- The second metadata lookup in querySwitchMirrorOnDropCapability() (SAI_HOSTIF_USER_DEFINED_TRAP_ATTR_TYPE) also dereferences meta->enummetadata without checking meta->isenum / enummetadata. Add the same guard here to avoid potential null dereference on unexpected metadata.
// Check enum value of SAI_HOSTIF_USER_DEFINED_TRAP_ATTR_TYPE
meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_HOSTIF_USER_DEFINED_TRAP,
SAI_HOSTIF_USER_DEFINED_TRAP_ATTR_TYPE);
if (meta == nullptr)
{
orchagent/sfloworch.cpp:89
- disableDropMonitor() unbinds TAM from the switch with objlist.count = 0 but still provides a non-null objlist.list pointer. For SAI object lists, list should be nullptr when count is 0; passing a pointer can violate the API contract and cause undefined behavior depending on the vendor implementation.
attr.id = SAI_SWITCH_ATTR_TAM_OBJECT_ID;
attr.value.objlist.count = 0;
attr.value.objlist.list = &null_oid;
status = sai_switch_api->set_switch_attribute(gSwitchId, &attr);
if (status != SAI_STATUS_SUCCESS)
orchagent/sfloworch.cpp:582
- removeHostifTrapGroup() logs an ERROR when the trap group object id is null, but then returns true. This creates misleading error logs during expected cleanup paths (e.g., partial initialization failure) and makes diagnosing real failures harder. Either treat this as a normal no-op (no log) or log at a non-error level.
if (m_hostifTrapGroup == SAI_NULL_OBJECT_ID)
{
SWSS_LOG_ERROR("The HOSTIF trap group does not exist or has already been removed.");
return true;
}
orchagent/switchorch.cpp:1954
- querySwitchMirrorOnDropCapability() dereferences meta->enummetadata without verifying the attribute is an enum (meta->isenum) and that enummetadata is present. Elsewhere in this file (e.g., ordered_ecmp handling) this check is performed before using enummetadata. Without it, unexpected metadata could cause a null dereference.
This issue also appears on line 1990 of the same file.
// Check enum value of SAI_TAM_EVENT_ATTR_TYPE
const auto* meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_TAM_EVENT,
SAI_TAM_EVENT_ATTR_TYPE);
if (meta == nullptr)
{
| if (m_sflowStatus != sflow_status || | ||
| m_sflowDropMonitor.getLimitRate() != sflow_drop_monitor_limit) | ||
| { | ||
| bool is_succ = true; | ||
|
|
||
| // Drop monitor only enabled when sFlow is enabled | ||
| if (sflow_status && sflow_drop_monitor_limit > 0) | ||
| { | ||
| is_succ = m_sflowDropMonitor.enableDropMonitor(sflow_drop_monitor_limit); | ||
| } | ||
| else | ||
| { | ||
| is_succ = m_sflowDropMonitor.disableDropMonitor(); | ||
| } | ||
|
|
||
| if (is_succ) | ||
| { | ||
| m_sflowStatus = sflow_status; | ||
| } | ||
| } |
| static bool getSflowDropMonitorStatusEnable(SflowOrch &obj) | ||
| { | ||
| return obj.m_sflowDropMonitor.m_enable; | ||
| } | ||
|
|
||
| static int32_t getSflowDropMonitorLimitRate(SflowOrch &obj) | ||
| { | ||
| return obj.m_sflowDropMonitor.m_limitRate; | ||
| } |
|
@prsunny, @madhupalu: the coverage issue has been fixed. |
|
@yehjunying do you have any pending changes to commit? |
|
there is no point of addressing copilot comment, also request to merge the changes. |
|
@prsunny no more outstanding review comments to address. Can you merge ? Thx |
Signed-off-by: junying_yeh <junying_yeh@edge-core.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
What I did
Extended sflowOrch to support Dropped Packet Notification (Mirror on Drop - MOD).
Implemented to manage MOD activation and sampling rates via the SFLOW_TABLE.
Why I did it
This enhancement enables the system to report the packet header, ingress port, and specific drop reason for each packet discarded by the network device (e.g., due to buffer congestion). This provides critical visibility into packet loss that was previously unavailable in standard sFlow sampling.
How I verified it
I implemented and executed integrated unit tests to verify that:
Details if related
HLD: sonic-net/SONiC#1786