Global Default Address Pool support #2241

selansen · 2018-07-24T20:02:16Z

This change brings global default address pool feature into
libnetwork. Idea is to reuse same code flow and functions that were
implemented for local scope default address pool.
Function InitNetworks carries most of the changes. local scope default
address pool init should always happen only once. But Global scope
default address pool can be initialized multiple times.

Signed-off-by: selansen elango.siva@docker.com

thaJeztah · 2018-07-26T06:22:59Z

ipams/builtin/builtin_unix.go

 	// defaultAddressPool Stores user configured subnet list
 	defaultAddressPool []*ipamutils.NetworkToSplit
+	// isGlobalScopePool indicates if it is global or local
+	isGlobalScopePool bool


How does this work? How can I set default address pools on the daemon (bridge networks), and global address pools (for swarm)?

fcrisciani · 2018-07-27T00:17:40Z

Dockerfile

 RUN apt-get update && apt-get -y install iptables \
 		protobuf-compiler
-
+RUN go get -d github.com/gogo/protobuf/protoc-gen-gogo && \


code got merged, you can rebase

fcrisciani · 2018-07-27T16:40:27Z

Dockerfile

 FROM golang:1.10.2 as dev
 RUN apt-get update && apt-get -y install iptables \
 		protobuf-compiler
-


just checkout this file, no need to add these changes

fcrisciani · 2018-07-27T17:02:38Z

ipamutils/utils.go


 // InitNetworks initializes the broad network pool and the granular network pool
