Check vmciVersion between client and server. #1046

lipingxue · 2017-03-17T21:09:44Z

This PR includes:

Add a new field "version" in struct "requestVmci" to store the current version of VMCI.
In ESX side, when receiving the request, read the version from client request and compare it against the server version of VMCI. If the VMCI version between client and server does not match, return with appropriate error.

Fixed #318

lipingxue · 2017-03-17T21:19:30Z

I did the unit testing manually with managed plugin.
Server VMCI version which is set in code is "12".

Case1: set client VMCI version to "11" through environment variable when install the managed plugin

install the managed plugin

root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin# docker plugin install --grant-all-permissions    lipingxue/docker-volume-vsphere:vmci_test  VDVS_LOG_LEVEL=debug  VMCI_VERSION=11
vmci_test: Pulling from lipingxue/docker-volume-vsphere
32627b4ce94d: Download complete
Digest: sha256:c798c643bacd33786dd118ca7053e50a0c379772cd34c23e969b18469af422a3
Status: Downloaded newer image for lipingxue/docker-volume-vsphere:vmci_test
Installed plugin lipingxue/docker-volume-vsphere:vmci_test
root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin#
root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin#
root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin#
root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin# docker plugin ls
ID                  NAME                                        DESCRIPTION                           ENABLED
705693b72f31        lipingxue/docker-volume-vsphere:vmci_test   VMWare vSphere Docker Volume plugin   true

create a volume with this plugin, which should fail

root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin# docker volume create --driver=lipingxue/docker-volume-vsphere:vmci_test  --name=vol1
Error response from daemon: create vol1: VolumeDriver.Create: Client version (11) is lower than server version (12), please upgrade client.

Case2: set client VMCI version in to "13" through environment variable when install the managed plugin

install the managed plugin

root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin# docker plugin install --grant-all-permissions    lipingxue/docker-volume-vsphere:vmci_test  VDVS_LOG_LEVEL=debug  VMCI_VERSION=13
vmci_test: Pulling from lipingxue/docker-volume-vsphere
32627b4ce94d: Download complete
Digest: sha256:c798c643bacd33786dd118ca7053e50a0c379772cd34c23e969b18469af422a3
Status: Downloaded newer image for lipingxue/docker-volume-vsphere:vmci_test
Installed plugin lipingxue/docker-volume-vsphere:vmci_test

create a volume with this plugin, which should fail

root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin# docker volume create --driver=lipingxue/docker-volume-vsphere:vmci_test  --name=vol1
Error response from daemon: create vol1: VolumeDriver.Create: Client version (13) is higher than server version (12), please upgrade server.

Case3: install managed plugin without setting environment variable

install the managed plugin

root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin# docker plugin install --grant-all-permissions    lipingxue/docker-volume-vsphere:vmci_test  VDVS_LOG_LEVEL=debug
vmci_test: Pulling from lipingxue/docker-volume-vsphere
32627b4ce94d: Download complete
Digest: sha256:c798c643bacd33786dd118ca7053e50a0c379772cd34c23e969b18469af422a3
Status: Downloaded newer image for lipingxue/docker-volume-vsphere:vmci_test
Installed plugin lipingxue/docker-volume-vsphere:vmci_test

create a volume with this plugin, which should succeed

root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin# docker volume create --driver=lipingxue/docker-volume-vsphere:vmci_test  --name=vol1
vol1
root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin#
root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin#
root@sc-rdops-vm18-dhcp-57-89:/tmp/plugin# docker volume ls
DRIVER                                      VOLUME NAME
lipingxue/docker-volume-vsphere:vmci_test   vol1@datastore2

lipingxue · 2017-03-17T21:19:58Z

Please review it @msterin @shaominchen @shuklanirdesh82

shuklanirdesh82 · 2017-03-17T21:36:18Z

esx_service/vmdk_ops.py

@@ -104,6 +104,10 @@
 MAX_SKIP_COUNT = 16       # max retries on VMCI Get Ops failures
 VMDK_ADAPTER_TYPE = 'busLogic'  # default adapter type

+#VMCI Version
+# !!! server side VMCI version, need to change according with client side version !!! 
+VMCI_VERSION = "12"


minor: don't we need to set as 13 as we are approaching to 0.13 release?

It does not really matter. To avoid confusion, I suggest naming it PROTOCOL_VERSION and assigning a value of 2 . It has nothing to do with release number

Also -please change comment to so something like Server side understand protocol version. If you are changing client/server protocol we use over VMCI, PLEASE DO NOT FORGET TO CHANGE IT FOR CLIENT in file <file name> !

