Add compile support of new plugin/driver "vsphere-shared" #1574

luomiao · 2017-07-11T22:56:02Z

New plugin: client_plugin/shared_plugin
New driver: client_plugin/drivers/shared
To build the new plugin: make shared-all
Plugin is pushed into: [DOCKER_HUB_REPO]/vsphere-shared:[VERSION_TAG][EXTRA_TAG]
Functions in the new shared_driver are to be implemented.

There are a few layout change in drivers directory.
Common utility functions are grouped under drivers/commonUtilDriver.

The new plugin shares the same plugin_dockerbuild Makefile with original vmdk plugin.
Thus the Dockerfile and config.json are replaced by Dockerfile-template and config.json-template in order to be generic.

Testing result:
test-e2e runalways passed.
Skipped local run of test-e2e runonce due to testbed requirements not ideal, and will use CI for final check.

msterin

It's a big change so I did not get all the details reviewed but all in all it looks good to me.
A few comments/requests inline.

msterin · 2017-07-11T23:24:27Z

Makefile

@@ -42,6 +42,11 @@ build-all: dockerized-build-ui
 	$(MAKE) --directory=client_plugin $@
 	$(MAKE) --directory=plugin_dockerbuild all

+# build shared plugin
+build-shared: dockerized-build-ui
+	$(MAKE) --directory=client_plugin dockerbuild-shared


its generally a good idea to add $(MAKEFLAGS) to nested calls to $(MAKE)

msterin · 2017-07-12T00:02:26Z

client_plugin/drivers/commonUtilDriver/commonUtilDriver.go

+)
+
+// VolumeDriver - vsphere shared plugin volume driver struct
+type CommonUtilDriver struct {


the name looks redundant. commonutilsdriver.CommonUtilsDriver ? And why its "commonUtils" not simply driver or pluginDriver ?

msterin · 2017-07-12T00:04:30Z

client_plugin/drivers/photon/photon_driver.go

-	d.refCounts.Init(d, mountDir, cfg.Driver)
-	d.mountIDtoName = make(map[string]string)
+
+	d.utils = &commonUtilDriver.CommonUtilDriver{


yeah, that's what I meant. THis looks slightly redundant

msterin · 2017-07-12T00:06:39Z

client_plugin/shared_plugin/main.go

+		os.Exit(1)
+	}
+
+	if reflect.ValueOf(driver).IsNil() == true {


so if driver == Nil does not work ?

Did some googling and it seems we cannot compare an interface with nil.
And here driver is an interface.
Although I tried to change the code to if driver == nil, compile still works.

msterin · 2017-07-12T00:07:54Z

plugin_dockerbuild/config.json-template

@@ -21,7 +21,7 @@
 			"Options": ["rbind"]
 		},
 		{
-			"Description" : "Location to look for config file (/etc/docker-volume-vsphere.conf)",
+			"Description" : "Location to look for config file (/etc/BINARY.conf)",


I strongly recommend using some pattern like %BINARY% to separate from comments etc.
Or better , try using golang template module

Hmmm I am not sure how to use golang template here.
We don't have any go code. This is where we build the docker plugins.
So it's the script world...
It might be an overkill to add and compile some go code here in order to replace texts only :)
Let me add the pattern thing instead.

New plugin: client_plugin/shared_plugin New driver: client_plugin/drivers/shared To build the new plugin: make shared-all Plugin is pushed into: [DOCKER_HUB_REPO]/vsphere-shared:[VERSION_TAG][EXTRA_TAG] Functions in the new shared_driver are to be implemented.

shuklanirdesh82

some minor comments.

shuklanirdesh82 · 2017-07-13T03:42:01Z

Makefile

@@ -42,6 +42,11 @@ build-all: dockerized-build-ui
 	$(MAKE) --directory=client_plugin $@
 	$(MAKE) --directory=plugin_dockerbuild all

+# build shared plugin
+build-shared: dockerized-build-ui


I guess we don't need to invoke dockerized-build-ui anymore, can you please confirm with @msterin ?

shuklanirdesh82 · 2017-07-13T03:50:04Z

client_plugin/drivers/shared/shared_driver.go

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


golang naming convention doesn't recommend _ in filename ..shared_driver.go .. it is better to avoid using _ .. how aboutsharedvolume.go?

As you are proposing new files I would suggest to correct the naming.

one more thing, package level comment should be above L15 .. that means before package xxx

Hmmm I know that the golang naming convention doesn't recommend _ in package name. But I don't think there is a convention that doesn't recommend _ in filename.
I saw _ in filenames almost everywhere in the codebase :)
In vDVS codebase we already have:

client_plugin/utils/plugin_utils/plugin_utils.go client_plugin/vmdk_plugin/sanity_test.go client_plugin/drivers/vmdk/vmdk_driver.go ....

I don't think you ignore them on purpose before? ;)
Also please see my comment below.
Even golang's source code has _ in their filenames :)

