Add local datastore to persist states of LocalScope network #466

chenchun · 2015-08-21T04:37:23Z

Signed-off-by: Chun Chen ramichen@tencent.com

Fixes #461

chenchun · 2015-08-21T04:39:54Z

Depending on godep updates of libkv.

mavenugo · 2015-08-21T04:40:37Z

@chenchun thanks for your fast response. The CI failure is expected since the corresponding boltdb changes have not made into libkv project.

FYI, this needs a Godep update. which we can update once docker/libkv#46 is merged

mavenugo · 2015-08-21T04:41:20Z

Makefile

@@ -68,6 +68,7 @@ install-deps:
 	go get github.com/tools/godep
 	go get github.com/golang/lint/golint
 	go get github.com/mattn/goveralls
+	go get github.com/boltdb/bolt


This should ideally go into godep.

Yes, once bolt gets merged into libkv project, I'll update.

abronan · 2015-08-21T07:04:01Z

@chenchun You added all the etcd log files, make sure to exclude the default.etcd folder from your commits.

abronan · 2015-08-21T07:05:43Z

Needs a vendoring of boltdb library too with Godep.

chenchun · 2015-08-21T07:22:17Z

@abronan Thanks, updated and removed them.

abronan · 2015-08-21T07:38:34Z

This will replace #439

mavenugo · 2015-08-23T01:08:37Z

config/config.go

-	Datastore DatastoreCfg
+	Daemon                    DaemonCfg
+	Cluster                   ClusterCfg
+	Datastore, LocalDataStore DatastoreCfg


Shall we call these GlobalStore and LocalStore ?

Addressed this

chenchun · 2015-08-24T10:20:33Z

Also contains the following change:
Removed useless function getNetworkFromStore and getEndpointFromStore from store.go.
Added tests to test default config for local store.
Added KVProviderConfigto netlabel/labels.go and passed LocalScope configuration to opt in controller.RegisterDriver.
Removed network.isGlobalScoped since it's useless now. watchNetworks and watchEndpoints will be invoked only by global store now.

chenchun · 2015-08-24T10:22:47Z

controller.go

+		if err := c.initLocalStore(); err != nil {
+			// Failing to initalize datastore is a bad situation to be in.
+			// But it cannot fail creating the Controller
+			log.Debugf("Failed to Initialize LocalDatastore due to %v.", err)


Since we added default configurations for local store, shall we return err here?

I think so.

Addressed this.

mavenugo · 2015-08-24T16:13:40Z

@chenchun thanks for addressing all the comments. its shaping up nicely. I just gave a few more comments. Could you please address them ?
If you have any specific design question, we can chat in #docker-network IRC channel.

chenchun · 2015-08-24T16:15:01Z

@mavenugo, I'll update the PR soon.

chenchun · 2015-08-24T17:23:52Z

@mavenugo, just updated the PR. It's quite later today, if you have more comments, I'll take a look 8 hours later.

mavenugo · 2015-09-10T21:01:31Z

store.go

-	if err != nil || !global {
+func (c *controller) newNetworkFromStore(n *network) error {
+	// Check if a network already exists with the specified network name
+	c.Lock()


Please remove the c.Lock() and Unlock() in this method. It causes deadlock :(

Fixed. And tested against docker, seems ok now.

chenchun · 2015-09-11T00:10:46Z

I agree it tries to do too many things. I think we can split the last commit and focus on introducing localstore. But these should be a way to handle https://github.com/docker/libnetwork/blob/master/drivers/bridge/bridge.go#L887

mavenugo · 2015-09-11T03:06:57Z

thanks @chenchun. I noticed that you have reverted the bridge driver changes that I reported. Is that expected ? Without the changes to the bridge driver, wouldnt restore fail as I indicated earlier ?
I will not be able to give this patch a try tonight. But it will be good if you can give me some description of the latest changes that you have done to enable with the code-review.

mavenugo · 2015-09-14T20:29:49Z

@chenchun based on our discussion, i agree that the bridge driver changes aren't required & also the daemon restart is going to cleanup the endpoint anyways.
I discussed this issue with the other maintainers and we feel that it is infact better to not even save the endpoint for local scope (we still need it in the globalstore).
Can you please make that change ? And I think that will solve all the issues and makes it is ready for merge.

chenchun · 2015-09-15T02:24:09Z

also the daemon restart is going to cleanup the endpoint anyways

Correct me if I'm wrong. I think daemon restarts won't delete endpoints currently because daemon only keeps SandboxID/EndpointID now and when daemon is killed manually, daemon won't be able to delete veth devices. So I think it's better to keep endpoints and deletes them upon restart.

mavenugo · 2015-09-15T03:05:53Z

@chenchun you are right. But, we have other ideas to clean up the stale endpoints. Another reason to keep the endpoints deletion away from this PR is to reduce the complexity and get this in ASAP.
So, it would be good if we get this PR in with network save/restore right away and work on the endpoint cleanup on bootup in docker. Also, we have multiple PRs pending on the libkv upgrade. Shall we get this PR to get Network save and restore completed followed by another for proper endpoint handling ?
am in IRC and we can work together on this.

mavenugo · 2015-09-15T10:01:15Z

drivers/remote/driver.go

 	case "local":
-		c.Scope = driverapi.LocalScope
+		c.DataScope = datastore.LocalScope


Thats a very quick rebase :-).

Unfortunately forget to refactor tests during rebase.

mavenugo · 2015-09-15T12:17:59Z

Thanks @chenchun for the contribution and addressing all the comments patiently.
I tested e2e with both local and global store.

LGTM.

ping @mrjana @aboch

aboch · 2015-09-15T22:41:57Z

LGTM

mavenugo · 2015-09-21T09:38:28Z

@chenchun as we discussed, we were trying to simplify this logic by removing my commit from this PR. With the introduction of #547, we can actually do that ... 🎆

Shall I request you to submit the PR again (with a rebase) and remove my commit (https://github.com/chenchun/libnetwork/commit/652d319c33bef98e58f1223b6f47d25a609f1291) from this PR ? BTW I attempted this change in : https://github.com/mavenugo/libnetwork/commits/defnet_save. Please use it if you find it useful. (You can ignore my last commit that introduces persist option),

This ofcourse needs changes in the docker daemon to call controller.New with driver init params. Which we will take care during vendoring in.

chenchun · 2015-09-21T09:44:44Z

OK, will update the PR shortly.

Signed-off-by: Chun Chen <ramichen@tencent.com>

mavenugo · 2015-09-21T12:08:45Z

Thanks @chenchun pls let me know if you need any help.

chenchun · 2015-09-21T14:18:04Z

@mavenugo, some problems with unit tests now. Since we changed many unit tests to pass configs to controller.New function, some tests will create localstore now. And cause they are run in a single process, they will hang on creating the same boltdb file lock.

mavenugo · 2015-09-21T14:36:24Z

@chenchun. Can't we use config Params to point to different Boltdb file for each of the different controllers to make use of ?

chenchun · 2015-09-21T14:38:06Z

Yes, doing it right now.

1. Don't save localscope endpoints to localstore for now. 2. Add common function updateToStore/deleteFromStore to store KVObjects. 3. Merge `getNetworksFromGlobalStore` and `getNetworksFromLocalStore` 4. Add `n.isGlobalScoped` before `n.watchEndpoints` in `addNetwork` 5. Fix integration-tests 6. Fix test failure in drivers/remote/driver_test.go 7. Restore network to store if deleteNework failed

chenchun · 2015-09-21T17:38:43Z

@mavenugo updated now. Will test it against docker tomorrow.

chenchun · 2015-09-21T17:41:09Z

network.go

 		if err == datastore.ErrKeyModified {
 			return types.InternalErrorf("operation in progress. delete failed for network %s. Please try again.")
 		}
 		return err
 	}

+	defer func() {


Add a defer here to restore network.

mavenugo · 2015-09-22T02:02:52Z

@chenchun thanks for your patience and handling all the comments.
We still need a couple of fixes to make it work properly e2e with Docker Daemon.
(such as mavenugo@76c9c13). But we can handle it via subsequent PRs.

Will merge this PR.

Add local datastore to persist states of LocalScope network

chenchun · 2015-09-22T02:08:57Z

👍 , Happy to see it finally gets merged. I apologize if I slow down your work during the process of this PR. Thanks for your patient review and help too.

mavenugo reviewed Aug 21, 2015
View reviewed changes

mavenugo mentioned this pull request Aug 21, 2015

Update libkv in Godep #439

Closed

mavenugo reviewed Aug 23, 2015
View reviewed changes

chenchun reviewed Aug 24, 2015
View reviewed changes

chenchun mentioned this pull request Sep 10, 2015

Fix for zookeeper backend #462

Merged

mavenugo reviewed Sep 10, 2015
View reviewed changes

mavenugo reviewed Sep 15, 2015
View reviewed changes

mavenugo mentioned this pull request Sep 16, 2015

Etcd can't be used a datastore on multihost #392

Closed

chenchun added 2 commits September 21, 2015 17:56

Update godep to support boltdb backend

91a8dc0

Signed-off-by: Chun Chen <ramichen@tencent.com>

Add local datastore to persist states of LocalScope network

b35d173

Signed-off-by: Chun Chen <ramichen@tencent.com>

chenchun reviewed Sep 21, 2015
View reviewed changes

mavenugo added a commit that referenced this pull request Sep 22, 2015

Merge pull request #466 from chenchun/localstore

5df9f84

Add local datastore to persist states of LocalScope network

mavenugo merged commit 5df9f84 into moby:master Sep 22, 2015

chenchun deleted the localstore branch September 22, 2015 07:15

mavenugo mentioned this pull request Sep 22, 2015

Providing KVObject option to skip persisting object in kvstore #554

Merged

Add local datastore to persist states of LocalScope network #466

Add local datastore to persist states of LocalScope network #466

Conversation

chenchun commented Aug 21, 2015

chenchun commented Aug 21, 2015

mavenugo commented Aug 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abronan commented Aug 21, 2015

abronan commented Aug 21, 2015

chenchun commented Aug 21, 2015

abronan commented Aug 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenchun commented Aug 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo commented Aug 24, 2015

chenchun commented Aug 24, 2015

chenchun commented Aug 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenchun commented Sep 11, 2015

mavenugo commented Sep 11, 2015

mavenugo commented Sep 14, 2015

chenchun commented Sep 15, 2015

mavenugo commented Sep 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo commented Sep 15, 2015

aboch commented Sep 15, 2015

mavenugo commented Sep 21, 2015

chenchun commented Sep 21, 2015

mavenugo commented Sep 21, 2015

chenchun commented Sep 21, 2015

mavenugo commented Sep 21, 2015

chenchun commented Sep 21, 2015

chenchun commented Sep 21, 2015

Choose a reason for hiding this comment

mavenugo commented Sep 22, 2015

chenchun commented Sep 22, 2015