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

change(network): Configurable maximum connections per IP #7013

Merged
merged 4 commits into from
Jun 20, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jun 19, 2023

Motivation

This could be helpful for integration testing.

Solution

  • Adds an optional maximum_connections_per_ip field to zebra-network config
  • Adds the new generated configs to zebrad/tests/common/configs

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@arya2 arya2 added C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup P-Low ❄️ C-testing Category: These are tests labels Jun 19, 2023
@arya2 arya2 requested a review from a team as a code owner June 19, 2023 23:31
@arya2 arya2 self-assigned this Jun 19, 2023
@arya2 arya2 requested a review from a team as a code owner June 19, 2023 23:31
@arya2 arya2 requested review from upbqdn and removed request for a team June 19, 2023 23:31
@github-actions github-actions bot added the C-feature Category: New features label Jun 19, 2023
teor2345
teor2345 previously approved these changes Jun 19, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

We might want to think about how to name the config files when we don't know if the next release will be 1.0.1 or 1.1.0. But that's not an issue for this PR to solve!

teor2345
teor2345 previously approved these changes Jun 20, 2023
@arya2 arya2 force-pushed the add-conns-per-ip-to-config branch from a262bf0 to fa8e14a Compare June 20, 2023 00:54
@arya2 arya2 requested a review from teor2345 June 20, 2023 01:02
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #7013 (fa8e14a) into main (231f5be) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7013      +/-   ##
==========================================
- Coverage   77.44%   77.39%   -0.06%     
==========================================
  Files         310      310              
  Lines       41609    41628      +19     
==========================================
- Hits        32225    32218       -7     
- Misses       9384     9410      +26     

mergify bot added a commit that referenced this pull request Jun 20, 2023
@teor2345
Copy link
Contributor

teor2345 commented Jun 20, 2023

This new config could be a new user-visible feature. So if it merges, that would make the next release a minor version (1.1.0).

This config is likely to be used in test environments. But we've also had production users say that they wanted node replication on internal networks or on the same machine, which would benefit from this config as well.

Just checking to see if anyone wants to call it a debugging feature instead?
If we want to do that, we could add "debug_" to the start of its name, and document that it is developer-focused.

(I don't mind either way, I'm just putting the question out there, so it's decided before the release.)

@mergify mergify bot merged commit b40fc9b into main Jun 20, 2023
@mergify mergify bot deleted the add-conns-per-ip-to-config branch June 20, 2023 05:11
@teor2345 teor2345 mentioned this pull request Jun 28, 2023
44 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement C-feature Category: New features C-testing Category: These are tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants