Allow pass global datastore config after boot #908

aboch · 2016-01-28T22:22:07Z

This to allow the controller to update the datastore in the local IPAM driver and overlay driver at runtime, via Controller.ReloadConfiguration()

Signed-off-by: Alessandro Boch aboch@docker.com

mavenugo · 2016-01-28T23:09:15Z

ipam/allocator.go

 	if !ok {
 		return
 	}
 	a.updateBitMasks(aSpace)
+	aSpace.Lock()


can you avoid locking aSpace around CheckConsistency ?

I need to iterate through the address space's map of pools. I don't think I can avoid this lock.
Anyway, the consistency check is run either at allocator creation or during datastore update. There won't be other activity on the datastore during these two phases. I mean to say this locking would not be on the way of some other request.

I still would like to avoid it. We cannot look at today's code and break a practice that we are following across other parts of the code. It will cause unncessary headaches in the long run, because of this particular assumption that it will not cause a deadlock with the current implementation.

Right, the reason I am locking is in fact because I do not want to make any assumptions.
I am missing what problem would cause locking the address space while running CheckConsistency on the bitmask. An address space is tied to a datastore, we need to lock it while we run the consistency check on its subnets.

mavenugo · 2016-02-06T21:24:51Z

drivers/overlay/overlay.go

+		if !ok {
+			return types.InternalErrorf("incorrect data in datastore update notification: %v", data)
+		}
+		d.store, err = datastore.NewDataStoreFromUpdate(datastore.GlobalScope, dsu)


When is VxlanIdm initialized in this case ?

Still in d.configure(). I have retained the existing lazy logic where the programming is done on first network create.

Signed-off-by: Alessandro Boch <aboch@docker.com>

mavenugo · 2016-02-17T02:38:03Z

controller.go

