Default Address Pool Code Refactor by selansen · Pull Request #2732 · moby/swarmkit

selansen · 2018-08-28T23:05:40Z

We have two params that have been passed around as part of swarm init
config. As part of refactoring the code, we created NetworkConfig struct
and added address pool and subnet mask length as part of the new struct
Older PR review comment has been addressed in this commit.
Signed-off-by: selansen elango.siva@docker.com

- What I did

- How I did it

- How to test it

- Description for the changelog

codecov · 2018-08-29T17:18:55Z

Codecov Report

Merging #2732 into master will increase coverage by 0.03%.
The diff coverage is 51.51%.

@@            Coverage Diff             @@
##           master    #2732      +/-   ##
==========================================
+ Coverage   61.77%   61.81%   +0.03%     
==========================================
  Files         134      134              
  Lines       21888    21832      -56     
==========================================
- Hits        13522    13495      -27     
+ Misses       6903     6881      -22     
+ Partials     1463     1456       -7

anshulpundir · 2018-08-30T21:17:32Z

manager/allocator/network.go

 }

+// NetworkConfig is used to store network related cluster config in the Manager.
+type NetworkConfig struct {


Why do we have two definitions of this?

I had faced cyclic import issue again so I ended up using similar struct in cnmnetworkallocator

I see. We can create a new file for the config and add the struct there. Duplicate definitions is not great for maintainability.

again...we need to create new package for this I guess. looks like placing new file in existing package will lead into same issue.

We'll need to find a way to solve this. As I said, having two definitions is not maintainable.

Can we not just put it in package cnallocator and use it from there?

I thought we are trying to put in a logical place. Let me see if I can move around.

anshulpundir · 2018-08-30T21:19:58Z

manager/allocator/network.go

 func (a *Allocator) doNetworkInit(ctx context.Context) (err error) {
-	na, err := cnmallocator.New(a.pluginGetter, a.defaultAddrPool, a.subnetSize)
+	var netConfig *cnmallocator.NetworkConfig
+	if a.networkConfig != nil && a.networkConfig.DefaultAddrPool != nil {


is the a.networkConfig.DefaultAddrPool != nil check needed?

There was one CI test case was failing due to segment fault . Hence added extra check

Interesting. Do we know why? Is the assignment below doing a dereference?

I saw illegal memory access info and segmentation fault. So i added this extra check. dont remember full details now. I can go back to old code and get full details if you want.

anshulpundir · 2018-08-30T21:20:33Z

manager/controlapi/cluster.go

+		if newCluster.DefaultAddressPool == nil {
+			// This is just for CLI display. Set the inbuilt default pool for
+			// user reference.
+			newCluster.DefaultAddressPool = []string{"10.0.0.0/8"}


lets define constants for these defaults?

ok. will do it

@anshulpundir I can't make slice as constant (go doesnt allow it). But created as regular variable.
I made subnetsize as constant variable. Hope this is fine with you.

manager/manager.go

anshulpundir · 2018-08-30T21:24:40Z

manager/allocator/network.go

+
+	// SubnetSize specifies the subnet size of the networks created from
+	// the default subnet pool
+	SubnetSize uint32


any reason to use int32 instead of int?

This value can never be negative . hence I use uint32. We use uint32 even in storing it in cluster object.

got it. I was also wondering if there is a motivation to use uint32? If not, maybe just use uint?

Yes. int and uint are 64-bit on 64-bit architecture and 32 bit on 32-bit architecture. Hence I wanted to use unit32.

We have two params that have been passed around as part of swarm init config. As part of refactoring the code, we created NetworkConfig struct and added address pool and subnet mask length as part of the new struct Older PR review comment has been addressed in this commit. Signed-off-by: selansen <elango.siva@docker.com>

dperny · 2018-09-10T15:25:13Z

I like that we've factored two fields into 1 config object. However, I don't like that it requires us to import github.com/docker/swarmkit/manager/allocator/cnmallocator everywhere. I think that breaks the separation of concerns, and will make moving to the new allocator more difficult.

This is not a blocker, though. If you think that's the best solution, feel free to disagree, and I'll LGTM.

selansen · 2018-09-10T16:02:59Z

I tried to move around many places and everywhere I had circle import error.
If you think this will affect our new allocator, I can create separate package like Anshul’s alternate suggestion only for this struct. but that doesn't seem to be good design approach ( having one just for a one object) so I told him lets not do it. Any other idea ?

dperny · 2018-09-10T16:06:53Z

Nah, this is fine.

This also brings in these PRs from swarmkit: - moby/swarmkit#2691 - moby/swarmkit#2744 - moby/swarmkit#2732 - moby/swarmkit#2729 - moby/swarmkit#2748 Signed-off-by: Tibor Vass <tibor@docker.com>

This also brings in these PRs from swarmkit: - moby/swarmkit#2691 - moby/swarmkit#2744 - moby/swarmkit#2732 - moby/swarmkit#2729 - moby/swarmkit#2748 Signed-off-by: Tibor Vass <tibor@docker.com> Upstream-commit: cce1763d57b5c8fc446b0863517bb5313e7e53be Component: engine

selansen force-pushed the default_addr_pool branch 3 times, most recently from 11174f0 to 3ef05f3 Compare August 29, 2018 16:55

selansen force-pushed the default_addr_pool branch 2 times, most recently from d434ab0 to de69ddc Compare August 30, 2018 19:19

anshulpundir reviewed Aug 30, 2018

View reviewed changes

selansen force-pushed the default_addr_pool branch 2 times, most recently from 9f4bf58 to eb9ac0b Compare August 31, 2018 16:18

selansen force-pushed the default_addr_pool branch 2 times, most recently from a6d8e98 to 7574634 Compare September 7, 2018 21:51

selansen force-pushed the default_addr_pool branch from 7574634 to e32e960 Compare September 7, 2018 21:53

dperny merged commit d7d23d7 into moby:master Sep 10, 2018

thaJeztah mentioned this pull request Sep 12, 2018

Global Default AddressPool - Update moby/moby#37736

Merged

tiborvass mentioned this pull request Sep 22, 2018

[18.09] Remove boltdb dependency docker-archive/engine#60

Merged

Conversation

selansen commented Aug 28, 2018

Uh oh!

codecov bot commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selansen Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selansen Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dperny commented Sep 10, 2018

Uh oh!

selansen commented Sep 10, 2018

Uh oh!

dperny commented Sep 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 29, 2018 •

edited

Loading

selansen Aug 31, 2018 •

edited

Loading

selansen Aug 31, 2018 •

edited

Loading