-
Notifications
You must be signed in to change notification settings - Fork 95
Conversation
6b76cd7
to
4759a30
Compare
cc @asomaiah |
@pdhamdhere I split the commits for the bug fix and the documentation update. We do need both to cut a release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit looks good!
@@ -478,6 +478,7 @@ def get_vol_path(datastore, tenant_name=None): | |||
# volumes is created on <datastore>/DOCK_VOLS_DIR | |||
# If the command is running under a tenant, the foler for Dock volume | |||
# is created on <datastore>/DOCK_VOLS_DIR/tenant_name | |||
dock_vol_path = os.path.join("/vmfs/volumes", datastore, DOCK_VOLS_DIR) | |||
if tenant_name: | |||
path = os.path.join("/vmfs/volumes", datastore, DOCK_VOLS_DIR, tenant_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.path.join(dock_vol_path, tenant_name). I am okay if you decide to leave it as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
6028fc9
to
4950692
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. I think we need to re-order tenant admin CLI commands.
|
||
October release is [out](https://github.com/vmware/docker-volume-vsphere/releases) | ||
October release is [out and updated to 0.8.1](https://github.com/vmware/docker-volume-vsphere/releases) | ||
This is a big release, it includes multi tenancy additions and the backend VMDK_OPS service is now multi-threaded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid adjectives like "big"! Perhaps highlight 2-3 most significant feature under "What's new?"?
I would also qualify "Multi-tenancy" as tech-preview.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -0,0 +1,13 @@ | |||
# Tenancy | |||
|
|||
Multi-tenancy is implemented via assigning VMs and Datastore to a tenant. A tenant can be granted access to create, delete or mount volumes on a specific datastore. VMs assigned to a tenant can then execute Docker volume APIs on an assigned datastores. Within a datastore multiple tenants can store their Docker volumes. A tenant cannot access volumes created by a different tenant, i.e. tenants have their own independent namespace even if tenants share datastores. VMs cannot be shared between tenants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about restructuring little bit;
Tenants can be used to provide isolation between independent groups in shared environment, where multiple groups are using a common infrastructure (compute, storage, network) fabric. Tenants are useful for isolating resources from one tenant from those of other tenants. For e.g. volumes created by TenantA will neither be visible nor accessible to TenantB, even if those volumes reside on storage shared by multiple tenants. In addition, vSphere Administrator can enforce access control policies and set storage quotas for each tenant.
Some attributes that define Tenant;
- vSphere Administrator can define group of one or more Docker Host (VM) as Tenant.
- Docker Host (VM) can be member of one and only one Tenant.
- Docker Host (VM) if not assigned to any Tenant are member of _Default tenant.
- vSphere Administrator can grant Tenant privileges & set resource consumption limits at granularity of datastore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will think about it. I do not want to explain tenancy itself but just how we implement it.
## References | ||
|
||
- [Design Spec for tenancy](https://github.com/vmware/docker-volume-vsphere/blob/master/docs/misc/docker-volume-auth-proposal.v1_2.md) | ||
- [Introduction to vSphere Storage](https://pubs.vmware.com/vsphere-60/index.jsp#com.vmware.vsphere.storage.doc/GUID-F602EB17-8D24-400A-9B05-196CEA66464F.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is relevance of this link here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are introducing terms such as datastore without giving any background.. the design spec is public and a good read, we can skip that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to handle _Default tenant is not implemented yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean "tenancy:ant privileges"? I don't understand. Any typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some changes to the write up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kerneltime datastore is vSphere basics and to use this feature you already need to be familiar with vSphere. I suggest to remove link to vSphere Storage. It seems out of place.
``` | ||
#### Add | ||
|
||
Add a datastore to be access by a tenant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grant datastore access to Tenant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
``` | ||
### Create | ||
|
||
Create a tenant that can be assigned VMs. Only the VMs assigned to the tenant will be able to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just limit it to "Creates a new named Tenant and assign VMs to Tenant". Also this should be first command in Admin CLI doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
-h, --help show this help message and exit | ||
``` | ||
|
||
### Access |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we should start with "create tenant".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
CC/ @asomaiah It would be great if you can take a quick look at docs. |
4950692
to
e7fbb8b
Compare
e7fbb8b
to
7aab639
Compare
Updated. Will review this again tomorrow. |
if a reviewer wants to look at the final output that will get posted to the project page
Open browser on same machine and head to 127.0.0.1:8000 More info https://github.com/vmware/docker-volume-vsphere/blob/master/CONTRIBUTING.md |
## References | ||
|
||
- [Design Spec for tenancy](https://github.com/vmware/docker-volume-vsphere/blob/master/docs/misc/docker-volume-auth-proposal.v1_2.md) | ||
- [Introduction to vSphere Storage](https://pubs.vmware.com/vsphere-60/index.jsp#com.vmware.vsphere.storage.doc/GUID-F602EB17-8D24-400A-9B05-196CEA66464F.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to handle _Default tenant is not implemented yet.
## References | ||
|
||
- [Design Spec for tenancy](https://github.com/vmware/docker-volume-vsphere/blob/master/docs/misc/docker-volume-auth-proposal.v1_2.md) | ||
- [Introduction to vSphere Storage](https://pubs.vmware.com/vsphere-60/index.jsp#com.vmware.vsphere.storage.doc/GUID-F602EB17-8D24-400A-9B05-196CEA66464F.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean "tenancy:ant privileges"? I don't understand. Any typo?
-h, --help show this help message and exit | ||
``` | ||
|
||
### Virtual Machine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to move the AdminCLI to add/remove virtual machine before those commands to manipulate datastore access privileges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
```bash | ||
/usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access add --name tenant1 --datastore datastore1 --rights create,delete,mount | ||
{'mount_volume': 1, 'global_visibility': 0, 'max_volume_size': 0, 'create_volume': 1, 'delete_volume': 1, 'datastore': 'datastore1', 'usage_quota': 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this line. It is a "print" which I forget to remove in the code. Please remove this print at Line 1008 (in function "tenant_access_add")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is part of this release we should keep it, once we update the CLI the documentation will need updating as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for you to include the small fix in this PR too? Just remove remove this print at Line 1008 (in function "tenant_access_add")? This should not affect any existing functionality. Just remove a unused print command which I added for debugging.
|
||
```bash | ||
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access add --name tenant1 --datastore datastore1 | ||
{'mount_volume': 0, 'global_visibility': 0, 'max_volume_size': 0, 'create_volume': 0, 'delete_volume': 0, 'datastore': 'datastore1', 'usage_quota': 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the CLI is updated we can change the documentation.
|
||
### Virtual Machine | ||
#### Add | ||
Add a VM to a tenant. A VM can only access the datastores for the tenant it is assigned to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more clear to move the example for "tenant vm add" command here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup.
``` | ||
### Remove | ||
|
||
Remove a tenant, optionally all volumes for a tenant can be removed as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "tenant rm" command will print "tenant rm succeeded" if command succeeded. Please include that in the command example.
``` | ||
|
||
The volume attributes are set and take effect only the next time the volume attached to a VM. The changes do not impact any VM | ||
thats currently using the volume. For the present, only the "access" attribute is supported to be modified via this command, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think set command now also support "attached_as" attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not show up in help, when the help is updated we can update the documentation.
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py set -h
usage: vmdkops_admin.py set [-h] --volume VOLUME --options OPTIONS
optional arguments:
-h, --help show this help message and exit
--volume VOLUME Volume to set options for, specified as
"volume@datastore".
--options OPTIONS Options (specifically, access) to be set on the volume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you file a issue to track this? If you don't want to fix this in current PR?
|
||
List all properties for all Docker volumes that exist on datastores accessible to the host. | ||
|
||
``` | ||
```bash | ||
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py ls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This need to be updated. The current output looks like this:
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py ls
Volume Datastore Created By VM Created Attached To VM Policy Capacity Used Filesystem Type Access Attach As
--------------- ---------- ------------- ------------------------ -------------- ------ -------- ------- --------------- ---------- ----------------------
non-tenant-vol1 datastore1 photon4 Sun Aug 14 09:36:45 2016 detached N/A 100.00MB 0B ext4 read-write independent_persistent
non-tenant-vol2 datastore1 photon4 Sun Aug 14 09:40:53 2016 detached N/A 100.00MB 0B ext4 read-write independent_persistent
non-tenant-vol3 datastore1 photon4 Thu Aug 18 07:22:24 2016 detached N/A 100.00MB 13.00MB ext4 read-write independent_persistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included a brief write up on tenants
## References | ||
|
||
- [Design Spec for tenancy](https://github.com/vmware/docker-volume-vsphere/blob/master/docs/misc/docker-volume-auth-proposal.v1_2.md) | ||
- [Introduction to vSphere Storage](https://pubs.vmware.com/vsphere-60/index.jsp#com.vmware.vsphere.storage.doc/GUID-F602EB17-8D24-400A-9B05-196CEA66464F.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some changes to the write up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Rather than documenting commands in sorted order, I would recommend to document workflow order.
## References | ||
|
||
- [Design Spec for tenancy](https://github.com/vmware/docker-volume-vsphere/blob/master/docs/misc/docker-volume-auth-proposal.v1_2.md) | ||
- [Introduction to vSphere Storage](https://pubs.vmware.com/vsphere-60/index.jsp#com.vmware.vsphere.storage.doc/GUID-F602EB17-8D24-400A-9B05-196CEA66464F.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kerneltime datastore is vSphere basics and to use this feature you already need to be familiar with vSphere. I suggest to remove link to vSphere Storage. It seems out of place.
``` | ||
### Create | ||
|
||
Creates a new named tenant and optionally assigns VMs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: assigns => assign?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cli cmd Creates and assigns..
|
||
Sample: | ||
``` | ||
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant create --name DevAppX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change DevAppX to TeamA or ProjectX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
Sample: | ||
``` | ||
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant ls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this sample output reflect result of "tenant create" above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try to keep the flow as much as possible. But then the commands need to follow a script rather than being independent.
tenant create succeeded | ||
``` | ||
|
||
The tenant to VM association can be done at create time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to move line after sample, just before vm-assignment sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
### Access | ||
|
||
Change the access control for a tenant. | ||
This includes ability to control which datastores are accessible by a tenant and controlling what a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This includes ability to grant privileges & set resource consumption limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
--datastore DATASTORE | ||
Datastore name | ||
``` | ||
### List |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is covered above with create?
32b029c
to
984c32f
Compare
Comments inline for why changes were not made.
Fixes #663 |
Add documentation for github page.
Fixes #687