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

[sflow] Fix ansible test problems caused by enabling sflow #1252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[sflow] Fix ansible test problems caused by enabling sflow #1252

wants to merge 1 commit into from

Conversation

fredyu190011
Copy link
Contributor

@fredyu190011 fredyu190011 commented Apr 9, 2020

What I did
To fix the issue that adding sflow packet-sample rule will cause COPP write error log repeatedly.

Why I did it
After adding the sflow packet-sample rule in COPP config, COPP will repeatedly write error log when user changes the config each time. Until sflow enable. The error log will cause loganalyzer of pytest-ansible raise the error then all test will be failed. By SAI definition and sflow HLD, the rule (SAI_HOSTIF_TRAP_TYPE_SAMPLEPACKET) used to create genlink for hsflowd not used to trap packet to CPU. So it can be directly set without waiting sflow enable. (By current code logic it doesn't consider to undo when sflow changed from enable to disable. It shall be not correct.)

How I verified it
Add the rule in copp json file.

  • When sflow is disable, confirm no any packet will be trap to CPU.
  • Confirmed no error log when any config changed.
  • Run COPP ansible and result is passed.
  • Enable sflow global, the packet-sampling is work, colloector recevied sflow packet. Then dislabe, the collector will not received sflow packet.
  • Confirmed interface enable / disable is work. The collector can received sflow packet when enable, and vice versa.
  • Confirmed counter sampling is work too.

Details if related

Signed-off-by: Fred Yu fred_yu@edge-core.com

@msftclas
Copy link

msftclas commented Apr 9, 2020

CLA assistant check
All CLA requirements met.

@prsunny
Copy link
Collaborator

prsunny commented Apr 9, 2020

This affects sflow functionality. We need a proper fix, @dgsudharsan

What I did
To fix the issue that adding sflow packet-sample rule will cause COPP write error log repeatedly.

Why I did it
After adding the sflow packet-sample rule in COPP config, COPP will repeatedly write error log when user changes the config each time. Until sflow enable. The error log will cause loganalyzer of pytest-ansible raise the error then all test will be failed. By SAI definition and sflow HLD, the rule (SAI_HOSTIF_TRAP_TYPE_SAMPLEPACKET) used to create genlink for hsflowd not used to trap packet to CPU. So it can be directly set without waiting sflow enable. (By current code logic it doesn't consider to undo when sflow changed from enable to disable. It shall be not correct.)

How I verified it
Add the rule in copp json file.
* When sflow is disable, confirm no any packet will be trap to CPU.
* Confirmed no error log when any config changed.
* Run COPP ansible and result is passed.
* Enable sflow global, the packet-sampling is work, colloector recevied sflow packet. Then dislabe, the collector will not received sflow packet.
* Confirmed interface enable / disable is work. The collector can received sflow packet when enable, and vice versa.
* Confirmed counter sampling is work too.

Details if related

Signed-off-by: Fred Yu fred_yu@edge-core.com
@fredyu190011
Copy link
Contributor Author

Hi @prsunny @dgsudharsan, could you please help to review this PR?

@dgsudharsan
Copy link
Collaborator

Hi @prsunny @dgsudharsan, could you please help to review this PR?

Hi,
As per discussion with prince i have raised the following pull request to address the problem
#1256

I was in parallel working on the issue when someone pointed out this PR. however since this would break the functionality i have raised the changes which would not impact the functionality.

@fredyu190011
Copy link
Contributor Author

fredyu190011 commented Apr 13, 2020

Hi @dgsudharsan , in #1256, it only remove the code caused logging ERR messages. 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?

@fredyu190011
Copy link
Contributor Author

Hi @dgsudharsan, is there any progress on it?

@dgsudharsan
Copy link
Collaborator

dgsudharsan commented Apr 27, 2020

Hi @dgsudharsan, is there any progress on it?

The fix requires a complete redesign of handling of copp in sonic. The design for the new CoPP has been raised by @prsunny . I am not sure about the timelines for the activity.

@prsunny
Copy link
Collaborator

prsunny commented Apr 27, 2020

@fredyu190011 , can you review sonic-net/SONiC#606

@fredyu190011
Copy link
Contributor Author

Hi @prsunny , so the sflow problem will be reviewed by waiting until copp arch changed?

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

4 participants