[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] [PATCH][RFC] libvirt ldoms support



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!
<?xml version="1.0"?>
<LDM_interface version="1.0">
  <data version="2.0">
    <ldom>
      <ldom_info>
        <ldom_name>primary</ldom_name>
        <mac_address>00:14:4f:02:7d:c6</mac_address>
      </ldom_info>
      <cpu>
        <number>10</number>
      </cpu>
      <mau>
        <number>3</number>
      </mau>
      <memory>
        <size>1920M</size>
      </memory>
      <physio_device>
        <name>pci 780</name>
      </physio_device>
      <physio_device>
        <name>pci 7c0</name>
      </physio_device>
      <vsw>
        <service_name>primary-vsw0</service_name>
        <dev_path>e1000g0</dev_path>
      </vsw>
      <vcc>
        <service_name>primary-vcc0</service_name>
        <min_port>5000</min_port>
        <max_port>5033</max_port>
      </vcc>
      <var>
        <name>boot-device</name>
        <value>/pci 7c0/pci 0/pci 1/pci 0,2/LSILogic,sas 2/disk 0,0:a disk net</value>
      </var>
      <var>
        <name>nvramrc</name>
        <value>devalias disk /pci 7c0/pci 0/pci 1/pci 0,2/LSILogic,sas 2/disk 0
</value>
      </var>
      <var>
        <name>use-nvramrc?</name>
        <value>true</value>
      </var>
    </ldom>
  </data>
</LDM_interface>
<?xml version="1.0"?>
<LDM_interface version="1.0">
  <cmd>
    <action>list-bindings</action>
    <data version="2.0">
      <ldom>
        <ldom_info>
          <ldom_name>ldom1</ldom_name>
          <mac_address>00:14:4f:fa:fb:63</mac_address>
        </ldom_info>
        <cpu>
          <number>1</number>
          <binding>
            <vid>0</vid>
            <pid>12</pid>
            <strand_percent>100</strand_percent>
          </binding>
        </cpu>
        <memory>
          <size>128M</size>
          <binding>
            <real_addr>0x8000000</real_addr>
            <phys_addr>0x88000000</phys_addr>
            <size>128M</size>
          </binding>
        </memory>
        <console>
          <service_name>primary-vcc0</service_name>
          <port>5001</port>
        </console>
      </ldom>
      <response>
        <status>success</status>
      </response>
    </data>
    <response>
      <status>success</status>
    </response>
  </cmd>
  <response>
    <status>success</status>
  </response>
</LDM_interface>

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]