-func InitNetworks(defaultAddressPool []*NetworkToSplit) {
+func InitNetworks(defaultAddressPool []*NetworkToSplit, isGlobalScope bool) {


does not seem a clean solution this one.
I would create a Reinit function if needed

selansen · 2018-07-28T19:15:08Z

@thaJeztah @fcrisciani @ctelfer As per our discussion I cleaned up the code along with review comments.
Now we have new API which will be used from swarmkit to set global pool address.

fcrisciani · 2018-07-30T16:22:36Z

ipamutils/utils.go

 }

+// ConfigGlobalScopeDefaultNetworks initializes the global scope default network pool
+func ConfigGlobalScopeDefaultNetworks(defaultAddressPool []*NetworkToSplit) {


if you define this function as: func ConfigGlobalScopeDefaultNetworks(defaultAddressPool *NetworkToSplit...) {
You can avoid from other places to call it with nil, but you will be able to call the method as:
ConfigGlobalScopeDefaultNetworks() that is nicer

swarm init is the only place we are going to call. we can reuse the same function for swarm update. We are not going to call from any other place.

sorry I was referring to the fact that there is a nil check at line 54. If that is needed, means that this method can be called with nil, I was just suggesting that in that case, would be nicer to have a method that takes 0 arguments instead of 1 argument set to nil

Added unit test to cover new changes

fcrisciani · 2018-08-09T06:25:23Z

ipamutils/utils.go

+	}
+	mutex.Unlock()
+	if defaultAddressPool != nil {
+		logrus.Infof("Swarm init global scope default address pool succeeded")


fcrisciani · 2018-08-09T06:26:13Z

ipamutils/utils.go

+	}
+	var err error
+	if PredefinedGlobalScopeDefaultNetworks, err = splitNetworks(defaultAddressPool); err != nil {
+		logrus.WithError(err).Error("InitAddressPools failed to initialize the local scope default address pool")


don't we want to return the err to the caller?

This is kind of Init function for swarmKit. We dont return error. Ideally this should never fail. Thats why we just log the error message. All the validations are done at CLI level.

Hrmm.. I'm not sure that I agree in this case or in the InitNetworks case. But especially in this case. The reason is that both functions take potentially erroneous parameters that can come from user input. Even though it should never fail, by not returning an error here we are implicitly forcing the caller to know all the rules for input validation that this library requires. Clearly the CLI and swarmkit should do some input validation. But IMHO, they should only have to do enough to ensure that the input is parses correctly. Things like invalid parameter combinations should really be handled here. This way, if the design or functionality changes, (e.g. becoming more restrictive or less restrictive) the changes don't have to happen in multiple locations in the code for it to validate the input correctly. I'd also think that the caller should be given the option to panic or abort the code entirely if such an error occurs.

I agree with you on caller should get notified. We dont pass user input while calling Libnetwork API the way we used to do. Instead of passing CLI input deep into stack, I was asked to change APIs params such a way that we pass well defined structure. when we receive input from CLI , now the parser does all validation and converts into []IPNet object. So if there is failure in the user input we reject way before it reaches swarmKit or Libnetwork. When we call, Libnetwork Init we dont pass wrong structure from swarmkit. So it is protected at CLI and swarmKit modules. Like I mentioned existing Init APIs also behave in the same way.

fcrisciani · 2018-08-09T06:26:43Z

ipamutils/utils.go

+	if defaultAddressPool == nil {
+		defaultAddressPool = globalScopeDefaultNetworks
+	}
+	var err error


why this err is defined here and not in the next line if not used later?

while re writing , I missed it out. corrected it.

If I move err inside if block, I am getting below error. So moved it back up.
ipamutils/utils.go:60:2: PredefinedGlobalScopeDefaultNetworks declared and not used

Agree. This is one of those ... er ... charming Go scope behaviors with := ... Go will assume that PredefinedGlobalScopeDefaultNetworks is also a local variable in need of fresh declaration. Go allows for partial redeclaration of variables ... except when it doesn't ... by overriding the outer declaration with an inner one.

fcrisciani · 2018-08-09T06:28:13Z

ipamutils/utils.go

 	initNetworksOnce.Do(func() {
-		// error ingnored should never fail
-		PredefinedGranularNetworks, _ = splitNetworks(defaultGranularNetwork)
+		mutex.Lock()


looks like the lock here can be removed and you can directly call the method ConfigGlobalScopeDefaultNetworks

Idea is ConfigGlobalScopeDefaultNetworks is invoked only from swarmkit and init network will be invoked only during Daemon Init. Later if we plan to support swarmkit update , we dont can modify ConfigGlobalScopeDefaultNetworks without worrying about InitNetworks calling this API. would like to keep it that way .

If you want to keep it this way I think you should add a comment explaining why. Otherwise someone may come along later and refactor InitNetworks() to call ConfigGlobalScopeDefaultNetworks() as the logic is (more or less) duplicated.

fcrisciani · 2018-08-09T06:29:00Z

ipamutils/utils.go

+		// error ignored should never fail
+		PredefinedGlobalScopeDefaultNetworks, _ = splitNetworks(globalScopeDefaultNetworks)
+		mutex.Unlock()
 		if defaultAddressPool == nil {


do you want to call this one localAddressPool?

I guess its not required. Daemon Init will be called only once and we will be initializing local pool only one time during Init. So I feel its not required. WDYT?

I think the point was more just for code clarity. We are initializing two default address pools in this function. Renaming the parameter makes it clear that one only can modify the local address pool using InitNetworks().

I agree, renaming the parameter to localAddressPool would make the intent clearer.

ctelfer

Sorry, wrote this in the wrong browser tab. Will review soon. Pls disregard ... github doesn't seem to want to let me delete the comment.

ctelfer · 2018-08-13T17:02:34Z

ipamutils/utils.go

+	}
+	var err error
+	if PredefinedGlobalScopeDefaultNetworks, err = splitNetworks(defaultAddressPool); err != nil {
+		logrus.WithError(err).Error("InitAddressPools failed to initialize the local scope default address pool")


Hrmm.. I'm not sure that I agree in this case or in the InitNetworks case. But especially in this case. The reason is that both functions take potentially erroneous parameters that can come from user input. Even though it should never fail, by not returning an error here we are implicitly forcing the caller to know all the rules for input validation that this library requires. Clearly the CLI and swarmkit should do some input validation. But IMHO, they should only have to do enough to ensure that the input is parses correctly. Things like invalid parameter combinations should really be handled here. This way, if the design or functionality changes, (e.g. becoming more restrictive or less restrictive) the changes don't have to happen in multiple locations in the code for it to validate the input correctly. I'd also think that the caller should be given the option to panic or abort the code entirely if such an error occurs.

ctelfer · 2018-08-13T17:06:27Z

ipamutils/utils_test.go

+
+	assert.Check(t, is.Len(m, 0))
+
+}


nit: probably delete at least one of the extra blank lines here.

ctelfer · 2018-08-13T17:11:31Z

ipamutils/utils.go

+		// error ignored should never fail
+		PredefinedGlobalScopeDefaultNetworks, _ = splitNetworks(globalScopeDefaultNetworks)
+		mutex.Unlock()
 		if defaultAddressPool == nil {


I think the point was more just for code clarity. We are initializing two default address pools in this function. Renaming the parameter makes it clear that one only can modify the local address pool using InitNetworks().

selansen · 2018-08-13T21:02:37Z

@ctelfer somehow I see your comments are duplicated twice in some cases. Not sure why :)

selansen · 2018-08-14T17:14:51Z

Ping @ctelfer @fcrisciani @abhi

euanh · 2018-08-15T12:22:09Z

ipamutils/utils.go

 	Size int    `json:"size"`
 }

 // InitNetworks initializes the broad network pool and the granular network pool


Please change 'broad network' and 'granular network' to 'local scope' and 'global scope' to match the new variable names.

euanh · 2018-08-15T12:22:33Z

ipamutils/utils.go

+		// error ignored should never fail
+		PredefinedGlobalScopeDefaultNetworks, _ = splitNetworks(globalScopeDefaultNetworks)
+		mutex.Unlock()
 		if defaultAddressPool == nil {


I agree, renaming the parameter to localAddressPool would make the intent clearer.

euanh · 2018-08-15T12:50:20Z

ipamutils/utils.go

 	initNetworksOnce.Do(func() {
-		// error ingnored should never fail
-		PredefinedGranularNetworks, _ = splitNetworks(defaultGranularNetwork)
+		mutex.Lock()


If you want to keep it this way I think you should add a comment explaining why. Otherwise someone may come along later and refactor InitNetworks() to call ConfigGlobalScopeDefaultNetworks() as the logic is (more or less) duplicated.

ctelfer · 2018-08-15T13:32:30Z

@selansen yeah, I have been seeing comments duplicated in other PRs as well. Weird.

selansen · 2018-08-15T21:44:38Z

@ctelfer @euanh , refactored init code completely as per our discussion. PTAL. Once we freeze Libnetwork PR, I can go back to swarmkit and update it.

ctelfer

The refactoring generally looks fine. Just a few nits re testing for exceptional conditions. I'll check on this early tomorrow and merge ASAP if we can answer these. BTW, do the unit tests pass cleanly with the changes? (I assume they do, but want to be sure.)

ctelfer · 2018-08-16T04:21:42Z

ipamutils/utils.go

+	var err error
+	if PredefinedGlobalScopeDefaultNetworks, err = splitNetworks(globalScopeDefaultNetworks); err != nil {
+		logrus.WithError(err).Error("InitAddressPools failed to initialize the global scope default address pool")
+	}


Since this default initialization only occurs on compile-time constant input it must always work. Therefore, it should be OK to panic() if some kind of error occurs. This is similar to https://golang.org/pkg/regexp/#MustCompile which panics if a regex is malformed. (again, one invokes this on compile-time regex strings that are known to be good.)

ctelfer · 2018-08-16T04:23:57Z

ipamutils/utils.go

+
+	if defaultAddressPool == nil {
+		defaultAddressPool = globalScopeDefaultNetworks
+	}


Since the globalScopeDefaultNetworks are already installed, why not just do:

if defaultAddressPool == nil { return nil }

at the start of the function? (even before the lock/deferred unlock) Re-invoking configureDefaultNetworks() again on globalScopeDefaultNetworks isn't going to change the configuration.

No. This is one of the issue I found out during testing and added as extra requirement after I & @mark-church discussed.
here is the scenario which I am trying to address

swarm init happens with new "subnet-pool"

tearing down swarm cluster

now bring up swarm init without "subnet-pool" option

we should init global pool back to inbuilt pool as it used to work without this feature.
so I will have to set it back to in built values and re initialize it again. else swam cluster will have old IPs for overlay networks.

ok, got it. leave as is, then.

ctelfer · 2018-08-16T04:25:02Z

ipamutils/utils.go

+		mutex.Lock()
+		return configDefaultNetworks(defaultAddressPool, &PredefinedLocalScopeDefaultNetworks)
+	}
+	return nil


Same could be done here (early return if parameter is nil). I believe this is the preferred form in go. (i.e. keep indentation down by returning early). @abhi please correct me if I'm wrong.

make sense. will change it.

selansen · 2018-08-16T14:19:26Z

CI unit test failed few places after refactor but fixed them all.

selansen · 2018-08-16T14:39:11Z

@ctelfer All set to go. pls let me know if everything looks good to you

ctelfer · 2018-08-16T14:48:01Z

LGTM

The defer mutex.Unlock()s before the mutex.Lock()s looks a little weird to me, but it isn't wrong per se.

euanh · 2018-08-16T14:53:06Z

ipamutils/utils.go

-	})
+func init() {
+	var err error
+	if PredefinedGlobalScopeDefaultNetworks, err = splitNetworks(globalScopeDefaultNetworks); err != nil {


Can't you use configDefaultNetworks() here as well?

This change brings global default address pool feature into libnetwork. Idea is to reuse same code flow and functions that were implemented for local scope default address pool. Function InitNetworks carries most of the changes. local scope default address pool init should always happen only once. But Global scope default address pool can be initialized multiple times. Signed-off-by: selansen <elango.siva@docker.com>

selansen force-pushed the global_add_pool branch from 3f5865d to 53d6c60 Compare July 24, 2018 20:45

selansen changed the title ~~Global Default Address Pool support~~ [WIP]Global Default Address Pool support Jul 25, 2018

selansen force-pushed the global_add_pool branch 2 times, most recently from 7500c9b to 2a40767 Compare July 25, 2018 20:35

thaJeztah reviewed Jul 26, 2018

View reviewed changes

euanh mentioned this pull request Jul 26, 2018

Dockerfile: Install a fixed version of gogoprotobuf #2242

Merged

fcrisciani reviewed Jul 27, 2018

View reviewed changes

selansen force-pushed the global_add_pool branch 2 times, most recently from 1b5e43f to 4b4b319 Compare July 27, 2018 16:14

fcrisciani reviewed Jul 27, 2018

View reviewed changes

selansen force-pushed the global_add_pool branch 3 times, most recently from 10f1257 to 1e555c7 Compare July 28, 2018 16:13

fcrisciani reviewed Jul 30, 2018

View reviewed changes

selansen force-pushed the global_add_pool branch 4 times, most recently from 1ce1f3e to 20e5a92 Compare August 6, 2018 23:48

selansen changed the title ~~[WIP]Global Default Address Pool support~~ Global Default Address Pool support Aug 7, 2018

fcrisciani requested a review from euanh August 9, 2018 06:06

fcrisciani reviewed Aug 9, 2018

View reviewed changes

ctelfer reviewed Aug 10, 2018

View reviewed changes

selansen force-pushed the global_add_pool branch from 20e5a92 to 00d711d Compare August 11, 2018 20:15

selansen mentioned this pull request Aug 11, 2018

Global Default Address Pool feature support moby/moby#37558

Merged

ctelfer reviewed Aug 13, 2018

View reviewed changes

selansen force-pushed the global_add_pool branch 2 times, most recently from c5b9182 to 69adc43 Compare August 14, 2018 03:57

euanh suggested changes Aug 15, 2018

View reviewed changes

selansen force-pushed the global_add_pool branch 4 times, most recently from d4c567d to 600c483 Compare August 15, 2018 21:30

ctelfer suggested changes Aug 16, 2018

View reviewed changes

selansen force-pushed the global_add_pool branch from 600c483 to db6bd79 Compare August 16, 2018 14:18

selansen force-pushed the global_add_pool branch from db6bd79 to fbdfb1f Compare August 16, 2018 14:26

ctelfer approved these changes Aug 16, 2018

View reviewed changes

euanh reviewed Aug 16, 2018

View reviewed changes

selansen force-pushed the global_add_pool branch from fbdfb1f to b7458ad Compare August 16, 2018 15:10

selansen force-pushed the global_add_pool branch from b7458ad to 8d8ab67 Compare August 16, 2018 15:28

euanh approved these changes Aug 16, 2018

View reviewed changes

ctelfer merged commit a6b0e62 into moby:master Aug 16, 2018

mark-church mentioned this pull request Aug 17, 2018

Proposal: provide the ability to black list IP address ranges #1167

Open

fcrisciani mentioned this pull request Nov 5, 2018

Have some way of configuring PredefinedBroadNetworks #1272

Closed

Global Default Address Pool support #2241

Global Default Address Pool support #2241

Uh oh!

Conversation

selansen commented Jul 24, 2018

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 commented Jul 28, 2018

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 11, 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!

selansen Aug 13, 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!

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!

ctelfer left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

selansen commented Aug 13, 2018

Uh oh!

selansen commented Aug 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selansen Aug 11, 2018 •

edited

Loading

selansen Aug 13, 2018 •

edited

Loading

ctelfer left a comment •

edited

Loading