Default Address Pool Code Refactor #2732
Conversation
selansen
force-pushed
the
default_addr_pool
branch
3 times, most recently
from
11174f0
to
3ef05f3
Compare
Codecov Report
@@ 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 |
selansen
force-pushed
the
default_addr_pool
branch
2 times, most recently
from
d434ab0
to
de69ddc
Compare
manager/allocator/network.go
Outdated
| @@ -67,8 +67,26 @@ type networkContext struct { | |||
| somethingWasDeallocated bool | |||
| } | |||
|
|
|||
| // NetworkConfig is used to store network related cluster config in the Manager. | |||
| type NetworkConfig struct { | |||
There was a problem hiding this comment.
Why do we have two definitions of this?
There was a problem hiding this comment.
I had faced cyclic import issue again so I ended up using similar struct in cnmnetworkallocator
There was a problem hiding this comment.
I see. We can create a new file for the config and add the struct there. Duplicate definitions is not great for maintainability.
There was a problem hiding this comment.
again...we need to create new package for this I guess. looks like placing new file in existing package will lead into same issue.
There was a problem hiding this comment.
We'll need to find a way to solve this. As I said, having two definitions is not maintainable.
There was a problem hiding this comment.
Can we not just put it in package cnallocator and use it from there?
There was a problem hiding this comment.
I thought we are trying to put in a logical place. Let me see if I can move around.
manager/allocator/network.go
Outdated
| 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 { |
There was a problem hiding this comment.
is the a.networkConfig.DefaultAddrPool != nil check needed?
There was a problem hiding this comment.
There was one CI test case was failing due to segment fault . Hence added extra check
There was a problem hiding this comment.
Interesting. Do we know why? Is the assignment below doing a dereference?
There was a problem hiding this comment.
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.
manager/controlapi/cluster.go
Outdated
| 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"} |
There was a problem hiding this comment.
lets define constants for these defaults?
There was a problem hiding this comment.
@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/allocator/network.go
Outdated
|
|
||
| // SubnetSize specifies the subnet size of the networks created from | ||
| // the default subnet pool | ||
| SubnetSize uint32 |
There was a problem hiding this comment.
any reason to use int32 instead of int?
There was a problem hiding this comment.
This value can never be negative . hence I use uint32. We use uint32 even in storing it in cluster object.
There was a problem hiding this comment.
got it. I was also wondering if there is a motivation to use uint32? If not, maybe just use uint?
There was a problem hiding this comment.
Yes. int and uint are 64-bit on 64-bit architecture and 32 bit on 32-bit architecture. Hence I wanted to use unit32.
selansen
force-pushed
the
default_addr_pool
branch
2 times, most recently
from
9f4bf58
to
eb9ac0b
Compare
selansen
force-pushed
the
default_addr_pool
branch
2 times, most recently
from
a6d8e98
to
7574634
Compare
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>
selansen
force-pushed
the
default_addr_pool
branch
from
7574634
to
e32e960
Compare
|
I like that we've factored two fields into 1 config object. However, I don't like that it requires us to import This is not a blocker, though. If you think that's the best solution, feel free to disagree, and I'll LGTM. |
|
I tried to move around many places and everywhere I had circle import error. |
|
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
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