Skip to content

[FlexCounter] Add trap flow counter support#954

Merged
prsunny merged 6 commits intosonic-net:masterfrom
Junchao-Mellanox:flow-counter-review
Nov 23, 2021
Merged

[FlexCounter] Add trap flow counter support#954
prsunny merged 6 commits intosonic-net:masterfrom
Junchao-Mellanox:flow-counter-review

Conversation

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor

What I did

Add trap flow counter support. See HLD: sonic-net/SONiC#858

Why I did it

Flow counters are usually used for debugging, troubleshooting and performance enhancement processes. Host interface trap counter can get number of received traps per Trap ID.

How I verified it

Manual test/VS test/sonic mgmt test

Details if related

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

Build dependency: sonic-net/sonic-swss-common#534

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

Hi @qiluo-msft , could you please suggest a reviewer?

kcudnik
kcudnik previously approved these changes Oct 20, 2021
@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

Hi @kcudnik ,

It seems the build is timeout, could you help check the issue? I suppose it is not related to the code change.

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

kcudnik
kcudnik previously approved these changes Nov 2, 2021
@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Copy Markdown
Collaborator

/azp run Azure.sonic-sairedis

@kcudnik
Copy link
Copy Markdown
Collaborator

kcudnik commented Nov 8, 2021

please resolve conflicts after merging ACL counters

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

Hi @kcudnik , the failure "Cancelled after 180m" is not caused by my change. I see that the step "Install dependencies" takes almost 3hrs.

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@kcudnik
Copy link
Copy Markdown
Collaborator

kcudnik commented Nov 15, 2021

why you need mockable interface at all? and not use "-fno-access-control" to have access to private methods and also please dont put code in header files

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

why you need mockable interface at all? and not use "-fno-access-control" to have access to private methods and also please dont put code in header files

Hi @kcudnik ,

  1. Why we need mockable interface? I don't understand here. Say we have SaiInterface::func, we want to mock it in test case A and test case B, and we want it behave differently in A and B. How can we achieve this without mockable interface?
  2. I will move the code from header to cpp file.

@kcudnik
Copy link
Copy Markdown
Collaborator

kcudnik commented Nov 16, 2021

in my understanding Mock class should be only wrapper to expose methods/variables for which we don't have have access since they are ether protected or private, and testing them directly would be much simpler. Behavior should be the same since mock should be created for specific class ?, it could be for base interface also i guess

@Junchao-Mellanox
Copy link
Copy Markdown
Contributor Author

Maybe "-fno-access-control" can do the job. But MockableInterface provide following capability:

  1. Mock any SaiInterface members in any test case with different implementation. E.g. the function getStatExt can fill different counter value if need.
  2. It is easy to mock negative case. E.g, the function create can return SAI_INVALID_PARAMTER if need.
  3. The implementation of the mocked function can be change dynamically without affecting any other test case.

@prsunny prsunny merged commit a7c8cfa into sonic-net:master Nov 23, 2021
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
* [FlexCounter] Add trap flow counter support
* Flow counters are usually used for debugging, troubleshooting and performance enhancement processes. Host interface trap counter can get number of received traps per Trap ID.
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.

5 participants