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.

Sign up for GitHub

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

Jump to bottom

[cbf] Add class-based forwarding support #1963

Merged
merged 29 commits into from
Nov 16, 2021
Merged

[cbf] Add class-based forwarding support #1963

merged 29 commits into from
Nov 16, 2021

Conversation

TACappleman
Copy link
Contributor

What I did

  • Added CBF NHG support to NhgOrch
  • Added NhgMapOrch to handle DSCP_TO_FC and EXP_TO_FC tables
  • Added UT for the new NhgOrch function and NhgMapOrch

Why I did it

Support sonic-net/SONiC#796

How I verified it

SWSS UTs covering new functionality

@TACappleman TACappleman requested a review from prsunny as a code owner October 15, 2021 16:22
@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2021

This pull request introduces 1 alert when merging cf32453 into f248e26 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@TACappleman
Copy link
Contributor Author

HLD here: sonic-net/SONiC#796

@prsunny
Copy link
Collaborator

prsunny commented Oct 18, 2021

Few high-level comments:

  1. Since you've multiple files for this feature, please move it to a new folder under orchagent
  2. There are some existing files that changed its name or moved code. Please note, we've already reviewed those and it will be hard to re-review the same. Please retain the existing files names and changes and suggest to only add new changes.

@TACappleman
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TACappleman TACappleman mentioned this pull request Oct 19, 2021
Merged
@TACappleman
Copy link
Contributor Author

I've moved most of the new files into a subdirectory. I've reduced changes where sensible so that moved code is as easy to see as possible. But to take advantage of being able to reuse code between the two types of next hop groups, a bunch of it has to move (e.g. to templates)

@lgtm-com
Copy link

lgtm-com bot commented Oct 19, 2021

This pull request introduces 1 alert when merging cd49fff into 6b15584 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@prsunny
Copy link
Collaborator

prsunny commented Oct 19, 2021

I've moved most of the new files into a subdirectory. I've reduced changes where sensible so that moved code is as easy to see as possible. But to take advantage of being able to reuse code between the two types of next hop groups, a bunch of it has to move (e.g. to templates)

This is too much of code churn for this feature. If its new, why modify lots of existing code unless it was not originally designed correctly. IMO, this is re-reviewing the code and not sure if this can be made to 202111

@TACappleman
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TACappleman
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TACappleman
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TACappleman
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TACappleman
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lgtm-com
Copy link

lgtm-com bot commented Oct 22, 2021

This pull request introduces 1 alert when merging e048b3a into d95823d - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@prsunny
Copy link
Collaborator

prsunny commented Oct 23, 2021

Based on discussion:

  1. Note: nhgbase.h and nhgbase.cpp are templatized
  2. Create a new folder named cbf
  3. Change cbfnhghandler to cbfnhgorch
  4. Move cbf*.h/cpp and nhgmap*.h/cpp to cbf
  5. Move nhgbase.* to orchagent/
  6. Retain nhgorch.h/cpp as it is.
  7. Separate out the entry points. APP_NEXTHOP_GROUP_TABLE_NAME -> nhgorch, APP_CLASS_BASED_NEXT_HOP_GROUP_TABLE_NAME -> cbfnhgorch.

@TACappleman
Copy link
Contributor Author

@prsunny I've made those suggested changes.

The changes to the original nhgorch now mostly consist of the following:

  • Code moved to the template classes
  • A few calls to cbfnhgorch when looking up next hop groups
  • Some fields renamed to better reflect the combined code (e.g. next hop group members may now be next hops or next hop groups, so renamed m_nh_key to m_key).

All these changes are now a lot easier to follow in the code, as there's less churn

@TACappleman
Copy link
Contributor Author

/azpw run

arlakshm
arlakshm approved these changes Nov 13, 2021
View reviewed changes
Copy link
Contributor

@arlakshm arlakshm left a comment

Choose a reason for hiding this comment

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

lgtm.

@TACappleman
Copy link
Contributor Author

@prsunny are you happy for this to merge now?

prsunny
prsunny approved these changes Nov 16, 2021
View reviewed changes
Copy link
Collaborator

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

Reviewed common code and lgtm.

@rlhui rlhui merged commit 339101c into sonic-net:master Nov 16, 2021
@stephenxs stephenxs mentioned this pull request Nov 17, 2021
Merged
5 tasks
@abanu-ms abanu-ms deleted the cbf_pr branch December 3, 2021 10:41
TACappleman added a commit to Metaswitch/sonic-swss that referenced this pull request Dec 13, 2021
@TACappleman @abanu-ms
[cbf] Add class-based forwarding support (sonic-net#1963)
f7b8ad8 
* [cbf] Added initial CBF support (#2)

* Added CBF NHG support to NhgOrch
* Added NhgMapOrch to handle DSCP_TO_FC and EXP_TO_FC tables
* Added UT for the new NhgOrch function and NhgMapOrch

Support sonic-net/SONiC#796

Co-authored-by: Alexandru Banu <[email protected]>
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
@dflynn-Nokia
[soft-reboot] Add support for platforms based on Device Tree (sonic-n…
14889ce 
…et#1963)

What I did
Add soft-reboot command support for the armhf and arm64 platforms

How I did it
Add logic to retrieve kernel boot arguments from the Device Tree rather than from the GRUB config for these platforms

How to verify it
Execute the soft-reboot command on an armhf or arm64 platform
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abanu-ms abanu-ms abanu-ms left review comments

@rlhui rlhui rlhui left review comments

@arlakshm arlakshm arlakshm approved these changes

@kperumalbfn kperumalbfn kperumalbfn approved these changes

@prsunny prsunny prsunny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@TACappleman @prsunny @mssonicbld @arlakshm @abanu-ms @rlhui @kperumalbfn