[Libvir] [PATCH][RFC] libvirt ldoms support
Eunice Moon
Eunice.Moon at Sun.COM
Thu Apr 10 01:52:44 UTC 2008
Hi Daniel,
Thanks for your comments. Please see my responses inline below.
Eunice
Daniel Veillard wrote:
> Hi Ryan and Eunice,
>
> First, thanks for the contribution !
>
>> LDoms Support
>>
>> This patch adds the Logical Domains (LDoms) support for the SPARC
>> platforms. LDoms software is Sun Microsystem's virtualization technology
>> to subdivide a supported system's resources (CPUs, memory, I/O, and
>> storage) creating partitions called logical domains. The Logical Domains
>> Manager is used to create and manage logical domains and maps logical
>> domains to physical resources. The LDoms Manager provides a command-line
>> interface and also exports an XML-based control interface. The Libvirt
>> for LDoms uses this XML interface to communicate with the LDoms Manager
>> to retrieve the LDoms data for:
>> - Listing domains
>> - Requesting CPU and memory resource updates
>> - Performing life-cycle actions for logical domains
>>
>> This libvirt patch supports LDoms 1.0.1 and 1.0.2.
>
> okay
>
>> This patch will modify the following existing files:
>> src/libvirt.c
>> src/virsh.c
>> src/virterror.c
>> src/driver.h
>> src/Makefile.am
>> include/libvirt/libvirt.h.in
>> include/libvirt/virterror.h
>> configure.in
>>
>> and add the following new files:
>> src/ldoms_common.h
>> src/ldoms_internal.h
>> src/ldoms_internal.c
>> src/ldoms_intfc.h
>> src/ldoms_intfc.c
>> src/ldoms_xml_parse.h
>> src/ldoms_xml_parse.c
>
> I think most of the issues have been raised in the thread started by Richard
> One think I appreciate in the code is the amount of comments (would have been
> perfect if the function comment style could be realigned to libvirt one,
> i don't know if that can be done automatically).
> Do you have example of the XML domain configurations, it's a bit hard to derive
> from the parsing code, and i would prefer to have them aligned with other
> container/partition hypervisors. And then have them documented in the
> public format pages like the others
> http://libvirt.org/format.html
> (we need to add OpenVZ/LinuxContainers exampels too there).
I've attached a couple of LDoms XML examples.
>
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -48,6 +48,9 @@
>> #ifdef WITH_LXC
>> #include "lxc_driver.h"
>> #endif
>> +#ifdef WITH_LDOMS
>> +extern int ldomsRegister(void);
>> +#endif
>
> Sounds to me it's cleaner to export the API from an ldom driver header and
> include the header, that way the signature is checked between the producer and
> the consumer.
OK. I will create the ldoms_driver.h file with the ldomsRegister()
and include that header file like Linux Containers.
>
>> +#ifdef WITH_LDOMS
>> +/**
>> + * virLDomConsole:
>> + * @domain: the domain if available
>> + *
>> + * Opens a terminal window to the console for a domain
>> + *
>> + * Returns -1 in case of error, LDom console port number in case of success
>> + */
>> +int
>> +virLDomConsole(virDomainPtr domain)
>
> virLDomConsole is really a no go, this can't be added to the list of exported
> symbols from the library. this was already discussed in the thread, but I
> want to raise it again w.r.t. the exported symbols set, this is regulated
> on the gcc tool chain with the src/libvirt_sym.version file, it's not modified
> anytwhere in the file and since you use virLDomConsole from virsh this implies
> that you export all C public symbols by default in the library, I really think
> this need fixing, we can add a specific linker file for Solaris in the
> distribution. that's not a problem, but the exported symbols set need to be
> controlled.
OK. I will remove virLDomConsole.
>
>> --- a/src/virsh.c
>
> Basically no change to virsh should be needed for adding a new driver,
> so it's a NACk here.
OK.
>
>> diff --git a/src/virterror.c b/src/virterror.c
>
> In general most of the #ifdef/#ifndef WITH_LDOMS need to go. The public API
> need to be identical for all drivers and platforms.
OK.
>
>> -
>> +#ifdef WITH_LDOMS
>> + case VIR_FROM_LDOMS:
>> + dom = "LDoms ";
>> + break;
>> +#endif
>
> [...]
>> diff --git a/src/driver.h b/src/driver.h
>> --- a/src/driver.h
>> +++ b/src/driver.h
>> @@ -24,7 +24,10 @@ typedef enum {
>> VIR_DRV_QEMU = 3,
>> VIR_DRV_REMOTE = 4,
>> VIR_DRV_OPENVZ = 5,
>> - VIR_DRV_LXC = 6
>> + VIR_DRV_LXC = 6,
>> +#ifdef WITH_LDOMS
>> + VIR_DRV_LDOMS = 7
>> +#endif
>> } virDrvNo;
>>
>
>
>> @@ -253,6 +256,11 @@ typedef virDomainPtr
>> const char *uri,
>> unsigned long flags);
>>
>> +#ifdef WITH_LDOMS
>> +typedef int
>> + (*virDrvLDomConsole) (virDomainPtr domain);
>> +#endif
>> +
>> typedef struct _virDriver virDriver;
>> typedef virDriver *virDriverPtr;
>>
>> @@ -337,6 +345,9 @@ struct _virDriver {
>> virDrvDomainInterfaceStats domainInterfaceStats;
>> virDrvNodeGetCellsFreeMemory nodeGetCellsFreeMemory;
>> virDrvNodeGetFreeMemory getFreeMemory;
>> +#ifdef WITH_LDOMS
>> + virDrvLDomConsole ldomConsole;
>> +#endif
>> };
>
> in the short term export of the console informations in the XML is the
> normal option, then a generic console extracting API may be provided if
> needed , but adding this is premature.
>
>> typedef int
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -549,6 +549,9 @@ typedef enum {
>> VIR_VCPU_OFFLINE = 0, /* the virtual CPU is offline */
>> VIR_VCPU_RUNNING = 1, /* the virtual CPU is running */
>> VIR_VCPU_BLOCKED = 2, /* the virtual CPU is blocked on resource */
>> +#ifdef WITH_LDOMS
>> + VIR_VCPU_UNKNOWN = 3, /* the virtual CPU state is unknown */
>> +#endif
>> } virVcpuState;
>>
>> typedef struct _virVcpuInfo virVcpuInfo;
>> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
>> --- a/include/libvirt/virterror.h
>> +++ b/include/libvirt/virterror.h
>> @@ -56,6 +56,9 @@ typedef enum {
>> VIR_FROM_STATS_LINUX, /* Error in the Linux Stats code */
>> VIR_FROM_LXC, /* Error from Linux Container driver */
>> VIR_FROM_STORAGE, /* Error from storage driver */
>> +#ifdef WITH_LDOMS
>> + VIR_FROM_LDOMS, /* Error from LDoms driver */
>> +#endif
>> } virErrorDomain;
>>
>>
>> @@ -139,6 +142,9 @@ typedef enum {
>> VIR_WAR_NO_STORAGE, /* failed to start storage */
>> VIR_ERR_NO_STORAGE_POOL, /* storage pool not found */
>> VIR_ERR_NO_STORAGE_VOL, /* storage pool not found */
>> +#ifdef WITH_LDOMS
>> + VIR_ERR_INVALID_OPTION, /* invalid command line option */
>> +#endif
>> } virErrorNumber;
>
> removal of all #ifdef/#endif , one API to bind them all
OK.
>
>> diff --git a/configure.in b/configure.in
>> --- a/configure.in
>> +++ b/configure.in
>> @@ -246,6 +246,10 @@ if test "$with_remote" = "yes" ; then
>> LIBVIRT_FEATURES="$LIBVIRT_FEATURES -DWITH_REMOTE"
>> fi
>>
>> +if test "$with_ldoms" = "yes" ; then
>> + LIBVIRT_FEATURES="$LIBVIRT_FEATURES -DWITH_LDOMS"
>> +fi
>> +
>
> I'm a bit surprized to not see the
> AC_ARG_WITH(ldom, ....
> option block in the configure.in, was that dropped from the patch ?
Yes, I think that should be added to the patch.
>
> I think this should be in a large part based on patform autodetection
> and the --with-ldom/--without-ldom should only be used for code portability
> tests and development.
>
>
>> +++ b/src/ldoms_common.h
>> @@ -0,0 +1,80 @@
>> +/*
>> + * ldoms_common.h: LDoms common definitions
>> + *
>> + * Copyright 2008 Sun Microsystems, Inc.
>> + *
>> + * See COPYING.LIB for the License of this software
> [...]
>
> looks fine to me but ...
>
>> +/* Debug print */
>> +extern void dprt(const char *template, ...);
>
> i would prefer to see the debug integrated with the existing debug support.
> Since we now compile with debug support enabled by default it is really useful
> to allow user provide debug informations e.g. by running the virsh command
> after having set the LIVIRT_DEBUG=1 environment variable.
> If you have more complex debugging need (levels of debug) the existing routine
> should be able to cope with this (see virErrorLevel in virterror.h).
> All those if (ldoms_debug) dprt(...) should be relatively easy to unify
> into DEBUG() macros like in other parts of the library.
OK. I will use DEBUG macros instead of dprt().
We have implemented two levels of debug (LDOMS_DEBUG and
LDOMS_DETAILED_DEBUG) and generate lots of debug messages if selected
as a DEBUG build. There are so many places in the code for debug check,
so we decided to code "if (ldoms_debug)..." instead of surrounding them
by #ifdef DEBUG, but I think DEBUG() macros can be just used instead of
dprt() without #ifdef DEBUG.
>
>> +++ b/src/ldoms_internal.h
>> @@ -0,0 +1,29 @@
> [...]
>> +int ldomsRegister(void);
>
>
> #include "ldoms_internal.h" should be added in libvirt.c and then
> the declaration of ldomsRegister should be removed there
OK.
>
>> +void ldomsError(virConnectPtr, virDomainPtr, virErrorNumber, const char*, int);
>
>
> [...]
>> diff --git a/src/ldoms_internal.c b/src/ldoms_internal.c
> [...]
>> + * Open a connection to the LDoms Manager and handshake to
>> + * verify the a connection can be made for future XML requests.
>
> interesting to see you are basically using XML as the intercahnge format
> with the manager.
>
> [...]
>> +/*
>> + * ldomsListDomains
>> + *
>> + * What: Return the array of Domain ID's for all the active Domains.
>> + * Send an XML request to LDM (LDoms Manager) for a list of all Domains.
>> + *
>> + * Yes, this functions does pretty much the same as ldomsNumOfDomains() with
>> + * the addition of returning the ID numbers for the valid Domains.
>> + *
>> + * Input: conn - The Connection structure
>> + * maxids - The total number of Domains to look at to
>> + * determine what Domain IDs to return.
>> + * Output: ids - An array of integers to hold the IDs of the
>> + * Domains whose state is other than 'inactive' -
>> + * VIR_DOMAIN_SHUTOFF
>
> I'm a bit confused about how active/inactive domains are handled in lDOM,
> and i don't really have a way to just get my hand on it.
LDoms have the following states:
1 = active LDOM_STATE_ACTIVE
2 = stopping LDOM_STATE_STOPPING
3 = inactive LDOM_STATE_INACTIVE
4 = binding LDOM_STATE_BINDING
5 = unbinding LDOM_STATE_UNBINDING
6 = bound LDOM_STATE_BOUND
7 = starting LDOM_STATE_STARTING
and the LDoms states are converted to the livirt states as follows:
(ldomsDomainGetInfo() in ldoms_internal.c)
LDOM_STATE_ACTIVE ->VIR_DOMAIN_RUNNING
LDOM_STATE_STOPPING ->VIR_DOMAIN_SHUTDOWN
LDOM_STATE_INACTIVE ->VIR_DOMAIN_SHUTOFF
case LDOM_STATE_BINDING->VIR_DOMAIN_SHUTDOWN
case LDOM_STATE_UNBINDING->VIR_DOMAIN_SHUTOFF
case LDOM_STATE_BOUND ->VIR_DOMAIN_SHUTDOWN
case LDOM_STATE_STARTING ->VIR_DOMAIN_RUNNING
>
> [...]
>> +/*
>> + * ldomsDomainCreateXML
>> + *
>> + * What: Create a domain from an XML file so that it is left in the
>> + * inactive state. Just call the DefineXML function. The XML input
>> + * has to be a complete and valid XML requeset for the LDM. No
>> + * modification is done to this file.
>> + *
>> + * Input: xml - The xml file
>> + * Output: function return - A ptr to a virDomain or NULL
>> + */
>> +virDomainPtr
>> +ldomsDomainCreateXML(virConnectPtr conn, const char *xml, unsigned int flags)
>> +{
>> + virDomainPtr domPtr = NULL;
>> + char *domainName = NULL;
>> +
>> + if (ldoms_debug) dprt("LDOMS_DEBUG: ldomsDomainCreateXML(ENTER) \n");
>> + if (ldoms_detailed_debug) dprt("LDOMS_DETAILED_DEBUG: ldomsDomainCreateXML(ENTER) xmldoc=\n%s\n",xml);
>> +
>> + /* Send the XML file along with the lifecycle action */
>> + domainName = send_ldom_create_domain((char *)xml, XML_ADD_DOMAIN);
>> +
>> + /*
>> + * If the create/bind domain was successful, then we have to create a DomainInfo
>> + * structure to pass back to the caller. We need the Domain name that was
>> + * created, and the only way to get that is from parsing the input xml
>> + * document. This is done in send_ldom_create_domain() and passed back as the return.
>> + * Also we create the uuid for the domain name.
>> + */
>
> I am completely unable to find informations about the XML format for a domain
> i looked at send_ldom_create_domain and it seems to send the XML directly to
> the domain manager, ther don't seems to be any parsing or analysis or checking
> of the XML input data at the libvirt level. To me this is a bit problematic
> as the XML is part of the API in practice. This really need to be documentd
> as the support is added to libvirt.
Yes, send_ldom_create_domain() sends the XML directly to the LDoms
Manager after adding some tags such as <cmd> tag that needs to be
present in the LDoms XML format.
>
> [...]
>> + /* get the current ldom data from LDM */
>> + (void) pthread_rwlock_wrlock(&update_lock);
>> + refresh_ldom_data();
>> + (void) pthread_rwlock_unlock(&update_lock);
>> +
>
> I'm a bit surprized by this, this looks a bit heavy to keep a complete
> list in libvirt memory updated with a global lock, that doesn't look optimal
> for example when vish asks for the list of domain you need first to update to
> get the number of domains and then reupdate in the next call(s) to extract the
> domain data. And that won't protect you from races anyway, so i'm a bit
> surprized.
I will look into this more..
>
>> +/*
>> + * refresh_ldom_data
>> + *
>> + * Send an XML request to LDM for a list of all Domains
>> + * and get the basic information of each Domain.
>> + *
>> + * This function will be called for each commands to
>> + * guarantee that we have the current LDom info for any
>> + * virsh CLI command.
>> + *
>> + * Input:
>> + * Output:
>> + */
> [...]
>> +/*
>> + * ldomsDomainDumpXML
>> + *
>> + * What: Send a list-constraints request to LDM for the domain specified
>> + * by the input virDomain.
>> + *
>> + * Input: domain - Pointer to a virDomain structure
>> + * Output: function return - char ptr to the XML data
>> + */
>> +char *
>> +ldomsDomainDumpXML(virDomainPtr domain, int flags)
> [...]
>> + /* Dump the response to memory */
>> + xml = malloc(sizeof(char) * 10000);
>> + xmlKeepBlanksDefault(0);
>
> hum, i usually suggest libxml2 users don't use xmlKeepBlanksDefault anymore
> as this uses a global variable and modify the behaviour of all libxml2
> processing including parsing, this is especially nasty in a library.
>
>> + xmlDocDumpFormatMemory(xml_received, &xml, &xmlSize, 1);
>
> the 1 for the format argument should be sufficient
> Also using xmlSaveToBuffer
> http://xmlsoft.org/html/libxml-xmlsave.html#xmlSaveToBuffer
> would avoid a fixed output buffer limitation which is really not necessary.
>
> I'm still a bit puzzled that the manager uses the libvirt XML format directly
> as the input.
>
> [...]
>> +static void
>> +refresh_ldom_data()
>> +{
>> + struct timeval tv;
>> + long delta;
>
> Question: why not do the locking here instead of around all the calling
> functions ? Simpler code, less error prone, no ?
Yes, I think I can put the rwlock here instead of calling functions.
>
>> + /* Try and throttle calling the LDM to update the list of
>> + * LDoms. If the calls are 2 secs or less apart, then we just
>> + * use the current LDoms info. This is because each
>> + * command from virsh can turn into several calls into
>> + * function in this file, which all call this function
>> + * to refresh the LDom data.
>> + */
>> + if (gettimeofday(&tv, NULL) >= 0) {
>> + /* See if the time since the last call to this functions
>> + * is less than 3 seconds. If so, just return.
>> + */
>> + if ((delta=tv.tv_sec - last_ldom_refresh) < 3) {
>> + if (ldoms_detailed_debug) dprt("LDOMS_DETAILED_DEBUG: refresh_ldom_data(NO Refresh) delta=%d\n",
>> + delta);
>> + return;
>> + }
>> + last_ldom_refresh = tv.tv_sec;
>> + }
>
> [...]
>> +int ldomsRegister(void) {
>> +
>> + /* for debug statements */
>> +#ifdef LDOMS_DEBUG
>> + ldoms_debug = 1;
>> + dprt("LDOMS_DEBUG on\n");
>> +#endif
>> +#ifdef LDOMS_DETAILED_DEBUG
>> + ldoms_detailed_debug = 1;
>> + dprt("LDOMS_DETAILED_DEBUG on\n");
>> +#endif
>
> maybe the debug vs detailed_debug could be integarted in libvirt normal
> debugging model, for example based on the value of LIBVIRT_DEBUG env variable.
OK. I will use the libvirt normal debugging model and use DEBUG instead
of dprt().
>
> [...]
>> diff --git a/src/ldoms_xml_parse.h b/src/ldoms_xml_parse.h
>> new file mode 100644
>> --- /dev/null
>> +++ b/src/ldoms_xml_parse.h
> [...]
>> diff --git a/src/ldoms_xml_parse.c b/src/ldoms_xml_parse.c
>> new file mode 100644
>
> a few comments on libxml2 use
>
> [...]
>> +xml_find_subnode(xmlNodePtr node, const xmlChar *name)
>> +{
>> + xmlNodePtr subnode;
>> +
>> + subnode = node->xmlChildrenNode;
>> + while (subnode != NULL) {
>> + if (xmlStrcmp(subnode->name, name) == 0)
>> + break;
>
> not that with recent libxml2 (any 2.6.x will do) all the markup name strings
> are interned in a dictionary associated to the document. With a bit of tweaking
> on your parsing routines would could also get all domuments parsed to share
> the same dictionary 9basically by always reusing the same XML parsing context,
> meaning all those strings could be interned for the whole process and let you
> fallback to pure string pointer comparisons, instead of comparing content.
> But it's probably a bit complex unless you're chasing performances issues.
Thanks for the info/comments on libxml2 use. We will improve the xml
code if we have to deal with performance issues.
>
> [...]
>> + * Example: The following XML file will be created for LDom ldom1
>> + * and a new memory value of 256 Kilobytes.
>> + *
>> + * <?xml version="1.0"?>
>> + * <LDM_interface version="1.0">
>> + * <cmd>
>> + * <action>set-memory</action>
>> + * <data version="2.0">
>> + * <ldom>
>> + * <ldom_info>
>> + * <ldom_name>ldom1</ldom_name>
>> + * </ldom_info>
>> + * <memory>
>> + * <size>256K</size>
>> + * </memory>
>> + * </ldom>
>> + * </data>
>> + * </cmd>
>> + * </LDM_interface>
>> + */
>> +
>> +xmlDocPtr
>> +create_xml_file_4_set_memory(char *ldom_name, unsigned long memory)
>> +{
>> + xmlDocPtr xml_output;
>> + xmlNodePtr root, cmd, data, ldom, info_node, mem;
>> + char mem_str[10]; /* ascii version of input int memory */
>> +
>> + if (ldoms_debug) dprt("LDOMS_DEBUG: create_xml_file_4_set_memory(ENTER): ldom=%s, memory=%lu\n",
>> + (ldom_name==NULL? "NULL" : ldom_name), memory);
>> +
>> + xml_output = xmlNewDoc(XML_VERSION);
>> +
>> + /* <LDM_interface> tag with version sttribute */
>> + root = xmlNewDocNode(xml_output, NULL, XML_LDM_INTERFACE, NULL);
>> + xmlDocSetRootElement(xml_output, root);
>> + xmlNewProp(root, VERSION_ATTR, LDOM_INTERFACE_VERSION);
>
> In practice building the tree may be a bit more complex than generating the
> XmL serialization directly makes for simpler code and you don't have to
> serialize later,
>
> [...]
>> +parse_xml_get_ldominfo(xmlDoc *doc, int *num_cpu, int *mem_size, int *mem_unit, int *num_crypto, int *num_iobus)
>> +{
>> + data_node = xml_find_subnode(cmd_node, XML_DATA);
>> + if (data_node != NULL)
>> + ldom_node = xml_find_subnode(data_node, LDOM_NODE);
>> + else {
>> + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. XML file does not have <data> tag\n");
>> + return (ret);
>> + }
>> +
>> + if (ldom_node == NULL) {
>> + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. XML file does not have <ldom> tag\n");
>> + return (ret);
>> + }
>> +
>> + /* get number of cpu */
>> + cpu_node = xml_find_subnode(ldom_node, CPU_NODE);
>> + if (cpu_node == NULL) {
>> + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. XML file does not have <cpu> tag\n");
>> + num_cpu_xml = 0;
>> + } else {
>> + subnode = xml_find_subnode(cpu_node, NUMBER_NODE);
>> + if (subnode == NULL) {
>> + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. XML file does not have <number> tag within <cpu> tag\n");
>> + num_cpu_xml = 0;
>> + } else {
>> +
>> + content = xmlNodeGetContent(subnode);
>> +
>> + num_cpu_xml = strtoll((char *)content, NULL, 0);
>> + if (num_cpu_xml <= 0) {
>> + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. Invalid cpu num specified: %s\n", content);
>> + }
>> + xmlFree(content);
>> + }
>> + }
>
> Direct XPath queries on the tree make for simpler code, and
> are probably as efficient (if needed the XPath expressions can be precompiled)
> virXPathLong("number(//ldom/memory/size[1], ctxt, &size)
> is more readable I'm sure, should simplify long term maintainance too,
> it's easy to forget an xmlFree in some path ...
>
> I'm not gonna complain, but it seems you had to deal with libxml2 way more
> than necessary, that's make for an awful lot of code in the end too.
>
> Daniel
>
Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ldom_example1.xml
Type: text/xml
Size: 1195 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20080409/26745bd1/attachment-0002.xml>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ldom_example2.xml
Type: text/xml
Size: 1047 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20080409/26745bd1/attachment-0003.xml>
More information about the libvir-list
mailing list