VMCI_VERSION is definitely confusing - we are not really dealing with different VMCI versions. PROTOCOL_VERSION is much better, but it's still a little bit confusing to me - what's exactly the "protocol"? When we say "If you are changing client/server protocol", we actually mean "If the version of your client plug-in doesn't match with the version of your server driver". So personally I feel we should not even use the term "protocol" here.

Since we are actually handling the version control between our client plugin and server driver, why can't we simply define a internal version number (name doesn't matter, just a property in build infrastructure), which should be compiled into client plugin as a constant, say CLIENT_VERSION, and into server driver as another constant, say SERVER_VERSION. When client sends requests to the server, the CLIENT_VERSION should be sent to the server over VMCI. And then server will compare the CLIENT_VERSION it receives with its own SERVER_VERSION, and decide if there's a version mismatch.

Comments?

Done. Change the name to "PROTOCOL_VERSION".

I don't see any feedback/response to my comments above :) @lipingxue @msterin

Furthermore, I feel our client plugin and server driver are two independent components running in different user spaces. So it makes sense to maintain two separate version numbers. Also to improve the flexibility of inter-op, the version matching logic should consider backward compatibility, i.e. SERVER_VERSION should be compatible with lower CLIENT_VERSION. We may decide a version gap that we can support - let's say 3, then SERVER_VERSION 5 will work with CLIENT_VERSION 5, 4 and 3, but won't work with CLIENT_VERSION 2.

@shaominchen - the only difference I see between the current code and your suggestion is that you want to rename the vmdk_ops.py:PROTOCOL_VERSION to SERVER_PROTOCOL_VERSION (note - it is NOT the version of server. It is the version of over-VMCI protocol the server expects) .
and to rename esx_vmdkcmd.go:protocolCurrentVersion to clientProtocolVersion.

Is it what you are saying ?

I agree, it makes the code easier to read and understand.

I'm fine with SERVER_PROTOCOL_VERSION and CLIENT_PROTOCOL_VERSION. The main point here is to distinguish the version numbers between client and server.

OK. @lipingxue - please rename , unless you have some reasons not to (and then share them please )

shuklanirdesh82 · 2017-03-17T21:42:26Z

plugin/config.json

