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

Trap type changes #1258

Merged
merged 3 commits into from Aug 13, 2021
Merged

Conversation

dkourkouzelis
Copy link
Contributor

  • Added UDP port 6784 for BFD and BFDv6 trap types
  • Added new LDP trap type

@kcudnik
Copy link
Collaborator

kcudnik commented Jun 29, 2021

please fix error: WARNING: Word 'LDP' is misspelled or is acronym, add to acronyms.txt? saihostif.h 376: * @brief LDP traffic (TCP src port == 646 or TCP dst port == 646) to local

Diamantis Kourkouzelis added 2 commits June 29, 2021 03:46
- Added UDP port 6784 for BFD and BFDv6 trap types
- Added new LDP trap type

Signed-off-by: Diamantis Kourkouzelis <dkourkou@cisco.com>
Signed-off-by: Diamantis Kourkouzelis <dkourkou@cisco.com>
@rlhui
Copy link
Collaborator

rlhui commented Jul 15, 2021

@rck-innovium - please help review/sign off. Thanks.

inc/saihostif.h Outdated
* router IP address (default packet action is drop)
* @brief BFD traffic (UDP dst port == 3784 or UDP dst port == 4784 or
* UDP dst port == 6784) to local router IP address (default packet
* action is drop)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a new trap type like below instead of modifying the existing traps.

Advantages:

  • Does not change behavior for existing implementations

  • Allows optimization of h/w resources when end user needs only BFD and not micro-BFD.

/* 
* @brief BFD traffic (UDP dst port == 6784) to local
* router IP address (default packet action is drop)
*/
SAI_HOSTIF_TRAP_TYPE_BFD_MICRO,

Similarly for BFDv6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish you had brought that up during the meeting today. I feel like the rest of the community should be aware of this request given that they all know we decided on using the same trap type.

In regards to your points:
a) I see it differently, given that someone will get BFD over LAG for free pretty much and it's a common use case rather than not.
b)If someone doesn't need BFD over LAG then they don't need to implement it, correct? An entry with type+port will have to be added to the hardware anyway, it's not easy to call out what is being optimized.

Perhaps we need someone to decide whether we need another meeting for this to a) rediscuss with the community, b) go with your proposal or c) go with the original one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhui Could you please share your thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good if we can discuss this in the next community meeting.

In most of the TCAM based Host Tables, we would need one ACL entry per L4 port match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes per your recommendation.

Signed-off-by: Diamantis Kourkouzelis <dkourkou@cisco.com>
@anshuv-mfst
Copy link

Hi @kcudnik - could you please help with merge, thanks.

@kcudnik
Copy link
Collaborator

kcudnik commented Aug 3, 2021

Hi @kcudnik - could you please help with merge, thanks.

was that already approved on weekly SAI meeting?

@anshuv-mfst
Copy link

Hi @kcudnik - could you please help with merge, thanks.

was that already approved on weekly SAI meeting?

Hi @kcudnik - yes, the PR is reviewed in SAI subgroup meeting.
Hi @dkourkouzelis - could you please confirm if all review comments are addressed.

Copy link
Contributor

@rck-innovium rck-innovium left a comment

Choose a reason for hiding this comment

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

lgtm

@kcudnik kcudnik merged commit 5164065 into opencomputeproject:master Aug 13, 2021
@dkourkouzelis dkourkouzelis deleted the trap-type-changes branch August 26, 2021 00:36
abanu-ms pushed a commit to Metaswitch/SAI that referenced this pull request Aug 27, 2021
Added UDP port 6784 for BFD and BFDv6 trap types
Added new LDP trap type
abanu-ms pushed a commit to Metaswitch/SAI that referenced this pull request Aug 27, 2021
Added UDP port 6784 for BFD and BFDv6 trap types
Added new LDP trap type
abanu-ms added a commit to Metaswitch/SAI that referenced this pull request Aug 27, 2021
Added UDP port 6784 for BFD and BFDv6 trap types
Added new LDP trap type

Co-authored-by: Diamantis Kourkouzelis <dkourkou@cisco.com>
TACappleman pushed a commit to Metaswitch/SAI that referenced this pull request Oct 4, 2021
Added UDP port 6784 for BFD and BFDv6 trap types
Added new LDP trap type

Co-authored-by: Diamantis Kourkouzelis <dkourkou@cisco.com>
@JafarSeyedi
Copy link

Hi,

with new changes, we have two problems:

  1. when changing copp config file to add isis, the sonic will crash and dockers will stop.
    2, eigrp does not work after these changes. we checked eigrp in sonic with activating it in frr and it was working, but currently, the messages did not pass to frr.

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

6 participants