[Carry 1534] Improve scalabiltiy of bridge network isolation rules #2117

AkihiroSuda · 2018-03-19T06:52:58Z

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

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

- This reduces complexity from O(N^2) to O(2N) Signed-off-by: Alessandro Boch <aboch@docker.com> Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda · 2018-03-19T06:54:08Z

cc @aboch @selansen
cc @hisaharu

codecov-io · 2018-03-19T07:13:50Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1b91bc9). Click here to learn what that means.
The diff coverage is 54.68%.

@@            Coverage Diff            @@
##             master    #2117   +/-   ##
=========================================
  Coverage          ?   40.41%           
=========================================
  Files             ?      139           
  Lines             ?    22388           
  Branches          ?        0           
=========================================
  Hits              ?     9047           
  Misses            ?    12014           
  Partials          ?     1327

Impacted Files	Coverage Δ
drivers/bridge/setup_ip_tables.go	`57.78% <45.09%> (ø)`
drivers/bridge/bridge.go	`55.11% <92.3%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b91bc9...9be4c10. Read the comment docs.

thaJeztah · 2018-03-19T12:58:29Z

❤️ thanks!

ping @fcrisciani @ctelfer PTAL

ctelfer

Overall this seems like a sensible approach and the patch does a few thing cleaner than the original code. So far I only one thing that probably needs to change. (see other comment)

ctelfer · 2018-03-20T15:02:27Z

drivers/bridge/setup_ip_tables.go

 	if err != nil {
-		return nil, nil, nil, fmt.Errorf("failed to create FILTER isolation chain: %v", err)
+		return nil, nil, nil, nil, fmt.Errorf("failed to create FILTER isolation chain: %v", err)
 	}



If creating isolationChain2 fails, this code will not roll back the creation of isolationChain1. Please add a deferred cleanup function as above w/ the DockerChains for Filter and NAT. Also, although the original code didn't have a cleanup for isolationChain (now isolationChain1), please add one to gracefully handle failure to add the return rule2.

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

fcrisciani

LGTM

ctelfer · 2018-03-26T17:01:02Z

Fixes to address my comments look good. Doing one more pass on a review. Thanks!

ctelfer · 2018-03-26T17:10:15Z

drivers/bridge/bridge.go

-		}
+	// Install the rules to isolate this network against each of the other networks
+	if err := setINC(thisConfig.BridgeName, enable); err != nil {
+		return err
 	}

 	return nil


Hrmm.. I didn't catch this the first time around, but when doing a quick build test locally, my machine flagged something that CI apparently didn't with golint. Maybe I have a different golint version.

drivers/bridge/bridge.go:316:2: redundant if ...; err != nil check, just return error instead. Makefile:109: recipe for target 'lint' failed

It is right: it would be cleaner to change the last few lines to:

return setINC(thisConfig.BridgeName, enable)

It builds fine once that change is made. Sorry, for not catching this when requesting the error handling change.

Agreed @ctelfer , If there is an error anyway we are not printing any error messages inside the if case.

selansen · 2018-03-26T18:23:50Z

drivers/bridge/bridge.go

+	networks        map[string]*bridgeNetwork
+	store           datastore.DataStore
+	nlh             *netlink.Handle
+	configNetwork   sync.Mutex
 	sync.Mutex
 }



What changed in the entire structure ? new change and old attributes looks same to me except "isolationChain1 , isolationChain2"

It's due to go's formatting; it will align all variables, and 1 and 2 make the left column one position wider. If you add ?w=1 to the GitHub URL you'll see the diff without whitespace changes; https://github.com/docker/libnetwork/pull/2117/files?w=1

selansen · 2018-03-26T18:49:31Z

LGTM

selansen · 2018-03-26T18:58:43Z

Thats make it cleaner to view diff. Thanks *@thaJeztah*

…

On Mon, Mar 26, 2018 at 2:56 PM, Sebastiaan van Stijn < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In drivers/bridge/bridge.go <#2117 (comment)>: > - filterChain *iptables.ChainInfo - isolationChain *iptables.ChainInfo - networks map[string]*bridgeNetwork - store datastore.DataStore - nlh *netlink.Handle - configNetwork sync.Mutex + config *configuration + network *bridgeNetwork + natChain *iptables.ChainInfo + filterChain *iptables.ChainInfo + isolationChain1 *iptables.ChainInfo + isolationChain2 *iptables.ChainInfo + networks map[string]*bridgeNetwork + store datastore.DataStore + nlh *netlink.Handle + configNetwork sync.Mutex sync.Mutex } It's due to go's formatting; it will align all variables, and 1 and 2 make the left column one position wider. If you add ?w=1 to the GitHub URL you'll see the diff without whitespace changes; https://github.com/docker/libnetwork/pull/2117/files?w=1 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2117 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKEKn9ewKieEOKdqpbI2Nyj_wp0wyBmPks5tiTnvgaJpZM4Svq1V> .

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda · 2018-04-02T06:52:25Z

fixed lint issue, PTAL

selansen · 2018-04-02T10:37:40Z

LGTM

ctelfer

LGTM Thanks!

fcrisciani

LGTM

fcrisciani · 2018-04-02T17:08:18Z

Thanks @AkihiroSuda

Improve scalabiltiy of bridge network isolation rules

1c73b1c

- This reduces complexity from O(N^2) to O(2N) Signed-off-by: Alessandro Boch <aboch@docker.com> Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda mentioned this pull request Mar 19, 2018

Improve scalabiltiy of bridge network isolation rules #1534

Closed

ctelfer suggested changes Mar 20, 2018

View reviewed changes

bridge: fix handling errors during setupIPChains()

b6503b8

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

fcrisciani approved these changes Mar 26, 2018

View reviewed changes

ctelfer suggested changes Mar 26, 2018

View reviewed changes

selansen reviewed Mar 26, 2018

View reviewed changes

bridge: fix lint issue

9be4c10

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

ctelfer approved these changes Apr 2, 2018

View reviewed changes

fcrisciani approved these changes Apr 2, 2018

View reviewed changes

fcrisciani merged commit 5c1218c into moby:master Apr 2, 2018

AkihiroSuda mentioned this pull request Apr 3, 2018

update libnetwork to improve scalabiltiy of bridge network isolation rules moby/moby#36774

Merged

thaJeztah mentioned this pull request Apr 18, 2018

[18.03] Update libnetwork docker-archive/docker-ce#514

Merged

bephinix mentioned this pull request Aug 13, 2019

Use new isolation scheme robbertkl/docker-ipv6nat#22

Closed

thaJeztah mentioned this pull request Oct 31, 2020

bridge: remove unused "others" argument from isolateNetwork() #2591

Closed

thaJeztah mentioned this pull request Jun 9, 2021

libnetwork/bridge: remove unused "others" argument from isolateNetwork() moby/moby#42503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Carry 1534] Improve scalabiltiy of bridge network isolation rules #2117

[Carry 1534] Improve scalabiltiy of bridge network isolation rules #2117

AkihiroSuda commented Mar 19, 2018

AkihiroSuda commented Mar 19, 2018

codecov-io commented Mar 19, 2018 •

edited

Loading

thaJeztah commented Mar 19, 2018

ctelfer left a comment

ctelfer Mar 20, 2018

AkihiroSuda Mar 22, 2018

fcrisciani left a comment

ctelfer commented Mar 26, 2018

ctelfer Mar 26, 2018

selansen Mar 26, 2018

selansen Mar 26, 2018

thaJeztah Mar 26, 2018

selansen commented Mar 26, 2018

selansen commented Mar 26, 2018 via email

AkihiroSuda commented Apr 2, 2018

selansen commented Apr 2, 2018

ctelfer left a comment

fcrisciani left a comment

fcrisciani commented Apr 2, 2018

[Carry 1534] Improve scalabiltiy of bridge network isolation rules #2117

[Carry 1534] Improve scalabiltiy of bridge network isolation rules #2117

Conversation

AkihiroSuda commented Mar 19, 2018

AkihiroSuda commented Mar 19, 2018

codecov-io commented Mar 19, 2018 • edited Loading

Codecov Report

thaJeztah commented Mar 19, 2018

ctelfer left a comment

Choose a reason for hiding this comment

ctelfer Mar 20, 2018

Choose a reason for hiding this comment

AkihiroSuda Mar 22, 2018

Choose a reason for hiding this comment

fcrisciani left a comment

Choose a reason for hiding this comment

ctelfer commented Mar 26, 2018

ctelfer Mar 26, 2018

Choose a reason for hiding this comment

selansen Mar 26, 2018

Choose a reason for hiding this comment

selansen Mar 26, 2018

Choose a reason for hiding this comment

thaJeztah Mar 26, 2018

Choose a reason for hiding this comment

selansen commented Mar 26, 2018

selansen commented Mar 26, 2018 via email

AkihiroSuda commented Apr 2, 2018

selansen commented Apr 2, 2018

ctelfer left a comment

Choose a reason for hiding this comment

fcrisciani left a comment

Choose a reason for hiding this comment

fcrisciani commented Apr 2, 2018

codecov-io commented Mar 19, 2018 •

edited

Loading