VMODL API for multi-tenancy support #754

shaominchen · 2016-11-19T01:26:56Z

The implementation (to integrate with auth_api) has not been implemented yet. Please review the API signatures first.

msterin

Looks generally good , no comments for the the content of the model . JavaDocs should be a bit more elaborate, it's awfully short now :-).
[Sam] Agree with you. Will add more detailed JavaDocs.

More comments inline.

BTW I think we need to remove the Java file with the (obsolete) model draft
[Sam] Will do.

msterin · 2016-11-21T23:07:05Z

esx_service/vmodl/DatastoreAccessPrivilege.py

+
+   @JavaDocs(parent=_name, docs =
+   """
+   Max volume size allowed on this datastore


need units. I suggest the attribute name to have "in_bytes" or whatever at the end. Actually, in_Mib should be fine

This PR will include VMODL definitions only. In the next PR I will provide the implementation along with the unit tests.

Will add _in_MiB suffix.

msterin · 2016-11-21T23:07:23Z

esx_service/vmodl/DatastoreAccessPrivilege.py

+   """
+   )
+   @Attribute(parent=_name, typ="string")
+   def usage_quota(self):


Sure, will add _in_MiB suffix.

msterin · 2016-11-21T23:08:27Z

esx_service/vmodl/DatastoreAccessPrivilege.py

+
+   @JavaDocs(parent=_name, docs =
+   """
+   Datastore name


what if the datastore name changes (and url stays the same) ? Should we keep url here ?

Good catch! I will use datastore URL instead. BTW, this issue already exists in the existing logic. I have filed a tracking issue: #782.

msterin · 2016-11-21T23:09:55Z

esx_service/vmodl/Tenant.py

+
+   @JavaDocs(parent=_name, docs =
+   """
+   Tenant name


Any limitations ? Length ? Char set ? Can it be unicode ?

Will document the length limitation for name and description.

AFAIK, VMODL string supports unicode on all different language bindings: C++, Java, Python, etc. But this is implicit and we don't usually document this in the JavaDoc.

msterin · 2016-11-21T23:12:10Z

esx_service/vmodl/Tenant.py

+   """
+   )
+   @Method(parent=_name, wsdlName="AddVMs")
+   @Param(name="vms", typ="string[]")


a question abut all *VMs() operations: should we use vim.VirtualMachine MoRef instead of strings here ?

Will use MoRef consistently for all references of VMs and Datastores.

msterin · 2016-11-21T23:13:20Z

esx_service/vmodl/TenantManager.py

+   """
+   )
+   @Method(parent=_name, wsdlName="RemoveTenant")
+   @Param(name="name", typ="string")


vim.vcs.Tenant instead of string here ?

Since all Tenant related APIs use tenant name as the unique key, I think it makes sense to also use name as the key for this API. What do you think?

msterin · 2016-11-21T23:14:25Z

esx_service/vmodl/TenantManager.py

