Removing volume size unit support in KB

[shuklanirdesh82]

Currently our driver is silently ignoring the value passed in KB and coverts into MB due a bug in covert.py: this PR is proposed to handle -o size parameter correctly while passing the input in KB.

Testing done: Yes (see below)

tested the proposed code with following volume creation commands:

root@photon-EHiCjpUvt [ ~ ]# docker volume create --driver=vmdk --name=MyVol8 -o size=4000kb
MyVol8
root@photon-EHiCjpUvt [ ~ ]# docker volume create --driver=vmdk --name=MyVol9 -o size=2000kb
MyVol9
root@photon-EHiCjpUvt [ ~ ]# docker volume create --driver=vmdk --name=MyVol10 -o size=1100kb
MyVol10

[root@promc-2n-dhcp105-97:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py ls
Volume   Datastore       Created By VM  Created                   Attached To VM  Policy  Capacity   Used      Disk Format  Filesystem Type  Access      Attach As               
-------  --------------  -------------  ------------------------  --------------  ------  ---------  --------  -----------  ---------------  ----------  ----------------------  
MyVol8   TestDatastore1  photon2        Fri Dec 16 23:03:49 2016  detached        N/A     3.00MB     3.00MB    thin         ext4             read-write  independent_persistent  
MyVol9   TestDatastore1  photon2        Fri Dec 16 23:05:01 2016  detached        N/A     1.00MB     2.00MB    thin         ext4             read-write  independent_persistent  
MyVol10  TestDatastore1  photon2        Fri Dec 16 23:05:26 2016  detached        N/A     1.00MB     2.00MB    thin         ext4             read-write  independent_persistent  

while working on this PR came to know about the limitation from the ESX server that we can't create vmdk < 1024 KB going to file a separate issue to initiate the discussion for the same.

/CC: @ashahi1

[msterin]

Actually I think it will be easier (and better) to simply fail (return parse error) if the value end with a letter and does not end with mb/gb/tb

[shuklanirdesh82]

@msterin: we can do that, by looking at the code (https://github.com/vmware/docker-volume-vsphere/blob/0e08691def491e7f36a0a03b8c013fe7cf3a5c37/esx_service/vmdk_ops.py#L386) thought to fix the issue at moment.

It needs some extra validation required if we down the route to handle KB is a valid unit, see below:

root@photon-EHiCjpUvt [ ~ ]# docker volume create --driver=vmdk --name=MyVol11 -o size=1011kb
Error response from daemon: create MyVol11: VolumeDriver.Create: Server returned an error: (vmodl.fault.InvalidArgument) {
   dynamicType = <unset>,
   dynamicProperty = (vmodl.DynamicProperty) [],
   msg = 'A specified parameter was not correct: ',
   faultCause = <unset>,
   faultMessage = (vmodl.LocalizableMessage) [
      (vmodl.LocalizableMessage) {
         dynamicType = <unset>,
         dynamicProperty = (vmodl.DynamicProperty) [],
         key = 'vim.hostd.vdisksvc.diskSizeTooSmall',
         arg = (vmodl.KeyAnyValue) [],
         message = 'The virtual disk size is too small.'
      }
   ],
   invalidProperty = <unset>
}

If we are OK to limit the unit with MB, GB, TB (http://vmware.github.io/docker-volume-vsphere/documentation/user-guide/docker-volume-cli/#size) then I need to revert my changes and need to cleanup the tests if there are any.

Please share your opinion.

[kerneltime]

LGTM after you fix the CI errors, I can see why supporting kb is not interesting but given that it is there we can fix it instead of removing it. A customer might want to specify it in kb :-)

[msterin]

@kerneltime

A customer might want to specify it in kb :-)

The point is - why do we need to allow this ? do you expect people to actually need 512KB disks :-) ?

[shuklanirdesh82]

@kerneltime @msterin

A customer might want to specify it in kb :-)

The point is - why do we need to allow this ? do you expect people to actually need 512KB disks :-) ?

