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

Improve scalabiltiy of bridge network isolation rules #1534

Closed
wants to merge 1 commit into from
Closed

Improve scalabiltiy of bridge network isolation rules #1534

wants to merge 1 commit into from

Conversation

aboch
Copy link
Contributor

@aboch aboch commented Nov 2, 2016 •
edited
Loading

  • This reduces complexity from O(N^2) to O(2N)

Related to moby/moby#26435

Reported as example the time measurement for creating 50 bridge networks and for pruning them.
Also because of the removal of the loop in isolateNetwork(), the timing is drastically improved:


   CURRENT		  NEW
Creation of the 50th network:
real	0m3.035s	0m0.146s
user	0m0.004s	0m0.008s
sys	0m0.004s	0m0.000s

Creation of all 50 networks:
real	0m43.785s	0m6.931s
user	0m0.336s	0m0.324s
sys	0m0.084s	0m0.148s

Pruning of the 50 networks:
real	1m2.136s	0m7.342s
user	0m0.004s	0m0.000s
sys	0m0.008s	0m0.016s

@aboch aboch changed the title WIP: Improve scalabiltiy of bridge network isolation rules Improve scalabiltiy of bridge network isolation rules Nov 2, 2016
@aboch aboch changed the title Improve scalabiltiy of bridge network isolation rules WIP: Improve scalabiltiy of bridge network isolation rules Nov 2, 2016
@aboch aboch changed the title WIP: Improve scalabiltiy of bridge network isolation rules Improve scalabiltiy of bridge network isolation rules Nov 7, 2016
mrjana
mrjana suggested changes Nov 8, 2016
View reviewed changes
drivers/bridge/setup_ip_tables.go
actionMsg = "remove"
}

for i := 0; i < 2; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor this to something like this:

for i, chain := range chains {
     iptables.ProgramRule(..., chain, action, rules[i])
    ....
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will change it, thanks.

drivers/bridge/setup_ip_tables.go
IsolationChain = "DOCKER-ISOLATION"
DockerChain = "DOCKER"
IsolationChain = "DOCKER-ISOLATION"
IsolationChain2 = "DOCKER-ISOLATION-REV"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename DOCKER-ISOLATION-REV chain name and IsolationChain2 variable to something more meaningful. Right now no one will understand what they are for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought REV for reverse as it goes had in hand with DOCKER-ISOLATION chain.
One filter by source, the other one by destination.
I chose a short suffix because the chain name is already long.

Would expand it to DOCKER-ISOLATION-REVERSE make it better ?

Or should I change also the existing one and have two different suffixes, like FORWARD and REVERSE or SRC and DST ?

In the meantime I will replace the variable names to something more descriptive. Or I will add a comment.

drivers/bridge/setup_ip_tables.go
if err := iptables.ProgramRule(iptables.Filter, chains[i], action, rules[i]); err != nil {
msg := fmt.Sprintf("unable to %s inter-network communication rule: %v", actionMsg, err)
if enable {
return fmt.Errorf(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you have to rollback the rule #1 if there is a failure in plumbing rule#2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we should. Thanks.

@aboch
Copy link
Contributor Author

aboch commented Nov 8, 2016

Thanks @mrjana for the comments. I think I addressed them. PTAL.
I verified the old chain and jump rule are removed at boot.

mrjana
mrjana approved these changes Nov 16, 2016
View reviewed changes
Copy link
Contributor

@mrjana mrjana left a comment

Choose a reason for hiding this comment

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

LGTM

@aboch
Improve scalabiltiy of bridge network isolation rules
8f70c47 
- This reduces complexity from O(N^2) to O(2N)

Signed-off-by: Alessandro Boch <[email protected]>
@GordonTheTurtle
Copy link

@aboch It has been detected that this issue has not received any activity in over 6 months. Can you please let us know if it is still relevant:

  • For a bug: do you still experience the issue with the latest version?
  • For a feature request: was your request appropriately answered in a later version?

Thank you!
This issue will be automatically closed in 1 week unless it is commented on.
For more information please refer to #1926

@aboch
Copy link
Contributor Author

aboch commented Aug 30, 2017

@aboch It has been detected that this issue has not received any activity in over 6 months. Can you please let us know if it is still relevant:

For a bug: do you still experience the issue with the latest version?

Yes

For a feature request: was your request appropriately answered in a later version?

No

@AkihiroSuda
Copy link
Member

What's current status of this?

@aboch @mrjana

@AkihiroSuda
Copy link
Member

@fcrisciani @ctelfer @thaJeztah

Any chance to get this merged? 🙏

@fcrisciani
Copy link

@AkihiroSuda there is conflicts so we will need someone to take this patch over, will try to see if there is someone able to follow up on this

@thaJeztah
Copy link
Member

@AkihiroSuda are you interested in carrying this?

@AkihiroSuda AkihiroSuda mentioned this pull request Mar 19, 2018
Merged
@AkihiroSuda
Copy link
Member

carried as #2117

@fcrisciani
Copy link

closing because carried in another PR

@fcrisciani fcrisciani closed this Mar 27, 2018
@bephinix bephinix mentioned this pull request Aug 13, 2019
Closed
@thaJeztah thaJeztah mentioned this pull request Oct 31, 2020
Closed
@thaJeztah thaJeztah mentioned this pull request Jun 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrjana mrjana mrjana approved these changes

Assignees

@selansen selansen

Labels
status/2-needs-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@aboch @GordonTheTurtle @AkihiroSuda @fcrisciani @thaJeztah @mrjana @selansen