+
+   @JavaDocs(parent=_name, docs=
+   """
+   Query Tenants for the given name. Return all Tenants if name is null.


will we support globbing (e.g. ? *) If yes needs to be documented

I've thought about this. Globbing is typically used by shells for matching file and directory names using wildcards. Even though our Tenant name does map to a directory in the backend, this implementation detail should not be exposed to customers. Regular expressions are actually more commonly used in commands / functions for pattern matching in string text. However, supporting regex seems an overkill for our current requirements.

So, for now I think we can keep it simple, i.e. not supporting globbing or regex. In the future if there are real requirements for this, we can add this feature without breaking backward compatibility.

msterin · 2016-11-21T23:15:21Z

esx_service/vmodl/TenantManager.py

+   @Param(name="new_name", typ="string", flags=F_OPTIONAL)
+   @Param(name="new_description", typ="string", flags=F_OPTIONAL)
+   @Return(typ="void")
+   def RemoveTenant(self, name):


needs to be ModifyTenant

Copy-paste error. Will fix.

msterin · 2016-11-21T23:16:17Z

esx_service/vmodl/VsphereContainerService.py

+
+
+# _VERSION = newestVersions.Get("vim")
+_VERSION = 'vim.version.version10'


I think common modules import and version should be imported from a single file instead of copy-pasted to each module

Make sense, will introduce a common module for this.

I've decided to merge all classes into one single file/module as this seems more consistent with Python style. So no need to introduce a common module anymore.

lipingxue · 2016-11-28T22:49:38Z

esx_service/vmodl/Tenant.py

+   @Method(parent=_name, wsdlName="RemoveVMs")
+   @Param(name="vms", typ="string[]")
+   @Return(typ='void')
+   def RemoveVMsFromTenant(self, vms):


Rename to "RemoveVMs"

lipingxue · 2016-11-28T22:52:03Z

esx_service/vmodl/Tenant.py

+   This class provides operations to manage VMs and privileges associated with a tenant.
+   '''
+
+   _name = "vim.vcs.TenantManager"


_name should not be "vim.vcs.TenantManager". This class is a "Tenant" class.

Thanks for catching this. Will fix.

lipingxue

Overall looks good.
A general comment is we need to think about how to handle the exception in all VMODL functions? We should define some faults for "VsphereContainerService". This can be addressed in separate PR.

lipingxue · 2016-12-02T00:34:51Z

esx_service/vmodl/VsphereContainerService.py

+   )
+   @Method(parent=_name, wsdlName="RemoveTenant")
+   @Param(name="name", typ="string")
+   @Param(name="remove_volumes", typ="boolean")


param "remove_volumes" is a "optional" param.

All functions in class "TenantManager" can also through exception. For example "CreateTenant" may through an exception when the name specified is the same as existing tenant.

Will address this.

lipingxue · 2016-12-02T00:38:08Z

esx_service/vmodl/VsphereContainerService.py

+
+   @JavaDocs(parent=_name, docs =
+   """
+   Tenant is an abstraction of a group of VMs and the associated access


I think Tenant is an abstraction of "a group of VMs", but not the "associated access privileges".

Will clarify a little bit.

lipingxue · 2016-12-02T00:51:45Z

esx_service/vmodl/VsphereContainerService.py

+   """
+   )
+   @Method(parent=_name, wsdlName="AddPrivilege")
+   @Param(name="privilege", typ="vim.vcs.storage.DatastoreAccessPrivilege")


As we discussed, user should be able to switch "default_datastore" when adding a new datastore access privilege. So we need a new param "switch_default_datastore" here.

Yes, I will address this in next update.

shaominchen · 2016-12-02T01:09:28Z

@msterin @lipingxue

I've updated the PR based on your comments. Also redesigned some APIs. Major changes since last update:

Add detailed comments for all classes, methods, fields, parameters, return values and exceptions.
Add exception handling for all VMODL APIs
Use MoRef for all Datastore and VM references
Merge all classes into one single file/module

Exception handling is a major missing part that we haven't discussed before. I plan to introduce one or more exceptions to handle all different error conditions. Maybe we should have a discussion about this after everyone is back.

shaominchen · 2016-12-02T18:55:53Z

@lipingxue Addressed your comments.

msterin

LGTM. One nit about the naming - please take a look

msterin · 2016-12-08T21:26:41Z

esx_service/vmodl/VsphereContainerService.py

+        faults=["vim.fault.NotFound"])
+   @Param(name="datastore", typ="vim.Datastore", flags=F_OPTIONAL)
+   @Return(typ='vim.vcs.storage.DatastoreAccessPrivilege[]')
+   def GetPrivileges(self, datastore):


nit: it feels strange that AddPrivilige is singular but GetPrivileges is plural. I suggest to stick with singular everywhere.

I'm actually following existing VSAN VMODL style. Basically, if the return type is an array, the name will be plural, for example: GetTenants, GetVMs, GetPrivileges. Or if the input parameter is an array, the name is also plural: AddVMs, RemoveVMs, ReplaceVMs.

Please let me know if you still have concern about this. I can change it in the next PR if needed.

lipingxue · 2016-12-12T18:20:09Z

LGTM to me.

This brings in the VMODL Python APIs for multi tenancy. This is hard coded for return values for now, to help with testing.

vmwclabot added the cla-not-required label Nov 19, 2016

msterin reviewed Nov 21, 2016

View reviewed changes

lipingxue reviewed Nov 28, 2016

View reviewed changes

shaominchen force-pushed the vmodl_multi_tenancy.shchen branch from e1eee0d to dc4871e Compare December 1, 2016 23:58

lipingxue reviewed Dec 2, 2016

View reviewed changes

msterin approved these changes Dec 8, 2016

View reviewed changes

lipingxue and others added 14 commits December 12, 2016 10:53

VMODL work for multi-tenancy

fce359d

This brings in the VMODL Python APIs for multi tenancy. This is hard coded for return values for now, to help with testing.

Add and Modify datastore access privileges API.

6385e27

Add VMODL API to remove/list datastore access privileges.

778f204

VMODI API for multi-tenancy support

23fef5c

Remove deprecated classes

6bbc5bc

Remove the deprecate test class

772948d

Fix a typo

0082934

Remove obsolete Java VMODL draft file

6d68705

Addressed review comments

32351d6

Addressed review comments

753bec5

Merge to one single file

a4efa41

Add some more exception handlings

8ff6776

Addressed Liping's comments.

fcdf1f3

Minor fix

1667720

shaominchen force-pushed the vmodl_multi_tenancy.shchen branch from ad3623a to 1667720 Compare December 12, 2016 19:01

shaominchen changed the title ~~VMODL API for multi-tenancy support [DRAFT]~~ VMODL API for multi-tenancy support Dec 12, 2016

shaominchen merged commit 5630254 into master Dec 12, 2016

shaominchen deleted the vmodl_multi_tenancy.shchen branch December 12, 2016 19:11



		# _VERSION = newestVersions.Get("vim")
		_VERSION = 'vim.version.version10'

VMODL API for multi-tenancy support #754

VMODL API for multi-tenancy support #754

Conversation

shaominchen commented Nov 19, 2016 • edited Loading

msterin left a comment • edited by shaominchen 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

shaominchen Nov 29, 2016 • 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

shaominchen Nov 30, 2016 • 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

shaominchen Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

lipingxue 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

shaominchen commented Dec 2, 2016

shaominchen commented Dec 2, 2016

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

lipingxue commented Dec 12, 2016

shaominchen commented Nov 19, 2016 •

edited

Loading

msterin left a comment •

edited by shaominchen

Loading

shaominchen Nov 29, 2016 •

edited

Loading

shaominchen Nov 30, 2016 •

edited

Loading

shaominchen Nov 30, 2016 •

edited

Loading