On Tue, Jul 21, 2009 at 04:58:50PM +0200, Matthias Bolte wrote:

Hi,

I checked the diff patch, i.e. change to the current infrastructure, it all look clean and uncontroversial. I think it just misses adequate changes to the spec file including libcurl BuidDeps but I will handle this one :-) Still need to review the new files in src/esx/ which is obviously larger, but well contained. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Tue, Jul 21, 2009 at 04:58:50PM +0200, Matthias Bolte wrote:

Now reviewing the main code, [...]

+typedef struct _esxPrivate { + esxVI_Context *host; + esxVI_Context *vcenter; + int phantom; // boolean

Do we really need phantom ? this is basically a debug mode with no real ESX server, right ?

+ char *transport; + int32_t nvcpus_max; + esxVI_Boolean supports_vmotion; + int32_t usedCpuTimeCounterId; +} esxPrivate; + [...] + /* Check for 'esx:///phantom' URI */ + if (conn->uri->server == NULL && conn->uri->path != NULL && + STREQ(conn->uri->path, "/phantom")) { + phantom = 1; + }

Comparing against the exact string would have been simpler/more accurate if still available (to avoid esx:///phantom#foo or esx:///phantom?bar) [...]

+ + password = esxUtil_RequestPassword(auth, username, conn->uri->server); [...] + + if (vcenter != NULL) { + if (virAsprintf(&url, "%s://%s/sdk", priv->transport, + vcenter) < 0) { + virReportOOMError(conn); + goto failure; + } + + if (esxVI_Context_Alloc(conn, &priv->vcenter) < 0) { + goto failure; + } + + username = esxUtil_RequestUsername(auth, "administrator", vcenter); + + if (username == NULL) { + ESX_ERROR(conn, VIR_ERR_AUTH_FAILED, + "Username request failed"); + goto failure; + } + + password = esxUtil_RequestPassword(auth, username, vcenter);

I'm not sure I understand why there are 2 requests for authentication needed but there must be a reason :-) [...]

+static esxVI_Boolean +esxSupportsVMotion(virConnectPtr conn) +{ + esxPrivate *priv = (esxPrivate *)conn->privateData; + esxVI_String *propertyNameList = NULL; + esxVI_ObjectContent *hostSystem = NULL; + esxVI_DynamicProperty *dynamicProperty = NULL; + + if (priv->phantom) { + ESX_ERROR(conn, VIR_ERR_OPERATION_INVALID, + "Not possible with a phantom connection"); + goto failure; + }

I still didn't found the use for phantom :-)

+ if (priv->supports_vmotion != esxVI_Boolean_Undefined) { + return priv->supports_vmotion; + } + + if (esxVI_EnsureSession(conn, priv->host) < 0) { + goto failure; + } + + if (esxVI_String_AppendValueToList(conn, &propertyNameList, + "capability.vmotionSupported") < 0 || + esxVI_GetObjectContent(conn, priv->host, priv->host->hostFolder, + "HostSystem", propertyNameList, + esxVI_Boolean_True, &hostSystem) < 0) { + goto failure; + } + + if (hostSystem == NULL) { + ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, + "Could not retrieve the HostSystem object"); + goto failure; + } + + for (dynamicProperty = hostSystem->propSet; dynamicProperty != NULL; + dynamicProperty = dynamicProperty->_next) { + if (STREQ(dynamicProperty->name, "capability.vmotionSupported")) { + if (esxVI_AnyType_ExpectType(conn, dynamicProperty->val, + esxVI_Type_Boolean) < 0) { + goto failure; + } + + priv->supports_vmotion = dynamicProperty->val->boolean; + break; + } else { + VIR_WARN("Unexpected '%s' property", dynamicProperty->name); + } + } + + cleanup: + esxVI_String_Free(&propertyNameList); + esxVI_ObjectContent_Free(&hostSystem); + + return priv->supports_vmotion; + + failure: + priv->supports_vmotion = esxVI_Boolean_Undefined; + + goto cleanup; +}

Okay most driver entry points are similar query/response/parsing and conversion to expected output [...]

+static virDomainPtr +esxDomainLookupByID(virConnectPtr conn, int id) +{ [...] + char *name_ = NULL;

why the underscore. I was a bit puzzled by the way name_ is freed in the loop but in retrospection that works :) [...]

+static virDomainPtr +esxDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) +{ [...] + char *name_ = NULL;

same here.

+ goto failure; + }

