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

Added multi datastore support. #471

Merged
merged 2 commits into from
Jun 16, 2016
Merged

Added multi datastore support. #471

merged 2 commits into from
Jun 16, 2016

Conversation

msterin
Copy link
Contributor

@msterin msterin commented Jun 15, 2016

Fixes #288

Design

  • each volume has short name (same as today) and fully qualified name (volume@datastore).
  • the '@datastore' part can be omitted , and it means "use current VM datastore'
  • default behavior (when using short names) is exactly as today - no changes
  • on 'create' , a volume can be created as '--name=volume@datastore' . The result is a creation of 'volume.vmdk' in datastore
    • ' -o datastore=datastore' is not supported yet
    • if datastore is omitted, 'volume' is created on VMs's datastore and 'volume' is returned to Docker
    • if datastore is present, 'volume' is created on this store and 'volume@datastore' is returned to Docker
    • if datastore does not exits, a list of existing ones is returned in error message
  • 'list' operation shows short names for volumes on current datastore, and fully qualified names for others. E.g
    $ docker volume ls
    myVol vmdk
    myVol@vsanDatastore vmdk
    someOtherVolume vmdk

Test

  • basic test automation for name parse is included
  • test automation for create/delete based on parsed name is not included - needs agreement on datastore names, and general rethinking/redesign or automation suit and libs (see Redesign / Rework test infra #472 )
    It would be too much to do now, so tested it manually (below)
  • make all and CI

Manual test:

  • create/delete volumes with default datastore (vsanDatastore) and not default one (datastore266) but the same name
  • do create/delete on volume and make sure the right one is impacted when default datastotre is used, and when non-default datastore is used.
  • run container ans make sure the volumes are different.

Test log:

root@photon-machine [ ~ ]# docker volume ls 
DRIVER              VOLUME NAME
vmdk                myVol33@datastore226
vmdk                superduper
vmdk                superduper@datastore226
vmdk                vol
vmdk                vol1
root@photon-machine [ ~ ]# docker volume create -d vmdk --name=volume@badstore
Error response from daemon: create volume@badstore: VolumeDriver.Create: Invalid datastore 'badstore'.
Known datastores: datastore226, build-toolchain, exit14.eng.vmware.com, build-apps, pa-dbc1122.eng.vmware.com, scm-trees, vsanDatastore.
Default datastore: vsanDatastore
root@photon-machine [ ~ ]# docker volume create --driver=vmdk --name=vol@vsanDatastore
Error response from daemon: create vol@vsanDatastore: VolumeDriver.Create: File /vmfs/volumes/vsanDatastore/dockvols/vol.vmdk already exists
root@photon-machine [ ~ ]# docker volume create --driver=vmdk --name=vol@datastore226 
vol@datastore226
root@photon-machine [ ~ ]# docker volume ls 
DRIVER              VOLUME NAME
vmdk                myVol33@datastore226
vmdk                superduper
vmdk                superduper@datastore226
vmdk                vol
vmdk                vol1
vmdk                vol@datastore226
root@photon-machine [ ~ ]# docker volume rm superduper
superduper
root@photon-machine [ ~ ]# docker volume ls 
DRIVER              VOLUME NAME
vmdk                myVol33@datastore226
vmdk                superduper@datastore226
vmdk                vol
vmdk                vol1
vmdk                vol@datastore226
root@photon-machine [ ~ ]# docker volume rm superduper@datastore226
superduper@datastore226
root@photon-machine [ ~ ]# docker volume ls 
DRIVER              VOLUME NAME
vmdk                myVol33@datastore226
vmdk                vol
vmdk                vol1
vmdk                vol@datastore226
root@photon-machine [ ~ ]# docker run --rm -v vol:/v busybox touch /v/MyFile
root@photon-machine [ ~ ]# docker run --rm -v vol:/v busybox ls /v
MyFile
lost+found
root@photon-machine [ ~ ]# docker run --rm -v vol@datastore226:/v busybox ls /v          
lost+found
root@photon-machine [ ~ ]# docker run --rm -v vol@datastore226:/v busybox touch /v/HisFile
root@photon-machine [ ~ ]# docker run --rm -v vol@datastore226:/v busybox ls /v
HisFile
lost+found
root@photon-machine [ ~ ]# docker run --rm -v vol:/v busybox ls /v
MyFile
lost+found

@msterin msterin force-pushed the multi-ds.msterin branch 5 times, most recently from 45e430d to d3dd501 Compare June 15, 2016 22:44
@andrewjstone
Copy link
Contributor

@msterin Did you do any manual tests with the Admin CLI to ensure it all still works as expected?

@msterin
Copy link
Contributor Author

msterin commented Jun 15, 2016

@andrewjstone not before you asked. There is nothing really impacted in admin cli, but I did run it now and it all good - at least ls command , See below. What else do you think is worth checking ?
Docker:

root@photon-machine [ ~ ]# docker volume ls 
DRIVER              VOLUME NAME
vmdk                myVol33@datastore226
vmdk                vol
vmdk                vol1
vmdk                vol@datastore226

Admin CLI:

root@promc-2n-dhcp35> /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py  ls
Volume   Datastore      Created By VM  Created                   Attached To VM  Policy          Capacity  Used     
-------  -------------  -------------  ------------------------  --------------  --------------  --------  -------  
myVol33  datastore226   PhotonRC_1     Wed Jun 15 01:12:29 2016  detached        N/A             100.00MB  14.00MB  
vol      datastore226   PhotonRC_1     Wed Jun 15 21:22:13 2016  detached        N/A             100.00MB  14.00MB  
vol      vsanDatastore  Photon1RC      Tue Jun 14 01:23:06 2016  PhotonRC_1      [VSAN default]  1.00GB    40.00MB  
vol1     vsanDatastore  PhotonRC_1     Tue Jun 14 18:02:02 2016  detached        [VSAN default]  100.00MB  56.00MB  

@andrewjstone
Copy link
Contributor

@msterin That seems good enough :)

def list_vmdks(path):
""" Return a list all VMDKs in a given path """
try:
files = os.listdir(path)
return [f for f in files
return [f for f in os.listdir(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this change :)

@andrewjstone
Copy link
Contributor

LGTM

Implements the following design
- each volume has short name (same as today) and fully qualified name (volume@datastore).
- the '@datastore' part can be omitted , and it means "use current VM datastore'
- default behavior (when using short names) is exactly as today - no changes
- on 'create' , a volume can be created as '--name=volume@datastore' OR as '--name=volume -o datastore=datastore'.
  The result is the same - 'volume.vmdk' in datastore
  - if datastore is omitted, 'volume' is create on VMs's datastore and 'volume' is returned to Docker
  - if datastore is present, 'volume' is created on this store and 'volume@datastore' is returned to Docker
  - if datastore does not exits, a list of existing ones is returned in error message
- 'list' operation shows short names for volumes on current datastore, and fully qualified names for others. E.g
   $ docker volume ls
   myVol                vmdk
   myVol@vsanDatastore  vmdk
   someOtherVolume      vmdk
for file_name in list_vmdks(path):
volumes.append({'path': path,
'filename': file_name,
'datastore': datastore})
return volumes


def get_vmdk_name(path, vol_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should this be really get_vmdk_path?

Copy link
Contributor Author

@msterin msterin Jun 16, 2016

Choose a reason for hiding this comment

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

sure, fixed

@pdhamdhere
Copy link
Contributor

LGTM.

@msterin msterin merged commit 11ceb51 into master Jun 16, 2016
@msterin msterin deleted the multi-ds.msterin branch June 17, 2016 02:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants