9:58 a.m.
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
The next milestone is creating/defining a new domain from a domain XML
config, but this will not be finished until feature freeze for 0.7.0
on friday.
Regards,
Matthias
3:42 p.m.
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
The next milestone is creating/defining a new domain from a domain XML
config, but this will not be finished until feature freeze for 0.7.0
on friday.
Okay, at this point since we have import/export of the XML I think
embedding the driver makes sense, then the Create and others part can be
added incrementally. I will make another pass on the code and hopefully
we will be able to push it in by Friday,
thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:52 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
11:24 a.m.
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.
What is a 'phantom' URI / connection ? eg
esx:///phantom
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
12:50 p.m.
On Wed, Jul 22, 2009 at 05:24:09PM +0100, Daniel P. Berrange wrote:
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.
What is a 'phantom' URI / connection ? eg
esx:///phantom
I wondered that too, my guess was that it was a way to work without an
actual ESX server (possibly during the cluster being offline :-)
I didn't finished my review that was one of my questions :-)
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:02 a.m.
2009/7/22 Daniel Veillard <veillard(a)redhat.com>:
On Wed, Jul 22, 2009 at 05:24:09PM +0100, Daniel P. Berrange wrote:
> 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.
>
> What is a 'phantom' URI / connection ? eg
>
> esx:///phantom
I wondered that too, my guess was that it was a way to work without an
actual ESX server (possibly during the cluster being offline :-)
I didn't finished my review that was one of my questions :-)
Yes, esx:///phantom helped to develop esxVMX_ParseConfig(), because
the VMX <-> domain XML mapping doesn't require an actual connection to
the hypervisor.
The phantom URI allows to refer to the ESX driver without referring to
an actual physical machine and without the need to provide login
information. Its currently intended use case is this:
virsh -c esx:///phantom domain-xml-from-native vmware-vmx dummy.vmx
Regards,
Matthias
8:48 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:34 a.m.
2009/7/23 Daniel Veillard <veillard(a)redhat.com>:
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 ?
The phantom mode allows for using domain-xml-to/from-native without an
actual ESX server. But besides that, it has no real use.
> + 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)
The driver open function has the URI available is parsed xmlURIPtr form only.
[...]
> +
> + 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 :-)
The driver needs to talk to different hosts for different
functionality. Most stuff can be done with the ESX host itself, but a
vCenter is needed for migration. If the connection URI contains a
vCenter parameter the open function requests authentication for the
ESX host and the vCenter independently, because authentication
information can be different for each of them.
[...]
> +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 :-)
See esxDomainXMLFromNative().
> +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 :)
The underscore is superfluous, will be cleaned up. The loop iterates
over all domains and searches for the matching one.
esxVI_GetVirtualMachineIdentity() allocates the name, so the name must
be freed before calling esxVI_GetVirtualMachineIdentity() again to
free the allocated name from the previous iteration. Otherwise memory
could leak.
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
Already on the todo list.
> +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()
Added to the todo list.
> +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.
Already on the todo list.
> +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):
Added to the todo list.
> + 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.
I'll have a look at the virXPath* functions. Added to the todo list.
> 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 !!!
Well, our client implements a small subset of the VI API, only the
methods and types that are really needed for the driver. csoap
generates code for the whole thing.
> +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 ?
As the comment states, the remoteRequest->xpathExpression is a static
string (not allocated) and esxVI_RemoteRequest_Free() would free it.
So setting it to NULL before freeing prevents
esxVI_RemoteRequest_Free() from freeing non-allocated memory and
doesn't generate a leak.
Maybe I'll reverse the logic and esxVI_RemoteRequest_Free() doesn't
free xpathExpression anymore, because in most cases xpathExpression is
non-allocated. Maybe I'll also precompile the static XPath expressions
as you or DPB suggested earlier. Anyway, it's already on the todo list
to have a look at the XPath handling again.
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 _() ...
The string could be moved out of the macro.
Before the macro:
static const char *someErrorMessage = gettext_noop("XML node doesn't
contain text, expecting an %s value");
And inside the macro:
ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, gettext(someErrorMessage), _xsdType);
Or even this inside the macro may work:
ESX_VI_ERROR(conn, VIR_ERR_INTERNAL_ERROR, _("XML node doesn't contain
text, expecting an %s value"), _xsdType);
> 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 ...
Do you refer to "# FIXME: maybe not supported by ESX?"? This is just a
remark for me.
http://www.sanbarrow.com/vmx.html states that there are 4 modes for
networks. But it's not clear if all of them are supported by the ESX
host, because http://www.sanbarrow.com/vmx.html contains information
about the general VMX format that VMware uses for its other
virtualisation software too. I just haven't tested yet if hostonly and
nat mode are supported by ESX.
> +## 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.
Already on the todo list.
Regards,
Matthias
3:33 p.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:47 a.m.
2009/7/23 Daniel Veillard <veillard(a)redhat.com>:
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 !
Thanks!
If you have a computer with a supported NIC (an Intel NIC will do) and
some time, you could download the stripped down ESXi 3.5
60-days-evaluation version from VMware, dd it onto an USB thumb drive,
boot from it and provide some storage via NFS from a second computer.
You could also provide storage from a local harddisk if you have a
supported SCSI controller (SATA may also work, I'm not sure). That's
how I started playing with ESX before we had out testing cluster
available.
Regards,
Matthias
5599
days inactive
5603
days old
9 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Matthias Bolte