One thing we need to fix is all the erro strings need to be localized

+ if (powerState != esxVI_VirtualMachinePowerState_PoweredOn) { + ESX_ERROR(domain->conn, VIR_ERR_OPERATION_INVALID, + "Domain is not powered on");

should become _("Domain is not powered on") But that's really simple to change

+static char * +esxDomainGetOSType(virDomainPtr dom ATTRIBUTE_UNUSED) +{ + return strdup("hvm");

we need to check for failure and use the out of memory handler here. like in esxDomainGetSchedulerType() [...]

+static char * +esxDomainDumpXML(virDomainPtr domain, int flags) +{ [..] + /* expected format: "[<datastoreName>] <vmxPath>" */ + if (sscanf(vmPathName, "[%a[^]%]] %as", &datastoreName, &vmxPath) != 2) { + ESX_ERROR(domain->conn, VIR_ERR_OPERATION_INVALID, + "'config.files.vmPathName' property '%s' doesn't have " + "expected format '[<datastore>] <vmx>'", vmPathName); + goto failure; + } + + if (virAsprintf(&url, "%s://%s/folder/%s?dcPath=%s&dsName=%s", + priv->transport, domain->conn->uri->server, + vmxPath, priv->host->datacenter->value, + datastoreName) < 0) { + virReportOOMError(domain->conn); + goto failure; + } + + if (esxVI_Context_Download(domain->conn, priv->host, url, &vmx) < 0) { + goto failure; + } + + def = esxVMX_ParseConfig(domain->conn, vmx); + + if (def != NULL) { + xml = virDomainDefFormat(domain->conn, def, flags); + }

Rather cool :-) though the details lies in esxVMX_ParseConfig, a long function but not too nasty.

+static int +esxDomainGetSchedulerParameters(virDomainPtr domain, + virSchedParameterPtr params, int *nparams) +{

Hum, the scheduler API is so free form that some details of the scheduling parameter handled and their semantic need to be documented, here, and in the future HTML driver page.

+ snprintf (params[i].field, VIR_DOMAIN_SCHED_FIELD_LENGTH, "%s", + "reservation"); [...] + snprintf (params[i].field, VIR_DOMAIN_SCHED_FIELD_LENGTH, "%s", + "limit"); [...] + snprintf (params[i].field, VIR_DOMAIN_SCHED_FIELD_LENGTH, "%s", + "shares");

and same for here:

+static int +esxDomainSetSchedulerParameters(virDomainPtr domain, + virSchedParameterPtr params, int nparams) +{

Pointers to the 3 definitions on VMWare doc could be sufficient though

+ + /* Validate the purposed migration */ + if (esxVI_ValidateMigration(domain->conn, priv->vcenter, + virtualMachine->obj, + esxVI_VirtualMachinePowerState_Undefined, + NULL, resourcePool, hostSystem->obj, + &eventList) < 0) { + goto failure; + }

Migration checking, that's something we were planning at the libvirt API level too ! [...]

+static virDriver esxDriver = { + VIR_DRV_ESX, + "ESX", + esxOpen, /* open */ + esxClose, /* close */ + esxSupportsFeature, /* supports_feature */ + esxGetType, /* type */ + esxGetVersion, /* version */ + esxGetHostname, /* hostname */ + NULL, /* getMaxVcpus */ + esxNodeGetInfo, /* nodeGetInfo */ + NULL, /* getCapabilities */ + esxListDomains, /* listDomains */ + esxNumberOfDomains, /* numOfDomains */ + NULL, /* domainCreateXML */ + esxDomainLookupByID, /* domainLookupByID */ + esxDomainLookupByUUID, /* domainLookupByUUID */ + esxDomainLookupByName, /* domainLookupByName */ + esxDomainSuspend, /* domainSuspend */ + esxDomainResume, /* domainResume */ + esxDomainShutdown, /* domainShutdown */ + esxDomainReboot, /* domainReboot */ + esxDomainDestroy, /* domainDestroy */ + esxDomainGetOSType, /* domainGetOSType */ + esxDomainGetMaxMemory, /* domainGetMaxMemory */ + esxDomainSetMaxMemory, /* domainSetMaxMemory */ + esxDomainSetMemory, /* domainSetMemory */ + esxDomainGetInfo, /* domainGetInfo */ + NULL, /* domainSave */ + NULL, /* domainRestore */ + NULL, /* domainCoreDump */ + esxDomainSetVcpus, /* domainSetVcpus */ + NULL, /* domainPinVcpu */ + NULL, /* domainGetVcpus */ + esxDomainGetMaxVcpus, /* domainGetMaxVcpus */ + NULL, /* domainGetSecurityLabel */ + NULL, /* nodeGetSecurityModel */ + esxDomainDumpXML, /* domainDumpXML */ + esxDomainXMLFromNative, /* domainXmlFromNative */ + NULL, /* domainXmlToNative */ + esxListDefinedDomains, /* listDefinedDomains */ + esxNumberOfDefinedDomains, /* numOfDefinedDomains */ + esxDomainCreate, /* domainCreate */ + NULL, /* domainDefineXML */ + NULL, /* domainUndefine */ + NULL, /* domainAttachDevice */ + NULL, /* domainDetachDevice */ + NULL, /* domainGetAutostart */ + NULL, /* domainSetAutostart */ + esxDomainGetSchedulerType, /* domainGetSchedulerType */ + esxDomainGetSchedulerParameters, /* domainGetSchedulerParameters */ + esxDomainSetSchedulerParameters, /* domainSetSchedulerParameters */ + esxDomainMigratePrepare, /* domainMigratePrepare */ + esxDomainMigratePerform, /* domainMigratePerform */ + esxDomainMigrateFinish, /* domainMigrateFinish */ + NULL, /* domainBlockStats */ + NULL, /* domainInterfaceStats */ + NULL, /* domainBlockPeek */ + NULL, /* domainMemoryPeek */ + NULL, /* nodeGetCellsFreeMemory */ + NULL, /* nodeGetFreeMemory */ + NULL, /* domainEventRegister */ + NULL, /* domainEventDeregister */ + NULL, /* domainMigratePrepare2 */ + NULL, /* domainMigrateFinish2 */ + NULL, /* nodeDeviceDettach */ + NULL, /* nodeDeviceReAttach */ + NULL, /* nodeDeviceReset */ +};

That's not complete, but IMHO well worth providing at this point !

+char * +esxUtil_RequestUsername(virConnectAuthPtr auth, const char *default_username, + const char *server) +{ [...] +char * +esxUtil_RequestPassword(virConnectAuthPtr auth, const char *username, + const char *server) +{

auth hooks looks clean to me might be worth documenting what some of those low level utilities do, as it's not clear just from reading them, especially with things like handling different server version (assuming I understood):

+int +esxUtil_ParseQuery(virConnectPtr conn, char **transport, char **vcenter) +{ [...] +} [...] + if (errcode != 0) { + ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, + "IP address lookup for host '%s' failed: %s", hostname,

error message localisation is missing in this module too. [...]

diff --git a/src/esx/esx_util.h b/src/esx/esx_util.h [...] diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c [...] + /* + * Add a dummy expect header to stop CURL from waiting for a response code + * 100 (Continue) from the server before continuing the POST operation. + * Waiting for this response would slowdown each communication with the + * server by approx. 2 sec, because the server doesn't send the expected + * 100 (Continue) response and the wait times out resulting in wasting + * approx. 2 sec per POST operation. + */

I remember that one from my first review :-)

+ if (ctx->vmFolder == NULL || ctx->hostFolder == NULL) { + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, + "The 'datacenter' object is missing the " + "'vmFolder'/'hostFolder' propoerty");

should be 'property' and between _() :-)

+ if ((*remoteResponse)->response_code == 500) { + (*remoteResponse)->xpathObject = + xmlXPathEval(BAD_CAST + "/soapenv:Envelope/soapenv:Body/soapenv:Fault", + (*remoteResponse)->xpathContext); + + if (esxVI_RemoteResponse_DeserializeXPathObject + (conn, *remoteResponse, + (esxVI_RemoteResponse_DeserializeFunc) + esxVI_Fault_Deserialize, + (void **)&fault) < 0) { + goto failure; + }

the XPath code and handling could probably be made a bit easier and more resistant by using libvirt own wrapper, for example virXPathNode(NULL, "/soapenv:Envelope/soapenv:Body/soapenv:Fault" (*remoteResponse)->xpathContext); or virXPathNodeSet if multiple soapenv:Fault are allowed.

+ ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, + "HTTP response code %d. VI Fault: %s - %s", + (int)(*remoteResponse)->response_code, + fault->faultcode, fault->faultstring); + + goto failure; + } else { + (*remoteResponse)->xpathObject = + xmlXPathEval(BAD_CAST remoteRequest->xpathExpression, + (*remoteResponse)->xpathContext); + } + } else if ((*remoteResponse)->response_code != 200) { + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, + "HTTP response code %d", + (int)(*remoteResponse)->response_code); + + goto failure; + }

but that's not urgent in any way [...]

+int +esxVI_RemoteResponse_DeserializeXPathObject + (virConnectPtr conn, esxVI_RemoteResponse *remoteResponse, + esxVI_RemoteResponse_DeserializeFunc deserializeFunc, void **item) +{ + xmlNodePtr node = NULL; + + if (remoteResponse->xpathObject != NULL && + remoteResponse->xpathObject->type == XPATH_NODESET) { + if (remoteResponse->xpathObject->nodesetval->nodeNr != 1) {

that would just avoid this kind of nasty code present in all deserialization routines. [...]

+int +esxVI_BuildFullTraversalSpecList(virConnectPtr conn, + esxVI_SelectionSpec **fullTraversalSpecList) +{ + if (fullTraversalSpecList == NULL || *fullTraversalSpecList != NULL) { + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument"); + return -1; + } + + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList, + "visitFolders", + "Folder", "childEntity", + "visitFolders\0" + "datacenterToVmFolder\0" + "datacenterToHostFolder\0" + "computeResourceToHost\0" + "computeResourceToResourcePool\0" + "HostSystemToVm\0" + "resourcePoolToVm\0") < 0) { + goto failure; + } + + /* Traversal through vmFolder branch */ + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList, + "datacenterToVmFolder", + "Datacenter", "vmFolder", + "visitFolders\0") < 0) { + goto failure; + } + + /* Traversal through hostFolder branch */ + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList, + "datacenterToHostFolder", + "Datacenter", "hostFolder", + "visitFolders\0") < 0) { + goto failure; + } + + /* Traversal through host branch */ + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList, + "computeResourceToHost", + "ComputeResource", "host", + NULL) < 0) { + goto failure; + } + + /* Traversal through resourcePool branch */ + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList, + "computeResourceToResourcePool", + "ComputeResource", "resourcePool", + "resourcePoolToResourcePool\0" + "resourcePoolToVm\0") < 0) { + goto failure; + } + + /* Recurse through all resource pools */ + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList, + "resourcePoolToResourcePool", + "ResourcePool", "resourcePool", + "resourcePoolToResourcePool\0" + "resourcePoolToVm\0") < 0) { + goto failure; + } + + /* Recurse through all hosts */ + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList, + "HostSystemToVm", + "HostSystem", "vm", + "visitFolders\0") < 0) { + goto failure; + } + + /* Recurse through all resource pools */ + if (esxVI_BuildFullTraversalSpecItem(conn, fullTraversalSpecList, + "resourcePoolToVm", + "ResourcePool", "vm", NULL) < 0) { + goto failure; + } + + return 0; + + failure: + esxVI_SelectionSpec_Free(fullTraversalSpecList); + + return -1; +}

Now that I see this again I remember how perplex I was about the VI API when I tried working with it at the time ... [...]

diff --git a/src/esx/esx_vi.h b/src/esx/esx_vi.h

The header is incomparably simpler than what would be generated by csoap !!!

+int +esxVI_SessionIsActive(virConnectPtr conn, esxVI_Context *ctx, + const char *sessionID, const char *userName, + esxVI_Boolean *active) +{ [...] + cleanup: + /* + * Remove static values from the data structures to prevent them from + * being freed by the call to esxVI_RemoteRequest_Free(). + */ + if (remoteRequest != NULL) { + remoteRequest->xpathExpression = NULL; + }

Hum, and that doesn't generate a leak ? I see that others routines do the same, or is remoteRequest->xpathExpression still stored somewhere else ?

+ esxVI_RemoteRequest_Free(&remoteRequest); + esxVI_RemoteResponse_Free(&remoteResponse); + + return result; + + failure: + free(virBufferContentAndReset(&buffer)); + + result = -1; + + goto cleanup; +}

diff --git a/src/esx/esx_vi_methods.h b/src/esx/esx_vi_methods.h [...] diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c

Hum more localized strings needed here ... even in convenience macros

+ if (string == NULL) { \ + ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, \ + "XML node doesn't contain text, expecting an " \ + _xsdType" value"); \

The problem is that the string is generated as part of the macro, I wonder how this will play with _() ... Not much to say about the type handling, except it's heavy !!!

diff --git a/src/esx/esx_vi_types.h b/src/esx/esx_vi_types.h

even the list of entry point weights quite a bit.

diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c [...] +## nets: bridged ############################################################### + +... +->type = _NET_TYPE_BRIDGE <=> ethernet0.connectionType = "bridged" # defaults to "bridged" + + +## nets: hostonly ############################################################## + +... # FIXME: maybe not supported by ESX? +->type = _NET_TYPE_NETWORK <=> ethernet0.connectionType = "hostonly" # defaults to "bridged" + + +## nets: nat ################################################################### + +... # FIXME: maybe not supported by ESX? +->type = _NET_TYPE_USER <=> ethernet0.connectionType = "nat" # defaults to "bridged" +

I not so sure about what the question is there ...

+## nets: custom ################################################################ + +... +->type = _NET_TYPE_BRIDGE <=> ethernet0.connectionType = "custom" # defaults to "bridged" +->data.bridge.brname = <value> <=> ethernet0.vnet = "<value>"

One thing which should be added is test cases for the VMX <-> XML formats conversions, should be relatively easy to add in the infrastructure once we have the examples.

diff --git a/src/esx/esx_vmx.h b/src/esx/esx_vmx.h

Okay, beside those comments I think this is in a shape good enough for commit, then I suggest to start refinements as incremental patches. One of the challenges will be the localization of strings, I expect a huge amount to result from those modules. I think the .syms will need adjustment too, especially if we want to hook up regression tests for the VMX parsing. ACK from me, that's an impressive patch, but I'm not tempted to rereview it once again in full :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Tue, Jul 21, 2009 at 04:58:50PM +0200, Matthias Bolte wrote:

Hi,

the development was hindered by our testing cluster being offline since 2 weeks due to server room cooling system maintenance. But I finally got a basic version of dump-XML done, that fills all fields of the virDomainDef that the VMX file contains data for.

Changes since first announcement:

- Move code into esx subdirectory - Add esxNodeGetInfo() - Fix esxDomainGetInfo() to report the correct value for memory - Add memory and max-memory getter/setters - Add CPU scheduler getter/setters - Validate a migration before trying to perform it - Replace esxUtil_Strdup() with strdup() and remove esxUtil_MigrateStringFromLibXML() - Add esxVI_EnsureSession() to handle expiring sessions - Separate VI client code into multiple files and generate most of the type handling code with macros - Add esxDomainDumpXML() based on esxVMX_ParseConfig()

The ESX driver isn't complete yet, currently it supports:

- domain lookup by ID, UUID and name - domain listing - domain info retrieval - domain suspend and resume - domain start and destroy - domain reboot and shutdown, if the VMware tools are installed inside the domain - domain migration with previous validation - domain memory configuration - domain CPU amount and scheduler configuration - domain XML-dump - domain XML-from-native (VMX) - node info retrieval - node hostname retrieval

It's in ! I have no way to really test it (except for the phantom trick) but it's commited in git, congratulations, well done ! There is quite a bit left to do but we can handle this incrementally from now :-) Thanks a lot, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/