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

send-max: respect configuration #2783

Closed
wants to merge 1 commit into from
Closed

send-max: respect configuration #2783

wants to merge 1 commit into from

Conversation

maxime-peim
Copy link

@maxime-peim maxime-peim commented Mar 15, 2024 •
edited
Loading

  • send first SendMax routes for a given prefix to neighbors
  • when route withdrawal, send the next filtered route
  • add a local identifier to a path (rf:prefix:local id)
  • register which paths are filtered per peer

@maxime-peim maxime-peim force-pushed the routes-limit branch 2 times, most recently from c67d13f to a3cea61 Compare March 15, 2024 14:53
@maxime-peim maxime-peim marked this pull request as ready for review March 15, 2024 14:53
@maxime-peim maxime-peim changed the title prePolicyFilterpath: respect max routes config send-max: respect max routes configuration Mar 16, 2024
@maxime-peim maxime-peim marked this pull request as draft March 16, 2024 11:04
@maxime-peim maxime-peim changed the title send-max: respect max routes configuration send-max: respect configuration Mar 16, 2024
@maxime-peim maxime-peim force-pushed the routes-limit branch 2 times, most recently from 716e64a to acf1783 Compare March 18, 2024 15:29
@maxime-peim maxime-peim marked this pull request as ready for review March 18, 2024 15:29
@maxime-peim maxime-peim force-pushed the routes-limit branch 3 times, most recently from 7add48d to 6c0e9eb Compare March 20, 2024 09:43
@maxime-peim
Copy link
Author

Hi @fujita
I was not able to run zapi test locally, will it be possible to run the CI please?

@fujita
Copy link
Member

fujita commented Mar 20, 2024

I've just run the CI. btw, you can run the CI by forking the gobgp repository and creating a pull request for your repository.

@maxime-peim
Copy link
Author

Thanks!
This is what I have done, but the CI does not seem to be triggered when force-pushing

@maxime-peim
Copy link
Author

Sorry, it should be good now. However, I didn't find a way to re-run the CI from my side apart from closing and reopening the PR...

bayrinat
bayrinat reviewed Mar 27, 2024
View reviewed changes
api/gobgp.proto Outdated
uint32 local_identifier = 19;
bytes nlri_binary = 20;
repeated bytes pattrs_binary = 21;
bool send_max_filtered = 13;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an API breaking change

Copy link
Author

Choose a reason for hiding this comment

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

Should I put the new field at the end of the structure?

Copy link
Author

Choose a reason for hiding this comment

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

done

@maxime-peim maxime-peim requested a review from bayrinat March 28, 2024 08:26
@maxime-peim maxime-peim force-pushed the routes-limit branch 3 times, most recently from 83fe209 to 92353f2 Compare March 29, 2024 10:16
@maxime-peim
Copy link
Author

Hi @fujita @bayrinat,
Do you have comments to do on that PR, please?

@maxime-peim
Copy link
Author

Anyone?

@fujita
Copy link
Member

fujita commented Apr 16, 2024

Thanks a lot for working the feature!
So each peer needs to keep filtered paths in memory. looks like it could consume lots of memory in some configurations. Any other way to implement that feature?

@maxime-peim
Copy link
Author

maxime-peim commented Apr 22, 2024 •
edited
Loading

Thanks a lot for working the feature! So each peer needs to keep filtered paths in memory. looks like it could consume lots of memory in some configurations. Any other way to implement that feature?

Hi @fujita,
Each peer keeps a key to the path that is stored in the local RIB, it does not have a copy of the path.
Maybe we could only store N filtered paths if the limit is set to N. So, we could replace all the advertised paths are withdrawn at once.

@fujita
Copy link
Member

fujita commented Apr 29, 2024

Thanks a lot!

@fujita fujita closed this Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bayrinat bayrinat Awaiting requested review from bayrinat

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

Successfully merging this pull request may close these issues.
3 participants
@maxime-peim @fujita @bayrinat