Adding logic to restore networks in order #2571

abhi · 2018-03-23T20:16:12Z

This commits adds a fix for restore case where there might a mix of allocated and unallocated
network in raft. During restore the allocator was going over the networks lexicographically which would mean that there might be a chance for an unallocated network say net1 o be allocated the same vxlan id or subnet pool that was allocated to another networki net2. Because of this during restoring, when allocator tries to allocate the reallocate network net2, it would fail because it allocated network resources to net1 during restore. This would mean services,tasks and network itself would be in a
messed up state.
This commit also contains unit test and some test case modification that was not catching the issue before.

Signed-off-by: Abhinandan [email protected]

abhi · 2018-03-23T20:20:24Z

cc @dperny @anshulpundir @fcrisciani @nishanttotla

fcrisciani · 2018-03-23T21:20:26Z

manager/allocator/network.go

+			continue
+		}
+
+		if existingOnly &&


can we add a comment here stating the importance that both DriverState and IPAM are written together, else the restore logic will fail again

fcrisciani · 2018-03-23T21:20:40Z

manager/allocator/cnmallocator/networkallocator.go

@@ -309,8 +309,10 @@ func (na *cnmNetworkAllocator) DeallocateService(s *api.Service) error {

 // IsAllocated returns if the passed network has been allocated or not.
 func (na *cnmNetworkAllocator) IsAllocated(n *api.Network) bool {
-	_, ok := na.networks[n.ID]
-	return ok
+	if _, ok := na.networks[n.ID]; !ok {


this does not look needed as a change

yeah this isn't needed, this is the same as return ok.

right right. I had added some more logic to check for allocation. I removed it and did not revert this.

fcrisciani · 2018-03-23T21:31:54Z

manager/allocator/network.go

+
+	if err := a.store.Batch(func(batch *store.Batch) error {
+		for _, n := range allocatedNetworks {
+			if err := a.commitAllocatedNetwork(ctx, batch, n); err != nil {


maybe we can add here a check that a network to be committed must have IPAM and DriverState both != nil, this will put more guarantees on the previous check

I am going skip this coz its used in 2 places where the networks are "rightly" allocated now.

codecov · 2018-03-23T21:56:45Z

Codecov Report

Merging #2571 into master will increase coverage by 0.06%.
The diff coverage is 62.85%.

@@            Coverage Diff             @@
##           master    #2571      +/-   ##
==========================================
+ Coverage   61.42%   61.48%   +0.06%     
==========================================
  Files         134      134              
  Lines       21722    21817      +95     
==========================================
+ Hits        13342    13414      +72     
- Misses       6945     6946       +1     
- Partials     1435     1457      +22

dperny · 2018-03-26T21:30:52Z

LGTM

anshulpundir

an unallocated network say net1 o be allocated the same vxlan id or subnet pool that was allocated to another networki net2. Because of this during restoring, when allocator tries to allocate the reallocate network net2, it would fail because it allocated network resources to net1 during restore. This would mean services,tasks and network itself would be in a
messed up state.

For the unallocated net1, is there no way to check that the vxlan id or subnet pool being assigned was already assigned to net2 and avoid re-assigning it ? @abhi

anshulpundir · 2018-03-26T23:18:33Z

manager/allocator/network.go

@@ -560,6 +533,59 @@ func (a *Allocator) deallocateNode(node *api.Node) error {
 	return nil
 }

+// allocateNetworks allocates networks in the store so far before we process


store so far before we process watched events

Sorry, not sure what you mean by this.

Also, please add a comment for how the existingOnly flag is used.

anshulpundir · 2018-03-26T23:19:55Z

manager/allocator/network.go

@@ -165,6 +135,10 @@ func (a *Allocator) doNetworkInit(ctx context.Context) (err error) {
 		return err
 	}

+	if err := a.allocateNetworks(ctx, false); err != nil {


Please add a comment on why we do it in this order i.e. existingOnly=true first and existingOnly=false later.

The comments are added in detail in allocateNetworks and the usage of existingOnly flags

abhi · 2018-03-26T23:44:10Z

@anshulpundir We can check and it will be a O(n^2) op and its not needed. In this case we only process networks once.

abhi · 2018-03-26T23:45:28Z

manager/allocator/network.go

+		if nc.nwkAllocator.IsAllocated(n) {
+			continue
+		}
+		// Network is considered allocated only if the DriverState and IPAM are NOT nil.


I have explained it here

abhi · 2018-03-27T16:50:21Z

@anshulpundir elaborated some comments

anshulpundir · 2018-03-27T23:03:42Z

We can check and it will be a O(n^2) op and its not needed. In this case we only process networks once.

Got it thx for clarifying!

anshulpundir · 2018-03-27T23:04:18Z

manager/allocator/network.go

@@ -560,6 +534,60 @@ func (a *Allocator) deallocateNode(node *api.Node) error {
 	return nil
 }

+// allocateNetworks allocates (restores) networks in the store so far before we process
+// watched events. existingOnly flas is set to true to specify if only allocated


flas => flag

anshulpundir · 2018-03-28T00:47:11Z

manager/allocator/network.go

@@ -164,6 +134,11 @@ func (a *Allocator) doNetworkInit(ctx context.Context) (err error) {
 	if err := a.allocateTasks(ctx, true); err != nil {
 		return err
 	}
+	// Now allocated objects that were not previously allocated
+	// but were present in the raft.


nit: Thx for adding this, but it might make sense to add you description from the PR here in some form. Maybe: "we allocate previously unallocated objects after already handling the objects which have been previously partially allocated to first finish allocating those partially allocated objects (to avoid introducing conflicts) and then do the allocations for new objects.

This commits adds a fix for restore case where there might a mix of allocated and unallocated network in raft. During restore the allocator was going over the networks lexicographically which would mean that there might be a chance for an unallocated network say net1 o be allocated the same vxlan id or subnet pool that was allocated to another networki net2. Because of this during restoring, when allocator tries to allocate the reallocate network net2, it would fail because it allocated network resources to net1 during restore. This would mean services,tasks and network itself would be in a messed up state. Signed-off-by: Abhinandan <[email protected]>

fcrisciani reviewed Mar 23, 2018

View reviewed changes

abhi force-pushed the master branch from 954c8bb to 7f75de8 Compare March 23, 2018 21:56

abhi force-pushed the master branch from 7f75de8 to b10b88d Compare March 26, 2018 20:18

abhi force-pushed the master branch from b10b88d to d56c95b Compare March 26, 2018 22:37

anshulpundir reviewed Mar 26, 2018

View reviewed changes

abhi commented Mar 26, 2018

View reviewed changes

abhi force-pushed the master branch 3 times, most recently from 9bdbb7a to 165404f Compare March 27, 2018 16:49

anshulpundir approved these changes Mar 28, 2018

View reviewed changes

abhi force-pushed the master branch from 165404f to 7f9db67 Compare March 28, 2018 21:36

dperny merged commit 831df67 into moby:master Mar 29, 2018

thaJeztah mentioned this pull request Apr 2, 2018

Bump SwarmKit to 831df679a0b8a21b4dccd5791667d030642de7ff moby/moby#36701

Merged

fcrisciani mentioned this pull request Apr 3, 2018

Avoid predefined error log #2561

Merged

thaJeztah mentioned this pull request Apr 19, 2018

[18.03] update swarmkit docker-archive/docker-ce#521

Closed

thaJeztah mentioned this pull request Jun 5, 2018

[18.03] Adding logic to restore networks in order #2656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding logic to restore networks in order #2571

Adding logic to restore networks in order #2571

abhi commented Mar 23, 2018

abhi commented Mar 23, 2018 •

edited

Loading

fcrisciani Mar 23, 2018

fcrisciani Mar 23, 2018

dperny Mar 23, 2018

abhi Mar 23, 2018

fcrisciani Mar 23, 2018

abhi Mar 26, 2018

codecov bot commented Mar 23, 2018 •

edited

Loading

dperny commented Mar 26, 2018

anshulpundir left a comment

anshulpundir Mar 26, 2018

anshulpundir Mar 26, 2018

abhi Mar 26, 2018

abhi commented Mar 26, 2018 •

edited

Loading

abhi Mar 26, 2018

abhi commented Mar 27, 2018

anshulpundir commented Mar 27, 2018

anshulpundir Mar 27, 2018

anshulpundir Mar 28, 2018

Adding logic to restore networks in order #2571

Adding logic to restore networks in order #2571

Conversation

abhi commented Mar 23, 2018

abhi commented Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2018 • edited Loading

Codecov Report

dperny commented Mar 26, 2018

anshulpundir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhi commented Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

abhi commented Mar 27, 2018

anshulpundir commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhi commented Mar 23, 2018 •

edited

Loading

codecov bot commented Mar 23, 2018 •

edited

Loading

abhi commented Mar 26, 2018 •

edited

Loading