+	for _, ds := range c.stores {
+		if ds.Scope() == datastore.GlobalScope {
+			c.Unlock()
+			return types.ForbiddenErrorf(


This must fail ONLY if the config has changed.
If the config is exactly the same, this function must return immediately without any error.

- After boot via ReloadConfiguration() method Signed-off-by: Alessandro Boch <aboch@docker.com>

mavenugo · 2016-02-17T22:20:01Z

@aboch thanks for addressing all the comments.

LGTM

sanimej · 2016-02-17T23:19:28Z

LGTM

Allow pass global datastore config after boot

rade · 2016-02-18T13:56:54Z

The new DiscoverNew and DiscoverDelete aren't being passed through the IPAM remote plugin API. And the API docs haven't been updated either.

So are plugins really forced to implement two methods that are neither documented nor ever invoked? Am I missing something?

aboch · 2016-02-18T16:34:59Z

@rade
No IPAM plugins are not forced to implement those methods. In fact they will not receive any requests regarding those two new methods.

rade · 2016-02-18T16:39:14Z

According to the docs "The IPAM driver (internal or remote) has to comply with the contract specified in ipamapi.contract.go". That contract now includes those two methods.

We, and I suspect other plugin implementers working in golang, took that instruction quite literal and are using that interface as a type in the plugin implementation.

aboch · 2016-02-18T16:48:53Z

@rade I will update the docs. Thanks.

rade · 2016-02-18T16:51:19Z

Cheers.

It would be really handy if there was a golang interface that defines the API. Just as there used to be before this change.

mavenugo · 2016-02-18T20:39:54Z

@rade we have https://github.com/docker/go-plugins-helpers/tree/master/ipam for any proper docker plugin and we recommend you to use that as the API layer.

- LocalKVProvider, LocalKVProviderConfig, LocalKVProvider, GlobalKVProviderConfig are all unused since moby/libnetwork#908. - MakeKVProvider, MakeKVProviderURL, MakeKVProviderConfig are unused since moby#44683. - MakeKVClient is unused since moby#44875 Signed-off-by: Albin Kerouanton <albinker@gmail.com>

- LocalKVProvider, LocalKVProviderURL, LocalKVProviderConfig, GlobalKVProvider, GlobalKVProviderURL and GlobalKVProviderConfig are all unused since moby/libnetwork@be2b6962 (moby/libnetwork#908). - GlobalKVClient is unused since 781e666a and c11c2a16. - MakeKVProvider, MakeKVProviderURL and MakeKVProviderConfig are unused since moby/moby@96cfb076 (moby#44683). - MakeKVClient is unused since moby/moby@142b5229 (moby#44875). Signed-off-by: Albin Kerouanton <albinker@gmail.com>

- LocalKVProvider, LocalKVProviderURL, LocalKVProviderConfig, GlobalKVProvider, GlobalKVProviderURL and GlobalKVProviderConfig are all unused since moby/libnetwork@be2b6962 (moby/libnetwork#908). - GlobalKVClient is unused since 652d1bf and 5e9e400. - MakeKVProvider, MakeKVProviderURL and MakeKVProviderConfig are unused since moby/moby@96cfb076 (moby#44683). - MakeKVClient is unused since moby/moby@142b5229 (moby#44875). Signed-off-by: Albin Kerouanton <albinker@gmail.com>

- LocalKVProvider, LocalKVProviderURL, LocalKVProviderConfig, GlobalKVProvider, GlobalKVProviderURL and GlobalKVProviderConfig are all unused since moby/libnetwork@be2b6962 (moby/libnetwork#908). - GlobalKVClient is unused since bb54332 and 06e41ba. - MakeKVProvider, MakeKVProviderURL and MakeKVProviderConfig are unused since 96cfb07 (moby#44683). - MakeKVClient is unused since 142b522 (moby#44875). Signed-off-by: Albin Kerouanton <albinker@gmail.com>

- LocalKVProvider, LocalKVProviderURL, LocalKVProviderConfig, GlobalKVProvider, GlobalKVProviderURL and GlobalKVProviderConfig are all unused since moby/libnetwork@be2b6962 (moby/libnetwork#908). - GlobalKVClient is unused since 0fa873c and c8d2c6e. - MakeKVProvider, MakeKVProviderURL and MakeKVProviderConfig are unused since 96cfb07 (moby#44683). - MakeKVClient is unused since 142b522 (moby#44875). Signed-off-by: Albin Kerouanton <albinker@gmail.com>

GordonTheTurtle added the status/0-triage label Jan 28, 2016

mavenugo reviewed Jan 28, 2016
View reviewed changes

aboch changed the title ~~Ipam driver to support datastore update~~ Handle datastore update in Ipam and overlay drivers Feb 4, 2016

mavenugo reviewed Feb 6, 2016
View reviewed changes

Handle datastore update in Ipam and overlay drivers

be2b696

Signed-off-by: Alessandro Boch <aboch@docker.com>

aboch changed the title ~~Handle datastore update in Ipam and overlay drivers~~ Allow pass global datastore config after boot Feb 17, 2016

mavenugo reviewed Feb 17, 2016
View reviewed changes

Allow to pass global datastore config

71496ef

- After boot via ReloadConfiguration() method Signed-off-by: Alessandro Boch <aboch@docker.com>

sanimej pushed a commit that referenced this pull request Feb 17, 2016

Merge pull request #908 from aboch/dds

c4197ed

Allow pass global datastore config after boot

sanimej merged commit c4197ed into moby:master Feb 17, 2016

rade mentioned this pull request Feb 18, 2016

1.4 build broken due to change in libnetwork IPAM API weaveworks/weave#1984

Closed

bboreham mentioned this pull request Feb 18, 2016

Add two functions required by ipamapi "contract" weaveworks/weave#1985

Merged

aboch mentioned this pull request Feb 18, 2016

Fix IPAM driver documentation #958

Merged

aboch deleted the dds branch March 7, 2016 16:44

akerouanton mentioned this pull request Feb 9, 2023

libnetwork/overlay: remove dead code moby/moby#44965

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow pass global datastore config after boot #908

Allow pass global datastore config after boot #908

aboch commented Jan 28, 2016

mavenugo Jan 28, 2016

aboch Jan 28, 2016

mavenugo Feb 6, 2016

aboch Feb 8, 2016

mavenugo Feb 6, 2016

aboch Feb 8, 2016

mavenugo Feb 17, 2016

aboch Feb 17, 2016

mavenugo commented Feb 17, 2016

sanimej commented Feb 17, 2016

rade commented Feb 18, 2016

aboch commented Feb 18, 2016

rade commented Feb 18, 2016

aboch commented Feb 18, 2016

rade commented Feb 18, 2016

mavenugo commented Feb 18, 2016

Allow pass global datastore config after boot #908

Allow pass global datastore config after boot #908

Conversation

aboch commented Jan 28, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo commented Feb 17, 2016

sanimej commented Feb 17, 2016

rade commented Feb 18, 2016

aboch commented Feb 18, 2016

rade commented Feb 18, 2016

aboch commented Feb 18, 2016

rade commented Feb 18, 2016

mavenugo commented Feb 18, 2016