Shared plugin: refactor kvstore and docker ops #1687

luomiao · 2017-08-03T00:10:25Z

Create a new kvStore interface, and have etcd as one of
the implementations of the kvStore interface.
Move docker host related operations to dockerops package

local testbed e2e passed.

govint · 2017-08-03T06:33:34Z

client_plugin/drivers/shared/dockerops/dockerops.go

+}
+
+// VolumeCreate - create volume from docker host with specific volume driver
+func (d *DockerOps) VolumeCreate(volumeDriver string, volName string, options map[string]string) error {


Could name as just Create(), Mount()... like what we have for vmdkops. Since all these ops are on volumes may not need to call out Volume in each function.

I don't agree since we are not just having volume operations here.
Below we have the function to launch the file sharing servers, which is not volume operation.
And later we will add more function like the ones to get swarm information here too.

govint · 2017-08-03T06:35:31Z

client_plugin/drivers/shared/dockerops/dockerops.go

+)
+
+// DockerOps is the interface for docker host related operations
+type DockerOps struct {


Should this be VolumeOps and thats using docker on the backend to create and manage the volumes. Something like vmdkops that manages vmdks via ESX.

It's not only for create and manage volumes... We have all the operations who will directly talk to docker host sitting in this struct. We will also need to do operations like launch services in this package too.

govint · 2017-08-03T06:44:25Z

client_plugin/drivers/shared/dockerops/dockerops.go

+}
+
+// StartSMBServer - Start SMB server
+func (d *DockerOps) StartSMBServer(volName string) bool {


Won't these be a separate interface. Or at least should be named differently?

Inside this function, we will talk to docker host in order to start a swarm service. It will need the same information as other operations who need to talk to docker host. That's why I put it here. All the functions inside this structure is related to docker host.

govint · 2017-08-03T06:50:57Z

client_plugin/drivers/shared/kvstore/kvstore.go

+	ReadVolMetadata(keys []string) ([]KvPair, error)
+
+	// DeleteVolMetadata - Delete volume metadata in KV store
+	DeleteVolMetadata(name string) error


These three could be like WriteMetaData, ReadMetaData, DeleteMetaData, List.

will update.

govint · 2017-08-03T06:58:37Z

client_plugin/drivers/shared/shared_driver.go

@@ -94,27 +78,28 @@ const (

 // VolumeMetadata - Contains metadata of shared volumes
 type VolumeMetadata struct {


Pls. add a description of the fields as done for the VolumeDriver struct. For example what is port here, is it the samba service port for this specific volume? How does a VM on a different ESX host access the same volume via the single samba instance. Is it via looking up the service name? How to figure which VM is running the samba service?

The description is already done above. :)

govint · 2017-08-03T07:05:47Z

client_plugin/drivers/shared/shared_driver.go

+	ServiceName    string            `json:"serviceName,omitempty"`
+	Username       string            `json:"username,omitempty"`
+	Password       string            `json:"password,omitempty"`
+	ClientList     []string          `json:"clientList,omitempty"`


Are these passwords in clear text? Also, is it possible for a user to gain access to the etcd and be able to retrieve this meta-data? This should be documented.

This information will be protected by etcd security in the future.

lipingxue · 2017-08-03T16:44:30Z

client_plugin/drivers/shared/dockerops/dockerops.go

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+


Please add comment about this file. What functionality will be provided by this file.

lipingxue · 2017-08-03T16:56:05Z

client_plugin/drivers/shared/dockerops/dockerops.go

+}
+
+// VolumeCreate - create volume from docker host with specific volume driver
+func (d *DockerOps) VolumeCreate(volumeDriver string, volName string, options map[string]string) error {


lipingxue · 2017-08-03T16:57:10Z

client_plugin/drivers/shared/kvstore/etcdops/etcdops.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.



Please add comments for this file.

lipingxue · 2017-08-03T17:02:13Z

client_plugin/drivers/shared/kvstore/kvstore.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package kvstore


Please add comments for this file.

lipingxue · 2017-08-03T17:03:19Z

client_plugin/drivers/shared/kvstore/kvstore.go

+// VolStatus: Datatype for keeping status of a shared volume
+type VolStatus string
+
+/*


Have we documented/added comments for the possible state transition? It would be very helpful for others to understand the code

The state transition will be documented on the wiki page.

lipingxue · 2017-08-03T17:03:58Z

client_plugin/drivers/shared/kvstore/kvstore.go

+	ReadVolMetadata(keys []string) ([]KvPair, error)
+
+	// DeleteVolMetadata - Delete volume metadata in KV store
+	DeleteVolMetadata(name string) error


lipingxue · 2017-08-03T17:05:12Z

client_plugin/drivers/shared/shared_driver.go

+	ServiceName    string            `json:"serviceName,omitempty"`
+	Username       string            `json:"username,omitempty"`
+	Password       string            `json:"password,omitempty"`
+	ClientList     []string          `json:"clientList,omitempty"`


shivanshu21 · 2017-08-04T21:26:44Z

client_plugin/drivers/shared/dockerops/dockerops.go

+	}
+
+	// if node is not in active swarm mode, return error
+	if info.Swarm.LocalNodeState != swarm.LocalNodeStateActive {


Have we explored other docker swarm states, like pending?
https://docs.docker.com/engine/swarm/admin_guide/#monitor-swarm-health

Thinking about the case where node reboots. Does swarm status become Active before plugin loads? (probably not relevant for tech preview)

It's ok even we return error here when the swarm state is pending.
The plugin will try to restart again and it will come to here again and again until the state is active :)

shivanshu21 · 2017-08-04T21:42:01Z

client_plugin/drivers/shared/kvstore/kvstore.go

+	// CompareAndPutStateOrBusywait - Compare the volume state with oldVal
+	// if equal, replace with newVal and return true; or else, return false;
+	// waits if volume is in a state from where it can reach the ready state
+	CompareAndPutStateOrBusywait(key string, oldVal string, newVal string) bool


This function will only be used to compare-and-put volume state. Maybe we shouldn't take "key string" here as it only busywaits depending on volume state.

Lets take volume name as input here and turn it into key inside the function.

If we pass in volume name instead of a key, this function is in fact "CompareAndPutVolStateOrBusywait".
But if you take a look at other KvStore interface methods, they are not "volume" operations. Instead, they are more generic "key-value" operations.
I know that inside the ETCD implementation we explicitly have all those volume state comparison. But still let's keep the methods at "key-value" level instead of "volume" level for consistency.

shivanshu21 · 2017-08-04T21:47:45Z

client_plugin/drivers/shared/shared_driver.go

+	sambaImageName = "dperson/samba"
+	sambaUsername  = "root"
+	sambaPassword  = "badpass"
+	stateIdx       = 0


These are basically useless. The values are returned in the same order as the keys[] requested in ReadMetaData().

If the caller doesnt stick to stateIDX=0, gRefIdx=1... convention, these wont work.

Should we really introduce them to avoid writing 1-2 integers as array indices? (I know.. I introduced them)

shivanshu21

Left few reviews.

Have we added a separate mount path for shared volumes?

Eg. /mnt/vmdk/vol1 -> /mnt/vshared/vol1?

luomiao · 2017-08-04T21:54:30Z

@shivanshu21
Edit: this change will be done in another separate PR.

* Create a new kvStore interface, and have etcd as one of the implementations of the kvStore interface. * Move docker host related operations to dockerops package

luomiao requested review from lipingxue, shivanshu21, govint and shaominchen August 3, 2017 00:10

vmwclabot added the cla-not-required label Aug 3, 2017

govint reviewed Aug 3, 2017

View reviewed changes

lipingxue reviewed Aug 3, 2017

View reviewed changes

luomiao force-pushed the shared-plugin-refactor branch from c4f80c1 to 2a7037a Compare August 4, 2017 20:53

shivanshu21 reviewed Aug 4, 2017

View reviewed changes

luomiao force-pushed the shared-plugin-refactor branch from 2a7037a to 5abceb4 Compare August 4, 2017 21:28

shivanshu21 reviewed Aug 4, 2017

View reviewed changes

shivanshu21 approved these changes Aug 4, 2017

View reviewed changes

Shared plugin: refactor kvstore and docker ops

c1428d6

* Create a new kvStore interface, and have etcd as one of the implementations of the kvStore interface. * Move docker host related operations to dockerops package

luomiao force-pushed the shared-plugin-refactor branch from 5abceb4 to c1428d6 Compare August 4, 2017 22:18

luomiao merged commit 5ee941f into vmware-archive:master Aug 7, 2017

luomiao mentioned this pull request Aug 9, 2017

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shared plugin: refactor kvstore and docker ops #1687

Shared plugin: refactor kvstore and docker ops #1687

luomiao commented Aug 3, 2017

govint Aug 3, 2017

lipingxue Aug 3, 2017

luomiao Aug 3, 2017

govint Aug 3, 2017

luomiao Aug 3, 2017

govint Aug 3, 2017

luomiao Aug 3, 2017

govint Aug 3, 2017

lipingxue Aug 3, 2017

luomiao Aug 3, 2017

govint Aug 3, 2017

luomiao Aug 3, 2017 •

edited

Loading

govint Aug 3, 2017

lipingxue Aug 3, 2017

luomiao Aug 3, 2017

lipingxue Aug 3, 2017

lipingxue Aug 3, 2017

lipingxue Aug 3, 2017

lipingxue Aug 3, 2017

lipingxue Aug 3, 2017

luomiao Aug 3, 2017

lipingxue Aug 3, 2017

lipingxue Aug 3, 2017

shivanshu21 Aug 4, 2017

luomiao Aug 4, 2017

shivanshu21 Aug 4, 2017

luomiao Aug 4, 2017

shivanshu21 Aug 4, 2017

shivanshu21 left a comment

luomiao commented Aug 4, 2017 •

edited

Loading

		@@ -94,27 +78,28 @@ const (

		// VolumeMetadata - Contains metadata of shared volumes
		type VolumeMetadata struct {

		@@ -12,7 +12,7 @@
		// See the License for the specific language governing permissions and
		// limitations under the License.

Shared plugin: refactor kvstore and docker ops #1687

Shared plugin: refactor kvstore and docker ops #1687

Conversation

luomiao commented Aug 3, 2017

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 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshu21 left a comment

Choose a reason for hiding this comment

luomiao commented Aug 4, 2017 • edited Loading

luomiao Aug 3, 2017 •

edited

Loading

luomiao commented Aug 4, 2017 •

edited

Loading