Shared plugin: add etcd watcher and event handler #1661

luomiao · 2017-07-30T23:28:35Z

Every docker swarm manager starts a watcher on all the global
refcount keys in etcd. All the managers get events when any global
refcount key is updated. Only one of the managers is able to get
the right to adjust the state of volume and start/stop the file
system server.

This PR also changed some of the etcd functions into kvStore interface:

type kvStore interface {
	// Init - initialize the KV store cluster
	Init() error
	// WriteVolMetadata - Update or Create volume metadata in KV store
	WriteVolMetadata(entries []kvPair) error
	// ReadVolMetadata - Read volume metadata in KV store
	ReadVolMetadata(keys []string) ([]kvPair, error)
        // DeleteVolMetadata - Delete volume metadata in KV store
        DeleteVolMetadata(name string) error
	// CompareAndPut - Compare the value of key with oldVal, if equal, replace with newVal
	CompareAndPut(key string, oldVal string, newVal string) bool
	// ListVolumeName - List all the volumes in the KV store
	ListVolumeName() ([]string, error)
}

This PR needs to be rebased after PR #1631 is merged.

Basic e2e test passed on local testbed.

lipingxue

Overall looks good, and I do have several comments/questions.

lipingxue · 2017-07-31T21:26:11Z

client_plugin/drivers/shared/etcdops.go

@@ -134,7 +163,7 @@ func (d *VolumeDriver) initEtcd() error {
 					"manager ID": nodeID},
 			).Info("Swarm node role: manager. Action: find leader ")

-			joinEtcdCluster(addr, n.ManagerStatus.Addr, nodeID)
+			e.joinEtcdCluster(n.ManagerStatus.Addr)
 			return nil


Why here return error as "nil? I think e.startEtcdCluster returns with err, right?

Thanks for catching this... I changed the logic of return last time and forgot to update return error here!

lipingxue · 2017-07-31T21:35:39Z

client_plugin/drivers/shared/etcdops.go

@@ -255,9 +292,11 @@ func joinEtcdCluster(nodeAddr string, leaderAddr string, nodeID string) error {
 		"--initial-cluster-state", etcdClusterStateExisting,
 	}

+	// start the etcd cluster routine


Why the comments is start the etcd cluster routine? I think it should be "join the etcd cluster"?

Join etcd cluster is called by manager nodes who are not initializing the cluster. To join, they must start their own etcd, hence the comment.

So it really means "start" ... "routine". will update the comment to avoid confusion :)

lipingxue · 2017-07-31T21:38:19Z

client_plugin/drivers/shared/etcdops.go

+// checkLocalEtcd function check if local ETCD endpoint is successfully started or not
+// if yes, start the watcher for volume global refcount
+func (e *etcdInfo) checkLocalEtcd() error {
+	ticker := time.NewTicker(checkSleepDuration)


Just curious, how do we decide the right value of "checkSleepDuration" and "requestTimeout"?

These values are mainly set according to other projects. Will need to do more testing and adjust values to be more accurate.

lipingxue · 2017-07-31T21:39:37Z

client_plugin/drivers/shared/etcdops.go

+	}
+}
+
+func (e *etcdInfo) etcdWatcher() {


Please add comments for this function.

lipingxue · 2017-07-31T21:40:06Z

client_plugin/drivers/shared/etcdops.go

+	}
+}
+
+func (e *etcdInfo) etcdEventHandler(ev *etcdClient.Event) {


Please add comments for this function.

lipingxue · 2017-07-31T21:41:39Z

client_plugin/drivers/shared/etcdops.go

+			ev.PrevKv != nil &&
+			string(ev.PrevKv.Value) == etcdNoRef {
+			// watcher observes global refcount from 0 to 1
+			// transactional edit state first


Where is the code to make sure the state edit is transactional?

I see. Inside function "CompareAndPut", you call the transaction interface. But you call function "CompareAndPut" 2 or 3 times here. How do we make sure it is transactional too?

I am not sure we want the different compare and put steps to be in the same transaction. State of the volume has to be updated first which will then act as a lock for all other operations. Miao can throw some light maybe.

We cannot have those CompareAndPut as a single transaction :)
As Shivanshu has mentioned above, please consider the CompareAndPut to change the state from Ready to Intermediate as "lock", and consider the CompareAndPut to change state from Intermediate to Mounted as "unlock". Only when the "lock" is successful, we go ahead to do corresponding operations (that is, startSMBServer), and "unlock" later.

lipingxue · 2017-07-31T21:45:47Z

client_plugin/drivers/shared/etcdops.go

+			return
+		}
+
+		if string(ev.Kv.Value) == etcdNoRef &&