shuklanirdesh82 · 2017-07-13T04:02:32Z

client_plugin/drivers/utils/pluginDriver.go

+
+package utils
+
+//


Package level comment should be above L15 .. that means before package xxx

file name should be corrected too

Reference: "By convention, packages are given lower case, single-word names; there should be no need for underscores or mixedCaps."

Generally, file names follow the same convention as package names (https://golang.org/doc/effective_go.html#package-names)

Same as the comment above...
I don't think filename need to follow this rule...

shuklanirdesh82 · 2017-07-13T04:02:58Z

client_plugin/shared_plugin/main.go

+
+package main
+
+// A vSphere Shared Docker Data Volume plugin - main


need to move above L17

shuklanirdesh82 · 2017-07-13T04:11:41Z

Commonvars.mk

@@ -54,7 +54,8 @@ EXTRA_TAG ?= -dev
 PLUGIN_TAG := $(VERSION_TAG)$(EXTRA_TAG)

 # plugin name - used as a base for full plugin name and container for extracting rootfs
-PLUGIN_NAME=$(DOCKER_HUB_REPO)/docker-volume-vsphere
+PLUGIN_NAME ?= $(DOCKER_HUB_REPO)/docker-volume-vsphere


curious to know here .. ?= why is this change?

is it passed or set?

don't we need for L58?

?= indicates to set the variable only if it's not set/doesn't have a value.
This change is because of we will recursively call the Makefile for the shared plugin, to avoid duplicate Makefile code.
See Line 67 in the new plugin_dockerbuild/Makefile:

shared-clean: PLUGIN_NAME=$(SHARED_PLUGIN_NAME) BINARY=$(SHARED_BINARY) $(MAKE) clean

L58 doesn't need ?= because there is no recursively setting of SHARED_PLUGIN_NAME.

shuklanirdesh82 · 2017-07-13T04:15:41Z

client_plugin/utils/config/config_linux.go

+	// DefaultVMDKPluginLogPath is the default location of log (trace) file
+	DefaultVMDKPluginLogPath = "/var/log/docker-volume-vsphere.log"
+	// DefaultSharedPluginConfigPath is the default location of Log configuration file for shared plugin
+	DefaultSharedPluginConfigPath = "/etc/vsphere-shared.conf"


Haven't come across any document file .. would like to make sure to document this newly introduced conf/log file location. will it be in the following PR?

Yes. We will have readme for the new plugin. And it will be documented there.

shuklanirdesh82 · 2017-07-13T04:17:30Z

client_plugin/shared_plugin/main.go

+	"github.com/docker/go-plugins-helpers/volume"
+	"github.com/vmware/docker-volume-vsphere/client_plugin/drivers/shared"
+	"github.com/vmware/docker-volume-vsphere/client_plugin/utils/config"
+	"github.com/vmware/docker-volume-vsphere/client_plugin/utils/plugin_server"


I might have missed the PR where plugin_server is introduced ..

see import package seems ugly as go doesn't recommend _ in the PR (not for this PR though)

Please don't ignore the _ in "client_plugin" part. :)

luomiao · 2017-07-13T07:12:03Z

@shuklanirdesh82
I did some googling and found out some discussion about filename naming convention in golang:
https://stackoverflow.com/questions/25161774/what-are-conventions-for-filenames-in-go

Besides that there is special use for _test.go and _linux.go and so on, even golang's own package has _ in their filenames:
https://golang.org/src/compress/bzip2/move_to_front.go

So I guess there isn't a recommendation about not using _ in filenames.

shuklanirdesh82 · 2017-07-13T15:08:35Z

@luomiao

Besides that there is special use for _test.go and _linux.go

_test.go is kept to be picked by go test and identify test easily where as _linux.go & _<architecture> is also being kept for go build to pick up files while building.

luomiao · 2017-07-13T16:35:10Z

@shuklanirdesh82
That's why I said besides :)

There are more in golang source code:
https://golang.org/src/compress/bzip2/move_to_front.go
https://github.com/golang/go/blob/master/src/compress/flate/huffman_bit_writer.go
https://github.com/golang/go/blob/master/src/archive/tar/stat_atimespec.go
https://github.com/golang/go/blob/master/src/container/heap/example_intheap_test.go

msterin · 2017-07-13T18:58:13Z

Lots of code is not compliant with standards and code style guidance, so examples do not work very well.
The golang.org says in https://blog.golang.org/package-names

 Good package names are short and clear. They are lower case, with no under_scores or mixedCaps. They are often simple nouns, such as:

    time (provides functionality for measuring and displaying time)
    list (implements a doubly linked list)
    http (provides HTTP client and server implementations)

The style of names typical of another language might not be idiomatic in a Go program. Here are two examples of names that might be good style in other languages but do not fit well in Go:

    computeServiceClient
    priority_queue

A Go package may export several types and functions. For example, a compute package could export a Client type with methods for using the service as well as functions for partitioning a compute task across several clients.

Abbreviate judiciously. Package names may be abbreviated when the abbreviation is familiar to the programmer. Widely-used packages often have compressed names:

    strconv (string conversion)
    syscall (system call)
    fmt (formatted I/O)

On the other hand, if abbreviating a package name makes it ambiguous or unclear, don't do it.

BTW, they actually advice against generic name. e.g. util :-)

