Update plugin to support running as a Windows service #1624

shaominchen · 2017-07-22T01:01:17Z

Fixes issue #1623.

Note: this PR changes the plugin server code so that the plugin can be run as a Windows service. The Windows service installation related logic will be handled in a separate PR.

Testing done: manually registered vdvs.exe as a Windows service - start/stop the service successfully and docker volume ops ran smoothly.

venilnoronha

Overall it looks great! Have a few nits and comments.

venilnoronha · 2017-07-22T01:40:17Z

client_plugin/vmdk_plugin/main.go

@@ -31,6 +31,10 @@ import (
 // main for docker-volume-vsphere
 // Parses flags, initializes and mounts refcounters and finally initializes the server.
 func main() {
+	runVdvs()


Can we have a better name? May be startDaemon()?

startDaemon looks better.

venilnoronha · 2017-07-22T01:40:42Z

client_plugin/vmdk_plugin/main_linux.go

@@ -0,0 +1,21 @@
+// Copyright 2016-2017 VMware, Inc. All Rights Reserved.


nit: remove 2016

venilnoronha · 2017-07-22T01:47:56Z

client_plugin/vmdk_plugin/main_linux.go

+// A VMDK Docker Data Volume plugin for Linux
+package main
+
+// Run vDVS plugin on Linux


nit: Should the comment start with the function name itself? That comes from Effective Go.

Yes, I'm aware of that convention. My understanding is: exported functions should follow this convention so that godoc can generate the documents properly. Unexported functions shouldn't really matter. But anyway, I will still fix it.

Yep, just for consistency sake.

venilnoronha · 2017-07-22T01:50:46Z

client_plugin/vmdk_plugin/main_windows.go

+
+func (p *program) Stop(s service.Service) error {
+    // Stop should not block. Return directly.
+    return nil


A query: does the service actually stop without us having to handle anything?

Yes, it stops properly. I didn't see anything unexpected when stopping the service.

venilnoronha · 2017-07-22T01:51:28Z

client_plugin/vmdk_plugin/main_windows.go

+    return nil
+}
+
+// Run vDVS plugin on Windows


nit: function comment style, like above.

venilnoronha · 2017-07-22T01:53:55Z

client_plugin/vmdk_plugin/main_windows.go

+    log "github.com/Sirupsen/logrus"
+)
+
+var logger service.Logger


Currently, logger is only used inside runVdvs(), so can have it as a local member there?

Fine. I was thinking we might need to add more service logs to Start/Stop in the future. But anyway, we can add it whenever we really need it.

venilnoronha · 2017-07-22T01:53:56Z

client_plugin/vmdk_plugin/main_windows.go

+
+var logger service.Logger
+
+type program struct{}


Should we name it differently? May be pluginService?

I'm fine with either one, as both will not affect the readability of this file.

We should probably add a comment explaining the need to have the service code for Windows, specifically about the Windows service timing-out because of the infinitely blocking startPluginServer() call.

Will add some comments.

venilnoronha · 2017-07-22T01:59:32Z

client_plugin/vmdk_plugin/main_windows.go

+
+func (p *program) Start(s service.Service) error {
+    // Start should not block. Do the actual work async.
+    go p.run()


Can we just do go startPluginServer()?

We can, but I think we'd better keep Start() as a wrapper of run(), in case we may need to add some logic into run() in the future.

venilnoronha

LGTM

venilnoronha · 2017-07-22T21:36:12Z

client_plugin/vmdk_plugin/main_windows.go

+// pluginServer is a wrapper for vDVS plugin so that the plugin can be run
+// as a service on Windows. Windows controls services by setting up callbacks
+// that is non-trivial. This wrapper utilizes a vendor library to handle this.
+type pluginServer struct{}


We have another PluginServer in our project, so I'm not sure if we should name this something else.

I will change it to PluginService then. Let me know if you have better suggestions.

That is good :)

lipingxue

Overall looks good.

lipingxue · 2017-07-24T17:00:04Z

client_plugin/vmdk_plugin/main.go

@@ -31,6 +31,11 @@ import (
 // main for docker-volume-vsphere
 // Parses flags, initializes and mounts refcounters and finally initializes the server.


The code which parses flags is moved to startPluginServer. So should move this comment to that function.

lipingxue · 2017-07-24T17:02:34Z

client_plugin/vmdk_plugin/main_windows.go

+
+    logger, err := svc.Logger(nil)
+    if err != nil {
+        log.Fatal(err)


How about log.Fatal("Failed to get logger : ", err)?

luomiao · 2017-07-25T02:25:57Z

@shaominchen
Sorry to bring back a merged PR.
This file:
client_plugin/vmdk_plugin/main_windows.go
the format (indent) seems wrong.
Everytime after I did a build, this file will be changed by go's formatting tool.
Can you update it with the correct format?

shaominchen · 2017-07-25T04:46:04Z

@luomiao Fixed with PR #1630. Sorry for the confusion.

shaominchen requested review from venilnoronha and msterin July 22, 2017 01:01

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

shaominchen changed the title ~~Refactor plugin main function to support Windows platform~~ Update plugin to support running as a Windows service Jul 22, 2017

venilnoronha reviewed Jul 22, 2017

View reviewed changes

venilnoronha approved these changes Jul 22, 2017

View reviewed changes

Shaomin Chen added 4 commits July 24, 2017 01:25

Refactor plugin main function to support Windows platform

75687b4

Fix some formats

a81dee7

Addressed Venil's comments.

0f1d0e2

Addressed Venil's comments

bace00a

shaominchen force-pushed the issue_1623 branch from 3af7e87 to d19cf87 Compare July 23, 2017 19:58

Trivial fix and rebase with master

d19cf87

lipingxue approved these changes Jul 24, 2017

View reviewed changes

Addressed Liping's comments.

841b9f6

shaominchen merged commit 5be6e01 into master Jul 24, 2017

shaominchen mentioned this pull request Jul 24, 2017

vDVS plugin for Windows should be running as a Windows service #1623

Closed

shaominchen deleted the issue_1623 branch July 24, 2017 20:36

shaominchen mentioned this pull request Jul 25, 2017

Trivial format fixes [SKIP CI] #1630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update plugin to support running as a Windows service #1624

Update plugin to support running as a Windows service #1624

shaominchen commented Jul 22, 2017

venilnoronha left a comment

venilnoronha Jul 22, 2017

shaominchen Jul 22, 2017 •

edited

Loading

venilnoronha Jul 22, 2017

shaominchen Jul 22, 2017

venilnoronha Jul 22, 2017

shaominchen Jul 22, 2017

venilnoronha Jul 22, 2017

venilnoronha Jul 22, 2017

shaominchen Jul 22, 2017

venilnoronha Jul 22, 2017

venilnoronha Jul 22, 2017

venilnoronha Jul 22, 2017

shaominchen Jul 22, 2017

venilnoronha Jul 22, 2017

shaominchen Jul 22, 2017

venilnoronha Jul 22, 2017 •

edited

Loading

shaominchen Jul 22, 2017

venilnoronha Jul 22, 2017

shaominchen Jul 22, 2017

venilnoronha Jul 22, 2017

venilnoronha left a comment

venilnoronha Jul 22, 2017 •

edited

Loading

shaominchen Jul 23, 2017

venilnoronha Jul 24, 2017

lipingxue left a comment

lipingxue Jul 24, 2017

shaominchen Jul 24, 2017

lipingxue Jul 24, 2017

shaominchen Jul 24, 2017

luomiao commented Jul 25, 2017

shaominchen commented Jul 25, 2017

		@@ -0,0 +1,21 @@
		// Copyright 2016-2017 VMware, Inc. All Rights Reserved.

		@@ -31,6 +31,11 @@ import (
		// main for docker-volume-vsphere
		// Parses flags, initializes and mounts refcounters and finally initializes the server.

Update plugin to support running as a Windows service #1624

Update plugin to support running as a Windows service #1624

Conversation

shaominchen commented Jul 22, 2017

venilnoronha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaominchen Jul 22, 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

venilnoronha Jul 22, 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

venilnoronha left a comment

Choose a reason for hiding this comment

venilnoronha Jul 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

luomiao commented Jul 25, 2017

shaominchen commented Jul 25, 2017

shaominchen Jul 22, 2017 •

edited

Loading

venilnoronha Jul 22, 2017 •

edited

Loading

venilnoronha Jul 22, 2017 •

edited

Loading