This code snippet is very similar to the above code which handle "etcdSingleRef" to "etcdNonRef". Is it possible to create a common util function to reuse the code?

lipingxue · 2017-07-31T21:49:58Z

client_plugin/drivers/shared/etcdops.go

@@ -346,14 +520,14 @@ func (d *VolumeDriver) etcdList() ([]string, error) {
 }

 // writeVolMetadata - Update or Create volume metadata in KV store


Nit: "writeVolMetadata" -> "WriteVolMetadata" to be consistent with the function name.

lipingxue · 2017-07-31T21:50:14Z

client_plugin/drivers/shared/etcdops.go

@@ -379,13 +553,13 @@ func (d *VolumeDriver) writeVolMetadata(entries []kvPair) error {
 }

 // readVolMetadata - Read volume metadata in KV store


Same above.

shivanshu21 · 2017-07-31T23:07:04Z

client_plugin/drivers/shared/shared_driver.go

@@ -145,7 +147,8 @@ func NewVolumeDriver(cfg config.Config, mountDir string) *VolumeDriver {
 	d.dockerd = cli

 	// initialize built-in etcd cluster
-	err = d.initEtcd()
+	d.etcd.driver = &d


Why are we doing this?
Putting etcdInfo obj in VolumeDriver
And also putting VolumeDriver in etcdInfo obj?

I mean, we do have access to d. in etcdops.go right?

No really :)
For example, from etcdEventHandler function (which is a method of etcdInfo struct), we need to call startSMBServer function (which is a method of VolumeDriver struct). So from e *etcdInfo we need to get the VolumeDriver associate with it, and the best way is to link back VolumeDriver inside etcdInfo.

shivanshu21 · 2017-07-31T23:15:42Z

client_plugin/drivers/shared/etcdops.go

