Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling Sflow trap group in CoPP #1256

Closed
wants to merge 1 commit into from

Conversation

dgsudharsan
Copy link
Collaborator

What I did
Enabling SFLOW trap group in COPP json file.

Why I did it
Without the trap group being installed SFLOW will not work on hardware platforms. The bug sonic-net/sonic-buildimage#4386 was raised for the issue.

How I verified it
Added sflow trap in file and verified it gets installed only when 'config sflow enable' command is given.

Details if related
Apart from including sflow trap in the file, the PR contains fix for an iterator issue in copporch. Without the fix the trap groups after sflow trap will not get installed.

Removed the log for task_needed_retry case. This log will flood the logging file and even when set to debug level it would hinder analyzing other debug logs.

@prsunny
Copy link
Collaborator

prsunny commented Apr 13, 2020

@dgsudharsan , can you address this comment from #1252 as this is impacting warmboot scenario?

"Hi @dgsudharsan , in #1256, it only remove the code cause log ERR message. But the rule will be still in copp consumer until enable sflow. I think that will be a problem in warm-reboot check. Why not set the rule directly without waiting user enables sflow?"

@dgsudharsan
Copy link
Collaborator Author

@dgsudharsan , can you address this comment from #1252 as this is impacting warmboot scenario?

"Hi @dgsudharsan , in #1256, it only remove the code cause log ERR message. But the rule will be still in copp consumer until enable sflow. I think that will be a problem in warm-reboot check. Why not set the rule directly without waiting user enables sflow?"

Sure @prsunny . I will check on this.

@@ -451,14 +451,13 @@ bool CoppOrch::removeGenetlinkHostIf(string trap_group_name)
return true;
}

task_process_status CoppOrch::processCoppRule(Consumer& consumer)
task_process_status CoppOrch::processCoppRule(Consumer& consumer, SyncMap::iterator it)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this function only need the "tuple" as input, why we don't change it to:
task_process_status CoppOrch::processCoppRule(KeyOpFieldsValuesTuple tuple)

to make it clearer?

"COPP_TABLE:trap.group.sflow": {
"trap_ids": "sample_packet",
"trap_action":"trap",
"trap_priority":"1",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is taking the same priority as the ip2me? I would think we should make it lower than that, so maybe raise ip2me to "2", and keep this to "1". to keep consistent with the queue, we can make ip2me to queue 2, and sflow to queue 1.

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…mands (sonic-net#1256)

* [connect][sonic-clear] Align the exit code with consutil for line commands
* Fix comments and update document
* Update doc

Signed-off-by: Jing Kan jika@microsoft.com
@dgsudharsan dgsudharsan deleted the sflowcopp branch March 9, 2023 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants