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

feat(evpn): complete fastpath implementation of new arch #384

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

atulpatel261194
Copy link
Contributor

@atulpatel261194 atulpatel261194 commented May 16, 2024

Introducing the complete implementation for fastpath based on the new opi-evpn-br architecture that has been presented to opi-tsc in 15 Feb 2024 link

This PR includes:

This PR has dependancy on Netlink Watcher( #385) , Intel Vendor Plugin(#386). Once these 2 PRs ( #385 , #386 ) get merged, We will rebase current PR (#384) to main.
The Current PR (#384) Will be the Last PR to be merged.

Co-authored-by: Dimitrios Markou <dimitrios.markou@ericsson.com>
Co-authored-by: Saikumar Banoth <banoth.saikumar@intel.com>
Co-authored-by: Patel Atul <Atul.patel@intel.com>
Co-authored-by: Vemula Venkatesh <venkatesh.vemula@intel.com>
Co-authored-by: Jambekar Vishakha <vishakha.jambekar@intel.com>
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
@mardim91
Copy link
Contributor

Hello @atulpatel261194 can you please update the description to this one ?

Introducing the complete implementation for fastpath based on the new opi-evpn-br architecture that has been presented to opi-tsc in 15 Feb 2024 link

This PR includes:

Modular based implementation according to the architecture
NetlinkWatcher module
intel e2000 vendor module

Copy link
Contributor

@artek-koltun artek-koltun left a comment

Choose a reason for hiding this comment

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

My main question, if it is possible to split the PR to smaller chunks to ease the review process. I see that we can at least organize as separate PRs our preparation steps:

  • add of DeInit functionality
  • some patch to EventBus with subscribe/unsubscribe
  • netlink watcher,
  • ...
    and finally:
  • e2000 plugin

@mardim91
Copy link
Contributor

My main question, if it is possible to split the PR to smaller chunks to ease the review process. I see that we can at least organize as separate PRs our preparation steps:

  • add of DeInit functionality
  • some patch to EventBus with subscribe/unsubscribe
  • netlink watcher,
  • ...
    and finally:
  • e2000 plugin

Hello @artek-koltun,

The plan that has been presented and agreed by the TSC is to have the two big PRs (drop-0.5 and drop-1.0) first and then for any further development we can make smaller PRs. We have developed the code according that plan. However I understand that it is difficult to review such big PRs in the smallest detail so I think I can discuss with the team to split the drop-1.0 PR in 3 smaller chunks (netlinkWatcher, e2000 plugin and the rest of the code) just to make the review process easier but I cannot go further than that. Of course this will make the testing and bug fixing really hard for us but let me discuss with the team first. If you are ok with that split then maybe we can do it.

Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 0.24125% with 2481 lines in your changes missing coverage. Please review.

Project coverage is 13.54%. Comparing base (5440fb9) to head (dbd0624).
Report is 19 commits behind head on main.

Current head dbd0624 differs from pull request most recent head e82f7f6

Please upload reports for the commit e82f7f6 to get more accurate results.

Files Patch % Lines
pkg/netlink/netlink_watcher.go 0.00% 1270 Missing ⚠️
...ins/intel-e2000/p4runtime/p4translation/p4trans.go 0.00% 854 Missing ⚠️
...plugins/intel-e2000/p4runtime/p4driverapi/p4ctl.go 0.00% 180 Missing ⚠️
cmd/main.go 0.00% 42 Missing ⚠️
pkg/netlink/eventbus/eventbus.go 0.00% 36 Missing ⚠️
pkg/LinuxGeneralModule/lgm.go 0.00% 33 Missing ⚠️
pkg/utils/helpers.go 0.00% 27 Missing ⚠️
pkg/frr/frr.go 0.00% 17 Missing ⚠️
...g/infradb/subscriberframework/eventbus/eventbus.go 0.00% 13 Missing ⚠️
pkg/LinuxCIModule/lci.go 0.00% 5 Missing ⚠️
... and 1 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #384       +/-   ##
===========================================
- Coverage   50.77%   13.54%   -37.23%     
===========================================
  Files          37       49       +12     
  Lines        2525    10252     +7727     
===========================================
+ Hits         1282     1389      +107     
- Misses       1114     8695     +7581     
- Partials      129      168       +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
pkg/netlink/netlink_watcher.go Fixed Show fixed Hide fixed
pkg/netlink/netlink_watcher.go Fixed Show fixed Hide fixed
@Inbanoth Inbanoth force-pushed the drop1.0 branch 2 times, most recently from 23b91ca to 460a6fa Compare June 13, 2024 12:37
Signed-off-by: Saikumar, Banoth <banoth.saikumar@intel.com>
Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

no vendor code in OPI, please

@glimchb glimchb added the invalid This doesn't seem right label Jun 13, 2024
Signed-off-by: Saikumar, Banoth <banoth.saikumar@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants