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

fix(bridge): avoid LinkByName netlink calls #148

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

Conversation

glimchb
Copy link
Member

@glimchb glimchb commented Aug 31, 2023

Main reason is perfoamnce.
If we have the relevant info in the cache/DB,
we could just use it instead of netlink get calls.

Fixes #123

Signed-off-by: Boris Glimcher [email protected]

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #148 (09bf2fb) into main (c1f0da9) will decrease coverage by 0.33%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   75.00%   74.67%   -0.33%     
==========================================
  Files           5        5              
  Lines        1012      999      -13     
==========================================
- Hits          759      746      -13     
  Misses        218      218              
  Partials       35       35              
Files Changed Coverage Δ
pkg/evpn/bridge.go 73.23% <100.00%> (-0.54%) ⬇️
pkg/evpn/svi.go 73.01% <100.00%> (-0.53%) ⬇️
pkg/evpn/vrf.go 75.58% <100.00%> (-0.38%) ⬇️

Copy link
Contributor

@mardim91 mardim91 left a comment

Choose a reason for hiding this comment

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

Some comments

  1. I do not see where the tenantbridgeName variable gets assigned (take its value).
  2. I am not sure if it is the same thing creating a link object by hand compared to returning the link object by using the linkByName function. Maybe the returned object has more attributes that are needed during the subsequent netlink call (just a speculation).
  3. I prefer the LinkByName as this way we are sure that object actually exists on the system and has not been deleted accidentally by a third party process.
  4. I do not believe has a huge performance overhead. WDYT ?

@venkatmahalingam
Copy link
Member

Some comments

  1. I do not see where the tenantbridgeName variable gets assigned (take its value).
  2. I am not sure if it is the same thing creating a link object by hand compared to returning the link object by using the linkByName function. Maybe the returned object has more attributes that are needed during the subsequent netlink call (just a speculation).
  3. I prefer the LinkByName as this way we are sure that object actually exists on the system and has not been deleted accidentally by a third party process.
  4. I do not believe has a huge performance overhead. WDYT ?

I thought querying a Linux kernel every time while we maintain a cache is unnecessary, if there is a possibility of third party process deletion, IMO, that's not a good design/coding practice and we should have a single source of truth.

@mardim91
Copy link
Contributor

mardim91 commented Sep 6, 2023

Some comments

  1. I do not see where the tenantbridgeName variable gets assigned (take its value).
  2. I am not sure if it is the same thing creating a link object by hand compared to returning the link object by using the linkByName function. Maybe the returned object has more attributes that are needed during the subsequent netlink call (just a speculation).
  3. I prefer the LinkByName as this way we are sure that object actually exists on the system and has not been deleted accidentally by a third party process.
  4. I do not believe has a huge performance overhead. WDYT ?

I thought querying a Linux kernel every time while we maintain a cache is unnecessary, if there is a possibility of third party process deletion, IMO, that's not a good design/coding practice and we should have a single source of truth.

Not really sure. To be honest I think is more proper to for me actually get the link object that is returned and use it for deletion than creating a new object with just the name. Also we do no know how the subsequent netlink calls will behave if we do no actually get the real object but we create one by hand. Maybe they will nee some attributed from the object that do no exist and that make them fail.

Also kubernetes CNIs use also the LinkByName approach in order to do further netlink operations check here

Main reason is perfoamnce.
If we have the relevant info in the cache/DB,
we could just use it instead of netlink get calls.

Fixes opiproject#123

Signed-off-by: Boris Glimcher <[email protected]>
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.

consider avoid LinkByName for performance
3 participants