while working on this PR came to know about the limitation from the ESX server that we can't create vmdk < 1024 KB; we need to accommodate such restriction by validating user input.

[shuklanirdesh82]

@kerneltime

after you fix the CI errors

Tested with ESX 6.0U2 & ESX 6.5:

ESX 6.5:

[root@sc-rdops-vm06-dhcp-183-137:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py status
Version: 0.9.1.3336235-0.0.1
Status: Running
Pid: 1001428050
Port: 1019
LogConfigFile: /etc/vmware/vmdkops/log_config.json
LogFile: /var/log/vmware/vmdk_ops.log
LogLevel: INFO
[root@sc-rdops-vm06-dhcp-183-137:~] python --version
Python 3.5.1+
[root@sc-rdops-vm06-dhcp-183-137:~]

ESX 6.0U2:

[root@promc-2n-dhcp105-97:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py status
Version: 0.9.1.3336235-0.0.1
Status: Running
Pid: 3169410
Port: 1019
LogConfigFile: /etc/vmware/vmdkops/log_config.json
LogFile: /var/log/vmware/vmdk_ops.log
LogLevel: INFO
[root@promc-2n-dhcp105-97:~] python --version
Python 2.7.9
[root@promc-2n-dhcp105-97:~]

[msterin]

Great, so let's drop KB feom allowed suffixes, and the problem will be gone

[shuklanirdesh82]

@msterin

Comment has been addressed to drop support of 'KB', please have a look at the latest changes.

Thanks!

[kerneltime]

What is the failure model here? If a size is given where the unit is not in conversion.keys() how is supposed to behave? I would guess fail the create of volume. I would like to see a negative test as well checking the behavior for a unit not supported (Ex: zb)

[shuklanirdesh82]

Thanks @kerneltime for the review!

What is the failure model here? If a size is given where the unit is not in conversion.keys() how is supposed to behave?

https://github.com/vmware/docker-volume-vsphere/blob/0e08691def491e7f36a0a03b8c013fe7cf3a5c37/esx_service/vmdk_ops.py#L378

validate_size util method is stopping user to pass any invalid option. So vmdk_ops.validate_size stops the volume creation. There is a bug in convert.convert_to_MB when unit is not found, it simply return the passed value hence the proposal to return '0' instead.

[msterin]

@shuklanirdesh82 - thanks for taking care of this.
@kerneltime - same :-)

[govint]

Would have been great to have this check in the plugin code instead, since size is a plugin option.

…

On Tue, Dec 20, 2016 at 2:39 AM, Mark Sterin ***@***.***> wrote: @shuklanirdesh82 <https://github.com/shuklanirdesh82> - thanks for taking care of this. @kerneltime <https://github.com/kerneltime> - same :-) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#829 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APHseJ1G9lNGOiv4RWKkMFUTnaBQbydlks5rJvJzgaJpZM4LPrzm> .

[shuklanirdesh82]

Would have been great to have this check in the plugin code instead, since
size is a plugin option.

@govint : That's good point. By looking at the code (https://github.com/vmware/docker-volume-vsphere/blob/master/vmdk_plugin/vmdkops/vmdkops.go#L50) it seems plugin is just a pass through for all kind of options and driver is the one where all validation code relies.

Can someone ( @msterin / @kerneltime ) shed some idea on keeping validation code at driver level instead of plugin?

Thanks!

[kerneltime]

We cannot skip validation in esx service in my opinion. Adding validation on plugin side is an optimization to quickly reject what is not supported but the support really originates from the esx service. Going forward we will have to support mixing of versions for ESX and the Docker plugin.. in that case I can see why it would be clean to keep all validation on the ESX side and let Docker only deal with the docker API not the extensions via -o flag

[msterin]

I agree with @kerneltime here - ESX side should do validation. Plugin side may do it to shortcut the round trip, but I do not think does not really save much ; so I think having all validation on ESX side is perfectly reasonable