+				if e.CompareAndPut(etcdPrefixState+volName,
+					string(volStateIntermediate),
+					string(volStateReady)) == false {
+					// Failed to set state Intermediate->Mounted,


Comment fix: Intermediate -> ready. SMB server has stopped not started.

shivanshu21 · 2017-07-31T23:16:16Z

client_plugin/drivers/shared/etcdops.go

+						string(volStateError))
+				}
+			} else {
+				// failed to start SMB server, set to state Error


Same. Failed to stop smb server.

shivanshu21

Mostly minor comment fixes suggested.

Ques: Why are we putting etcdInfo obj in VolumeDriver and also VolumeDriver in etcdInfo when VolumeDriver obj "d" is accessible in etcdops?
miao: Maybe I missed something, but how do you access the corresponding "d" without linking it inside etcdInfo? Remember that etcdEventHandler is a method of etcdInfo now.

There seems a possibility that all watchers can get error in setting state from XXX -> Intermediate.
No change needed in this PR but we will have to handle that in Mount() via timeout

shaominchen

Overall looks good. Have a few comments related to coding style.

shaominchen · 2017-07-31T23:53:00Z

client_plugin/drivers/shared/etcdops.go

+type kvStore interface {
+	// Init - initialize the KV store cluster
+	Init() error
+	// WriteVolMetadata - Update or Create volume metadata in KV store


Can we add an empty line in between the interfaces so that it looks prettier?

shaominchen · 2017-08-01T00:12:14Z

client_plugin/drivers/shared/etcdops.go

@@ -319,10 +493,10 @@ func addrToEtcdClient(addr string) (*etcdClient.Client, error) {
 }

 // etcdList function lists all the volume names associated with this KV store


Nit: update the comment

shaominchen · 2017-08-01T00:15:59Z

client_plugin/drivers/shared/etcdops.go

+	nodeAddr string
+	client   *etcdClient.Client
+}
+
 // initEtcd start or join ETCD cluster depending on the role of the node


Nit: comments need to be updated

shaominchen · 2017-08-01T00:21:56Z

client_plugin/drivers/shared/etcdops.go

+	ListVolumeName() ([]string, error)
+}
+
+type etcdInfo struct {


Can we have a better name for this interface? The name "etcdInfo" sounds like a data object.

shaominchen · 2017-08-01T00:26:33Z

client_plugin/drivers/shared/etcdops.go

@@ -72,10 +77,32 @@ type kvPair struct {
 	value string
 }

+type kvStore interface {


Is it intended that the interface is not exported, but the functions in it are exported?

I will move the interface code to a following refactor PR :)

shaominchen · 2017-08-01T00:30:07Z

client_plugin/drivers/shared/etcdops.go

+		log.Fields{"type": ev.Type},
+	).Infof("Watcher on global refcount returns event ")
+
+	if ev.Type == etcdClient.EventTypePut {


Do we plan to add more handlers for other events? If we are just handling PUT event here, we can return fast to avoid too deep nested if conditions.

Yeah we will need to check other events in the future...

Every docker swarm manager starts a watcher on all the global refcount keys in etcd. All the managers get events when any global refcount key is updated. Only one of the managers is able to get the right to adjust the state of volume and start/stop the file system server.

Address review comments. *Add missing error return after joinEtcdCluster *Add missing function comments *Address misleading comments *Make duplicated code block into nested function *Delay interface of KvStore change to a later refactor PR

govint · 2017-08-01T08:22:32Z

client_plugin/drivers/shared/etcdops.go

 	nodeID := info.Swarm.NodeID
+	addr := info.Swarm.NodeAddr


Is info.Swarm guaranteed to be there? SHould there be a check on info.Swarm being valid.

Swarm is a pre-request for this plugin...
But we need a check and error out here too. will update.

govint · 2017-08-01T08:24:24Z

client_plugin/drivers/shared/etcdops.go

+		nodeAddr: addr,
+	}
+
+	// worker just returns
 	if info.Swarm.ControlAvailable == false {
 		log.WithFields(
 			log.Fields{"nodeID": nodeID},
 		).Info("Swarm node role: worker. Action: return from InitEtcd ")


Change function name in error message. And error message should really say why its bailing out.

Thanks for catching the function name. This one in fact is not a error message but a log info for debugging use.

govint · 2017-08-01T08:31:31Z

client_plugin/drivers/shared/etcdops.go

 	// if leader, proceed to start ETCD cluster
 	if node.ManagerStatus.Leader {
 		log.WithFields(
 			log.Fields{"nodeID": nodeID},
 		).Info("Swarm node role: leader, start etcd cluster")
-		err = startEtcdCluster(addr, nodeID)
+		err = e.startEtcdCluster()
 		if err != nil {
 			log.WithFields(log.Fields{"nodeID": nodeID,
 				"error": err}).Error("Failed to start ETCD Cluster")


could make all error messages use the same name - ETCD or etcd, line 113 calls it "etcd".

govint · 2017-08-01T08:33:03Z

client_plugin/drivers/shared/etcdops.go

 	}

 	// if manager, first find out who's leader, then proceed to join ETCD cluster
 	nodes, err := docker.NodeList(ctx, types.NodeListOptions{})
 	if err != nil {
 		log.WithFields(log.Fields{"nodeID": nodeID,
 			"error": err}).Error("Failed to get NodeList from swarm manager")
-		return err
+		return nil


Is it ok to leave after starting the etcd cluster above? What happens if this code runs again and calls startEtcdCluster(). Will that error or return ok.

Can the NodeList be got first and then startEtcdCluster()? this is an error flow that can be handled first before creating the cluster? Start etcd cluster after line 149.

For the first question:
It's OK because only swarm leader will call the startEtcdCluster, and non-leader managers won't call startEtcdCluster.

For the second question:
No. Because swarm leader doesn't need to call NodeList. The workflow is: if this node is a leader, startEtcdCluster and leave; if this node is a non-leader manager, do NodeList to find who's the leader, then joinEtcdCluster and leave.

govint · 2017-08-01T08:50:46Z

client_plugin/drivers/shared/etcdops.go

-			joinEtcdCluster(addr, n.ManagerStatus.Addr, nodeID)
-			return nil
+			e.joinEtcdCluster(n.ManagerStatus.Addr)
+			if err != nil {


Where is err set?

govint · 2017-08-01T10:26:45Z

client_plugin/drivers/shared/etcdops.go

+				// set to state Error
+				e.CompareAndPut(etcdPrefixState+volName,
+					string(volStateIntermediate),
+					string(volStateError))


Is this guaranteed to pass, logs should be included for these failures, because the volume may remain in the intermediate state (creating?) if the state change fails here (already error'ed at line 368). What does the user do in this case? Can the user delete the volume?

Yes, users can still delete the volume with "--force" flag. And we already have the log message inside CompareAndPut function.

govint · 2017-08-01T10:34:34Z

client_plugin/drivers/shared/etcdops.go

+		succeeded := e.CompareAndPut(etcdPrefixState+volName,
+			string(fromState), string(volStateIntermediate))
+		if !succeeded {
+			// this handler doesn't get the right to start server


Is this comment correct? The preceding line is to set state of a volume vs. starting a server?

It's set state and then start a server. And set the state is for starting the server :)

govint · 2017-08-01T10:38:15Z

client_plugin/drivers/shared/etcdops.go

+	if err != nil {
+		log.WithFields(
+			log.Fields{"Key": key,
+				"Error": err},


Could log the new and old values

govint · 2017-08-01T10:47:29Z

client_plugin/drivers/shared/etcdops.go

+}
+
+func (e *etcdKVS) createEtcdClient() *etcdClient.Client {
+	dclient := e.driver.dockerd


At line 92 we're calling this dockerd as "docker", could use a single variable name in different functions where e.driver.dockerd is used - dclient sounds correct.

govint · 2017-08-01T11:06:04Z

client_plugin/drivers/shared/etcdops.go

+	return
+}
+
+func (e *etcdKVS) CompareAndPut(key string, oldVal string, newVal string) bool {


Add a comment.

govint

Approving changes and @luomiao will handle any comments as appropriate.

lipingxue

LGTM

Address more comments.

luomiao requested review from msterin and shivanshu21 July 30, 2017 23:28

vmwclabot added the cla-not-required label Jul 30, 2017

luomiao requested review from govint, lipingxue and shaominchen July 31, 2017 00:31

luomiao force-pushed the shared-plugin-etcd-watcher branch from a032e77 to 3a83454 Compare July 31, 2017 06:53

lipingxue reviewed Jul 31, 2017

View reviewed changes

shivanshu21 reviewed Jul 31, 2017

View reviewed changes

shaominchen reviewed Aug 1, 2017

View reviewed changes

Miao Luo added 2 commits July 31, 2017 18:56

luomiao force-pushed the shared-plugin-etcd-watcher branch from 3a83454 to d1428f6 Compare August 1, 2017 01:57

govint suggested changes Aug 1, 2017

View reviewed changes

govint approved these changes Aug 1, 2017

View reviewed changes

lipingxue approved these changes Aug 1, 2017

View reviewed changes

Shared plugin: add etcd watcher and event handler

edf2a09

Address more comments.

luomiao force-pushed the shared-plugin-etcd-watcher branch from 2939b9e to edf2a09 Compare August 1, 2017 17:53

luomiao merged commit 8a21023 into vmware-archive:master Aug 1, 2017

shuklanirdesh82 mentioned this pull request Aug 2, 2017

golint error for one of the method in etcdops.go #1676

Closed

		@@ -346,14 +520,14 @@ func (d *VolumeDriver) etcdList() ([]string, error) {
		}

		// writeVolMetadata - Update or Create volume metadata in KV store

		@@ -379,13 +553,13 @@ func (d *VolumeDriver) writeVolMetadata(entries []kvPair) error {
		}

		// readVolMetadata - Read volume metadata in KV store

		@@ -319,10 +493,10 @@ func addrToEtcdClient(addr string) (*etcdClient.Client, error) {
		}

		// etcdList function lists all the volume names associated with this KV store

Shared plugin: add etcd watcher and event handler #1661

Shared plugin: add etcd watcher and event handler #1661

Conversation

luomiao commented Jul 30, 2017 • edited Loading

lipingxue left a 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

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

luomiao Jul 31, 2017 • edited Loading

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

Choose a reason for hiding this comment

shivanshu21 left a comment • edited by luomiao Loading

Choose a reason for hiding this comment

shaominchen left a 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

luomiao Aug 1, 2017 • edited Loading

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

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

luomiao Aug 1, 2017 • edited Loading

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

govint left a comment

Choose a reason for hiding this comment

lipingxue left a comment

Choose a reason for hiding this comment

luomiao commented Jul 30, 2017 •

edited

Loading

luomiao Jul 31, 2017 •

edited

Loading

shivanshu21 left a comment •

edited by luomiao

Loading

luomiao Aug 1, 2017 •

edited

Loading

luomiao Aug 1, 2017 •

edited

Loading