+	{
+      "name": "VMCI_VERSION",
+      "description": "Request VMCI version",
+      "value": "",


isn't it better to set default value for the version?

actually - this one is ONLY for debugging and testing, so we can set it to something and make sure the versions mismatch handling is correct

@lipingxue -- please change name to something more obvious (that it's for test only) and add proper comment in description. Something like TEST_PROTOCOL_VERSION

The client MUST have the version defined in the code. Please do not rely on the env variable for this

Done. Change the name to "TEST_PROTOCOL_VERSION".

msterin · 2017-03-17T22:33:41Z

esx_service/vmdk_ops.py

@@ -104,6 +104,10 @@
 MAX_SKIP_COUNT = 16       # max retries on VMCI Get Ops failures
 VMDK_ADAPTER_TYPE = 'busLogic'  # default adapter type

+#VMCI Version
+# !!! server side VMCI version, need to change according with client side version !!! 
+VMCI_VERSION = "12"


It does not really matter. To avoid confusion, I suggest naming it PROTOCOL_VERSION and assigning a value of 2 . It has nothing to do with release number

Also -please change comment to so something like Server side understand protocol version. If you are changing client/server protocol we use over VMCI, PLEASE DO NOT FORGET TO CHANGE IT FOR CLIENT in file <file name> !

msterin · 2017-03-17T22:36:59Z

esx_service/vmdk_ops.py

+            version = req["version"]
+            if version != VMCI_VERSION:
+                if int(version) < int(VMCI_VERSION):
+                    reply_string = err("Client version ({0}) is lower than server version ({1}), please upgrade client.".format(version, VMCI_VERSION))


Don't forget that it is the message the Docker user will see, so we need to be specific about where this message is coming from.
Something like "vSphere Docker Volume Service client version (1) is older than server version (2) , please update the client" and "vSphere Docker Volume Service client version (3) is newer than server version (2) , please update the server"

msterin · 2017-03-17T22:40:33Z

esx_service/vmdk_ops.py

+                if int(version) < int(VMCI_VERSION):
+                    reply_string = err("Client version ({0}) is lower than server version ({1}), please upgrade client.".format(version, VMCI_VERSION))
+                else:
+                    reply_string = err("Client version ({0}) is higher than server version ({1}), please upgrade server.".format(version, VMCI_VERSION))


nit: if the order of {} is the same as the order of params, no need for numbers inside of {}
i.e. this would be also correct and more pythonish:

reply_string = err("vSphere Docker Volume Service client version ({}) is newer than server version ({}) , please update the server".format(version, VMCI_VERSION))

msterin · 2017-03-17T22:42:06Z

plugin/config.json

+	{
+      "name": "VMCI_VERSION",
+      "description": "Request VMCI version",
+      "value": "",


actually - this one is ONLY for debugging and testing, so we can set it to something and make sure the versions mismatch handling is correct

@lipingxue -- please change name to something more obvious (that it's for test only) and add proper comment in description. Something like TEST_PROTOCOL_VERSION

msterin · 2017-03-17T22:42:50Z

vmdk_plugin/drivers/vmdk/vmdkops/esx_vmdkcmd.go

-
+	var versionEnv = os.Getenv("VMCI_VERSION")
+	log.Debugf("Run get request: version=%s", versionEnv)
+	// TODO: versionEnv need to read from enviroment variable when Nirdesh's Makefile is ready


please do not leave names in comments. It's better to open an issue and simply refer it.

msterin · 2017-03-17T22:44:09Z

plugin/config.json

+	{
+      "name": "VMCI_VERSION",
+      "description": "Request VMCI version",
+      "value": "",


The client MUST have the version defined in the code. Please do not rely on the env variable for this

msterin · 2017-03-17T22:46:16Z

vmdk_plugin/drivers/vmdk/vmdkops/esx_vmdkcmd.go

+	// TODO: versionEnv need to read from enviroment variable when Nirdesh's Makefile is ready
+	// versionEnv = "13"
+	var vmciVersion string
+	if versionEnv == "" {


this should not be picked up from environment.
You should do something like

protocolVersion := os.Getenv("TEST_PROTOCOL_VERSION") if protocolVersion == "" { protocolVersion = <some hardcoded constant from const() section in the file> }

pdhamdhere · 2017-03-18T00:59:38Z

esx_service/vmdk_ops.py

@@ -1469,6 +1473,15 @@ def execRequestThread(client_socket, cartel, request):
            reply_string = {u'Error': "Failed to parse json '%s'." % request}
            send_vmci_reply(client_socket, reply_string)
        else:
+            logging.debug("execRequestThread: req=%s", req)
+            version = req["version"]
+            if version != VMCI_VERSION:


This is too strict expecting user to have exact same version of plugin and esx service.

I think we need "MIN_SUPPORTED_VERSION" and "MAX_SUPPORTED_VERSION" and need to maintain forward/backward compatibility for at lease 1 release. For e.g. for upcoming release, 0.13 Plugin should be compatible with 0.12 Server or vice-versa.

currently protocol version IS NOT a release version and DOES NOT change with tagging or releases. It is a hardcoded (on both sides) version , and if it mismatches CURRENTLY we just decline. If/when we change the protocol and support mutli-protocol server , we will adjust accordingly

Check vmcVersion between client and server. Check vmciVersion between client and server. Add enviroment variable "VMCI_VERSION" in config.json file and some minor change. Change the default debug level back to "INFO". Small fix.

Address comments from Mark. Address comments from Mark. Remove unused code. One more fix.

lipingxue · 2017-03-22T05:36:23Z

Comments have been addressed, please take a look again. @msterin @shaominchen @shuklanirdesh82

msterin

Looks good. A couple of nits. One (remaining) question is - how it is going to behave if the curent code (with no version support) works vs. the new code (with version support). Will it fail - which is OK as long as it's clear what to do ?

msterin · 2017-03-22T08:28:04Z

plugin/config.json

-	}]
+	},
+	{
+      "name": "TEST_PROTOCOL_VERSION",


nit (since it's not for customers) Can we stick with VDVS_ as a prefix for all keys ?"

msterin · 2017-03-22T08:28:33Z

vmdk_plugin/drivers/vmdk/vmdkops/esx_vmdkcmd.go

-	maxRetryCount          = 5
+	commBackendName        string = "vsocket"
+	maxRetryCount                 = 5
+	protocolCurrentVersion        = "2"


Please add the comment similar to the one you've added on the server side

msterin · 2017-03-22T08:29:30Z

vmdk_plugin/drivers/vmdk/vmdkops/esx_vmdkcmd.go

 )

 // A request to be passed to ESX service
 type requestToVmci struct {
 	Ops     string     `json:"cmd"`
 	Details VolumeInfo `json:"details"`
+	Version string     `json:"version"`


so will it break compat with existng version ? Do you think an optional "version" is better, and the lack of version indicates V1 ?

shaominchen · 2017-03-22T17:14:13Z

esx_service/vmdk_ops.py

-#VMCI Version
-# !!! server side VMCI version, need to change according with client side version !!! 
-VMCI_VERSION = "12"
+# Server side understand protocol version. If you are changing client/server protocol we use


Personally I still feel "changing client/server protocol we use over VMCI" is a little bit confusing. As I have already commented in my previous comments, I suggest to use CLIENT_VERSION in VM plugin, and SERVER_VERSION in ESX driver. Please see my previous comments for more detail.

"VMCI_VERSION" has been changed to "PROTOCOL_VERSION". I think both the client and server side use the "PROTOCOL_VERSION" is clear enough.

I am actually with Sam here. Technically, there is no difference. Clarity-wise, with consts named properly (SERVER_PROTOCOL_VERSION and clientProtocolVersion) it is really much easier to understand

lipingxue · 2017-03-23T05:27:12Z

Added code to handle the case that client does not have version(v1 client), and do the test manually.

root@sc-rdops-vm18-dhcp-57-89:~# docker volume create --driver=vsphere --name=vol4
Error response from daemon: create vol4: VolumeDriver.Create: vSphere Docker Volume Service client is too old, and cannot talk to server with version (2), please update the client.

Address more comments from Mark. Small fix.

msterin

Thanks for addressing my comments.
A fresh nit (for the fresh code), but main request is to rename the vars per @shaominchen suggestion.
Other that that, LGTM

msterin · 2017-03-23T07:06:10Z

esx_service/vmdk_ops.py

+                                        "please update the server.".format(version, PROTOCOL_VERSION))
+                    send_vmci_reply(client_socket, reply_string)
+            else:
+                reply_string = err("vSphere Docker Volume Service client is too old, and cannot talk to server with version ({}), "


nit: can it be combined with line 1481 ?
Something lie "if verion not in req. or int(version) < int(clientVersion) { ... } else if ...

The error message vSphere Docker Volume Service client version ({}) is older than server version ({}) need to fill the client version. If client is too old (V1 client), there is no version available. Should we show the client version as None or show different error message as I did in the current code?

I think both approaches are fine (which is why it is a nit :-)).
My preference is to have the same message , and instead of "None" show ("1") since we start counting from 2 :-)

ok, got it. Will address it.

msterin · 2017-03-23T07:06:39Z

vmdk_plugin/drivers/vmdk/vmdkops/esx_vmdkcmd.go

+	maxRetryCount          = 5
+	// Server side understand protocol version. If you are changing client/server protocol we use
+	// over VMCI, PLEASE DO NOT FORGET TO CHANGE IT FOR SERVER in file <vmdk_ops.py> !
+	protocolCurrentVersion = "2"


I do agree with @shaominchen , calling clientProtocolVersion makes it much easier to comprehend

Address additional comments from Mark and Sam. Small fix.

lipingxue · 2017-03-23T17:53:24Z

@msterin @shaominchen I have address your comments, please take a look.

msterin

Looks good. Sorry, last comment related to rename - it has to be done for the 'version' var on the server as well. Just name it 'client_protocol_version'. A couple of small nits also in the comments

msterin · 2017-03-23T18:01:58Z

esx_service/vmdk_ops.py

+            version = req["version"] if "version" in req else "1"
+            logging.debug("execRequestThread: version=%s", version)
+            if version != SERVER_PROTOCOL_VERSION:
+                if not version or int(version) < int(SERVER_PROTOCOL_VERSION):


version is guaranted to be set in L1478, no need to do if 'not version' here. Should be simple
`if int(version) < int(SERVER_PROTOCOL_VERSION)

msterin · 2017-03-23T18:03:21Z

esx_service/vmdk_ops.py

@@ -1469,6 +1473,19 @@ def execRequestThread(client_socket, cartel, request):
            reply_string = {u'Error': "Failed to parse json '%s'." % request}
            send_vmci_reply(client_socket, reply_string)
        else:
+            logging.debug("execRequestThread: req=%s", req)
+            # If req from client does not include version number, set the version to "1" by default
+            version = req["version"] if "version" in req else "1"


just to make all consistent - can you rename 'version' -> 'client_protocol_version' ?
then the comparison would be 'if client_version < SERVER_PROTOCOL_VERSION'

msterin · 2017-03-23T18:05:07Z

esx_service/vmdk_ops.py

+            logging.debug("execRequestThread: version=%s", version)
+            if version != SERVER_PROTOCOL_VERSION:
+                if not version or int(version) < int(SERVER_PROTOCOL_VERSION):
+                    reply_string = err("vSphere Docker Volume Service client version ({}) is older than server version ({}), "


Also, SERVER_PROTOCOL_VERSION is int() already, isn't it?
I suggest setting client_protocol_version (L 1478) to int as well
client_protocol_version = int(req["version"] if "version" in req else 1 and droping int() conversion in other places

Address the comments from Mark to rename the variable. Small fix.

lipingxue · 2017-03-23T18:24:13Z

@msterin I have renamed the variable. Please take another look :)

shaominchen

Nice and clean! Thanks for addressing the comments.

shuklanirdesh82

LGTM!

pdhamdhere · 2017-03-24T16:23:59Z

CI is failing with errors unrelated to this PR.

plugin_sanity_test: [ERROR] error has occurred while fetching created plugin name..
         please make sure plugin is built correctly or not
bash: make: command not found
make[1]: *** [deploy-vm-plugin] Error 127
make: *** [deploy-vm-plugin] Error 2

Merging.

vmwclabot added the cla-not-required label Mar 17, 2017

shuklanirdesh82 reviewed Mar 17, 2017

View reviewed changes

msterin suggested changes Mar 17, 2017

View reviewed changes

pdhamdhere reviewed Mar 18, 2017

View reviewed changes

lipingxue mentioned this pull request Mar 21, 2017

Automate the version control test with managed plugin #1059

Closed

Check vmciVersion between client and server.

4e30b70

Check vmcVersion between client and server. Check vmciVersion between client and server. Add enviroment variable "VMCI_VERSION" in config.json file and some minor change. Change the default debug level back to "INFO". Small fix.

lipingxue force-pushed the vmci_version_control.liping branch 2 times, most recently from 6a9207b to 6a363fa Compare March 22, 2017 05:03

Address comments from Mark.

b9ddce5

Address comments from Mark. Address comments from Mark. Remove unused code. One more fix.

lipingxue force-pushed the vmci_version_control.liping branch from 6a363fa to b9ddce5 Compare March 22, 2017 05:32

msterin reviewed Mar 22, 2017

View reviewed changes

shaominchen reviewed Mar 22, 2017

View reviewed changes

Address more comments from Mark.

a681f12

Address more comments from Mark. Small fix.

msterin reviewed Mar 23, 2017

View reviewed changes

Address additional comments from Mark and Sam.

3193ab2

Address additional comments from Mark and Sam. Small fix.

msterin reviewed Mar 23, 2017

View reviewed changes

Address the comments from Mark to rename the variable.

6eee4b9

Address the comments from Mark to rename the variable. Small fix.

lipingxue force-pushed the vmci_version_control.liping branch from d08be68 to 6eee4b9 Compare March 23, 2017 18:22

msterin approved these changes Mar 23, 2017

View reviewed changes

shaominchen approved these changes Mar 23, 2017

View reviewed changes

shuklanirdesh82 approved these changes Mar 24, 2017

View reviewed changes

pdhamdhere merged commit 07b6371 into master Mar 24, 2017

pdhamdhere deleted the vmci_version_control.liping branch March 24, 2017 16:24

lipingxue mentioned this pull request Mar 28, 2017

Protocol compatible check need to print build version when client/server version mismatch is found #1103

Closed

Check vmciVersion between client and server. #1046

Check vmciVersion between client and server. #1046

Conversation

lipingxue commented Mar 17, 2017

lipingxue commented Mar 17, 2017

lipingxue commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaominchen Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaominchen Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

shaominchen Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

msterin Mar 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

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

lipingxue commented Mar 22, 2017

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

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

lipingxue commented Mar 23, 2017

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

Choose a reason for hiding this comment

lipingxue commented Mar 23, 2017

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

lipingxue commented Mar 23, 2017

shaominchen left a comment

Choose a reason for hiding this comment

shuklanirdesh82 left a comment

Choose a reason for hiding this comment

pdhamdhere commented Mar 24, 2017

shaominchen Mar 17, 2017 •

edited

Loading

shaominchen Mar 22, 2017 •

edited

Loading

shaominchen Mar 22, 2017 •

edited

Loading

msterin Mar 22, 2017 •

edited

Loading