So I am with @shuklanirdesh82 here, though IMO it's not a huge issue and we are not compliant in many other places already. Still, I suggest something like dockvolplugin. Or keep it as-is.

luomiao · 2017-07-13T19:19:16Z

I don't mind if we apply this rule to vDVS code!
But please be consistent.
I saw that there is newly checked in files like:
https://github.com/vmware/docker-volume-vsphere/blob/master/tests/e2e/volume_access_test.go (the first '_' :))
https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/esx/vim_cmd.go
https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/dockercli/docker_mgmt.go

And of course as @msterin mentioned, we didn't follow such rule before. All files under client_plugin mainly are not compliant with this rule.

So if someone want to open an issue and adjust all the current filenames, I won't have an opinion.
But I don't think we should apply a rule suddenly and leaving filenames like
client_plugin/drivers/vmdk/vmdk_driver.go and client_plugin/drivers/shared/shareddriver.go there at the same time.
So I suggest that, if we REALLY care about the filename naming, please update the current filenames, and then apply to future PRs :)

luomiao requested review from msterin, shivanshu21 and shuklanirdesh82 July 11, 2017 22:56

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

luomiao self-assigned this Jul 11, 2017

luomiao force-pushed the shared-plugin branch from 053986c to fa5f38f Compare July 11, 2017 23:00

luomiao mentioned this pull request Jul 11, 2017

Add support of new plugin/driver "vsphere-shared". #1539

Closed

msterin reviewed Jul 12, 2017

View reviewed changes

luomiao force-pushed the shared-plugin branch from fa5f38f to e5e9e8a Compare July 12, 2017 01:31

luomiao force-pushed the shared-plugin branch from e5e9e8a to 300327a Compare July 13, 2017 02:01

shuklanirdesh82 reviewed Jul 13, 2017

View reviewed changes

msterin approved these changes Jul 13, 2017

View reviewed changes

luomiao merged commit 57cae95 into vmware-archive:master Jul 13, 2017

shuklanirdesh82 mentioned this pull request Jul 17, 2017

Make vDVS plugin code compliant with golang standards and code style guidance #1593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compile support of new plugin/driver "vsphere-shared" #1574

Add compile support of new plugin/driver "vsphere-shared" #1574

luomiao commented Jul 11, 2017 •

edited

Loading

msterin left a comment

msterin Jul 11, 2017

msterin Jul 12, 2017

msterin Jul 12, 2017

msterin Jul 12, 2017

luomiao Jul 12, 2017 •

edited

Loading

msterin Jul 12, 2017

msterin Jul 12, 2017 •

edited

Loading

luomiao Jul 12, 2017

msterin Jul 12, 2017

shuklanirdesh82 left a comment

shuklanirdesh82 Jul 13, 2017

shuklanirdesh82 Jul 13, 2017

luomiao Jul 13, 2017 •

edited

Loading

shuklanirdesh82 Jul 13, 2017

luomiao Jul 13, 2017

shuklanirdesh82 Jul 13, 2017

shuklanirdesh82 Jul 13, 2017

luomiao Jul 13, 2017 •

edited

Loading

shuklanirdesh82 Jul 13, 2017

luomiao Jul 13, 2017

shuklanirdesh82 Jul 13, 2017

luomiao Jul 13, 2017

luomiao commented Jul 13, 2017

shuklanirdesh82 commented Jul 13, 2017

luomiao commented Jul 13, 2017

msterin commented Jul 13, 2017 •

edited

Loading

luomiao commented Jul 13, 2017


		package main

		// A vSphere Shared Docker Data Volume plugin - main

Add compile support of new plugin/driver "vsphere-shared" #1574

Add compile support of new plugin/driver "vsphere-shared" #1574

Conversation

luomiao commented Jul 11, 2017 • edited Loading

msterin 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

luomiao Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msterin Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuklanirdesh82 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luomiao Jul 13, 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

luomiao Jul 13, 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

luomiao commented Jul 13, 2017

shuklanirdesh82 commented Jul 13, 2017

luomiao commented Jul 13, 2017

msterin commented Jul 13, 2017 • edited Loading

luomiao commented Jul 13, 2017

luomiao commented Jul 11, 2017 •

edited

Loading

luomiao Jul 12, 2017 •

edited

Loading

msterin Jul 12, 2017 •

edited

Loading

luomiao Jul 13, 2017 •

edited

Loading

luomiao Jul 13, 2017 •

edited

Loading

msterin commented Jul 13, 2017 •

edited

Loading