Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Fix the issue that client without protocol version set cannot talk to server with protocol version check. #1106

Merged
merged 3 commits into from
Mar 29, 2017
Merged

Fix the issue that client without protocol version set cannot talk to server with protocol version check. #1106

merged 3 commits into from
Mar 29, 2017

Conversation

lipingxue
Copy link
Contributor

@lipingxue lipingxue commented Mar 28, 2017 •
edited

This PR is to make sure client without protocol version set can talk to server with protocol check
to make sure the backward compatibility.
Fixed #1102.

shaominchen
shaominchen reviewed Mar 28, 2017
View reviewed changes
esx_service/vmdk_ops.py Outdated
@@ -1491,8 +1491,8 @@ def execRequestThread(client_socket, cartel, 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
client_protocol_version = int(req["version"]) if "version" in req else 1
# If req from client does not include version number, set the version to "2" by default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"2" seems a magic number here. Shouldn't we just use the constant "SERVER_PROTOCOL_VERSION"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure.

@lipingxue
Copy link
Contributor Author

I have test the following two configuration manually with basic create/attach/detach/rm flow.

Docker version in Ubuntu VM:
root@sc-rdops-vm18-dhcp-57-89:~# docker -v
Docker version 17.03.0-ce, build 3a232c8

Config1: ESX with latest build (0.12.938dc3e-0.0.1, with protocol version check), VM install 0.12 build (0.12.dadf7db, without protocol version set)

step1: create volume

root@sc-rdops-vm18-dhcp-57-89:~# docker volume ls
DRIVER              VOLUME NAME

root@sc-rdops-vm18-dhcp-57-89:~# docker volume create --driver=vsphere --name=testvol1
testvol1
root@sc-rdops-vm18-dhcp-57-89:~#
root@sc-rdops-vm18-dhcp-57-89:~# docker volume ls
DRIVER              VOLUME NAME
vsphere             testvol1@datastore2

step2: run a busybox container to attach the volume

root@sc-rdops-vm18-dhcp-57-89:~# docker run --rm -it -v testvol1:/data busybox
Unable to find image 'busybox:latest' locally
latest: Pulling from library/busybox
7520415ce762: Pull complete
Digest: sha256:32f093055929dbc23dec4d03e09dfe971f5973a9ca5cf059cbfb644c206aa83f
Status: Downloaded newer image for busybox:latest
/ # pwd
/
/ # ls
bin   data  dev   etc   home  proc  root  sys   tmp   usr   var
/ # root@sc-rdops-vm18-dhcp-57-89:~#

step3: remove the container to detach the volume

root@sc-rdops-vm18-dhcp-57-89:~# docker rm d5b4b21c6f45 -f
d5b4b21c6f45

step4: remove volume

root@sc-rdops-vm18-dhcp-57-89:~# docker volume rm testvol1@datastore2
testvol1@datastore2
root@sc-rdops-vm18-dhcp-57-89:~#
root@sc-rdops-vm18-dhcp-57-89:~#
root@sc-rdops-vm18-dhcp-57-89:~# docker volume ls
DRIVER              VOLUME NAME

Config2: ESX with 0.12 build (0.12.3f64805-0.0.1, no protocol version check), VM with latest build (0.12.938dc3e, with protocol version set)

step1: create volume

root@sc-rdops-vm18-dhcp-57-89:~# docker volume ls
DRIVER              VOLUME NAME
root@sc-rdops-vm18-dhcp-57-89:~#
root@sc-rdops-vm18-dhcp-57-89:~# docker volume create --driver=vsphere --name=testvol2
testvol2
root@sc-rdops-vm18-dhcp-57-89:~#
root@sc-rdops-vm18-dhcp-57-89:~# docker volume ls
DRIVER              VOLUME NAME
vsphere             testvol2@datastore2

step2: run a busybox container to attach the volume

root@sc-rdops-vm18-dhcp-57-89:~# docker run --rm -it -v testvol2:/data busybox
/ # ls
bin   data  dev   etc   home  proc  root  sys   tmp   usr   var
/ # root@sc-rdops-vm18-dhcp-57-89:~#

step3: remove the container to detach the volume

root@sc-rdops-vm18-dhcp-57-89:~# docker rm e9dc38890c9f  -f
e9dc38890c9f

step4: remove volume

root@sc-rdops-vm18-dhcp-57-89:~# docker volume rm testvol2
testvol2
root@sc-rdops-vm18-dhcp-57-89:~#
root@sc-rdops-vm18-dhcp-57-89:~#
root@sc-rdops-vm18-dhcp-57-89:~# docker volume ls
DRIVER              VOLUME NAME

shuklanirdesh82
shuklanirdesh82 approved these changes Mar 28, 2017
View reviewed changes
Copy link
Contributor

@shuklanirdesh82 shuklanirdesh82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shuklanirdesh82
Copy link
Contributor

@lipingxue can you please link the issue this PR is fixing?

shaominchen
shaominchen approved these changes Mar 28, 2017
View reviewed changes
Copy link
Contributor

@shaominchen shaominchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

pdhamdhere
pdhamdhere approved these changes Mar 29, 2017
View reviewed changes
esx_service/vmdk_ops.py
client_protocol_version = int(req["version"]) if "version" in req else 1
# If req from client does not include version number, set the version to
# SERVER_PROTOCOL_VERSION by default to make backward compatible
client_protocol_version = int(req["version"]) if "version" in req else SERVER_PROTOCOL_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Looks like original implementation did consider backward compatibility but hardcoded version, 1, broke it!

@pdhamdhere pdhamdhere force-pushed the test_upgrade_version_change.liping branch from 1dcdfa1 to e4b7775 Compare March 29, 2017 03:14
@pdhamdhere pdhamdhere merged commit 5f6d430 into master Mar 29, 2017
@shuklanirdesh82 shuklanirdesh82 deleted the test_upgrade_version_change.liping branch March 31, 2017 07:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pdhamdhere pdhamdhere pdhamdhere approved these changes

@shuklanirdesh82 shuklanirdesh82 shuklanirdesh82 approved these changes

@shaominchen shaominchen shaominchen approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lipingxue @shuklanirdesh82 @pdhamdhere @shaominchen @vmwclabot