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

Re: [libvirt] [Matahari] [libvirt-qpid PATCH 1/3] Convert to QMFv2 APIs



Hi Daniel,
Thanks for the very helpful (and prompt!) feedback. Responses inline.

cheers,
Zane.

On 13/07/11 08:49, Andrew Beekhof wrote:
On Wed, Jul 13, 2011 at 2:46 AM, Daniel P. Berrange<berrange redhat com>  wrote:
On Tue, Jul 12, 2011 at 06:28:46PM +0200, Zane Bitter wrote:
diff --git a/src/DomainWrap.cpp b/src/DomainWrap.cpp
index 47876de..eab6bbc 100644
--- a/src/DomainWrap.cpp
+++ b/src/DomainWrap.cpp
@@ -2,266 +2,310 @@
  #include "NodeWrap.h"
  #include "DomainWrap.h"
  #include "Error.h"
+#include "Exception.h"

-#include "ArgsDomainMigrate.h"
-#include "ArgsDomainRestore.h"
-#include "ArgsDomainSave.h"
-#include "ArgsDomainGetXMLDesc.h"
+#include<cstdio>

-namespace _qmf = qmf::com::redhat::libvirt;

-DomainWrap::DomainWrap(ManagementAgent *agent, NodeWrap *parent,
-                       virDomainPtr _domain_ptr, virConnectPtr _conn) :
-                       domain_ptr(_domain_ptr), conn(_conn)
+DomainWrap::DomainWrap(
+        NodeWrap *parent,
+        virDomainPtr domain_ptr,
+        virConnectPtr conn):
+    PackageOwner<NodeWrap::PackageDefinition>(parent),
+    ManagedObject(package().data_Domain),
+    _domain_ptr(domain_ptr), _conn(conn)
  {
      char dom_uuid[VIR_UUID_STRING_BUFLEN];

-    domain = NULL;
-
-    if (virDomainGetUUIDString(domain_ptr, dom_uuid)<  0) {
-        REPORT_ERR(conn, "DomainWrap: Unable to get UUID string of domain.");
+    if (virDomainGetUUIDString(_domain_ptr, dom_uuid)<  0) {
+        REPORT_ERR(_conn, "DomainWrap: Unable to get UUID string of domain.");
          throw 1;

I've noticed that most (all?) objects are being passed in
a virConnectPtr instance too. This should be redundant for
two reasons

  - If you have a virDomainPtr, you can call virDomainGetConnect()
   to get back the associated virConnectPtr (likewise for other
   libvirt objects)
  - We should kill the 'virConnectPtr' parameter from the
   REPORT_ERR macro/functions, and use 'virGetLastError'
   exclusively. This function is thread-safe, unlike the
   current virConnGetLastError being used.

Since this problem was pre-existing, I'll leave it upto you
whether you want to incorporate such a change into this patch
or do it as a later followup.

Personally I'd prefer it be in a follow-up patch.

Agree, this is a good candidate for a follow-up.

  void
  DomainWrap::update()
  {
      virDomainInfo info;
      int ret;

-    ret = virDomainGetInfo(domain_ptr,&info);
+    ret = virDomainGetInfo(_domain_ptr,&info);
      if (ret<  0) {
-        REPORT_ERR(conn, "Domain get info failed.");
+        REPORT_ERR(_conn, "Domain get info failed.");
          /* Next domainSync() will take care of this in the node wrapper if the domain is
           * indeed dead. */
          return;
      } else {
+        const char *state = NULL;
          switch (info.state) {
-
              case VIR_DOMAIN_NOSTATE:

Add a 'default:' clause here

-                domain->set_state("nostate");
+                state = "nostate";
                  break;
              case VIR_DOMAIN_RUNNING:
-                domain->set_state("running");
+                state = "running";
                  break;
              case VIR_DOMAIN_BLOCKED:
-                domain->set_state("blocked");
+                state = "blocked";
                  break;
              case VIR_DOMAIN_PAUSED:
-                domain->set_state("paused");
+                state = "paused";
                  break;
              case VIR_DOMAIN_SHUTDOWN:
-                domain->set_state("shutdown");
+                state = "shutdown";
                  break;
              case VIR_DOMAIN_SHUTOFF:
-                domain->set_state("shutoff");
+                state = "shutoff";
                  break;
              case VIR_DOMAIN_CRASHED:
-                domain->set_state("crashed");
+                state = "crashed";
                  break;
          }

-        domain->set_numVcpus(info.nrVirtCpu);
-        domain->set_maximumMemory(info.maxMem);
-        domain->set_memory(info.memory);
-        domain->set_cpuTime(info.cpuTime);
+        if (state) {
+            _data.setProperty("state", state);
+        }

We shouldn't skip out 'NOSTATE' here, juust make this
unconditional

+        _data.setProperty("numVcpus", info.nrVirtCpu);
+        _data.setProperty("maximumMemory", info.maxMem);
+        _data.setProperty("memory", (uint64_t)info.memory);
+        _data.setProperty("cpuTime", (uint64_t)info.cpuTime);
      }

Fixed. Thanks!

There is a similar issue with the state of a Pool (as written it will segfault if a state other than inactive/building/running/degraded is returned). What would be an appropriate default there? Just set it to "unknown"?


+bool
+DomainWrap::migrate(qmf::AgentSession&  session, qmf::AgentEvent&  event)
+{
+    virConnectPtr dest_conn;
+    virDomainPtr rem_dom;
+    qpid::types::Variant::Map args(event.getArguments());
+    int ret;
+
+    // This is actually quite broken. Most setups won't allow unauthorized
+    // connection from one node to another directly like this.
+    dest_conn = virConnectOpen(args["destinationUri"].asString().c_str());

Since the agent was originally written, we now have a new migration API
which avoids the need for a destination virConnectPtr object. Instead
you can pass the 'destinationUri' directly to virDomainMigrateToURI,
and set the PEER2PEER flag.

NB, this still presumes that the source libvirtd can auth with the
target libvirtd directly, but it avoids the need for matahari itself
to authenticate. Typically we'd expect the libvirtd's to be setup
with TLS+x590 or Kerberos so they can talk to each other

+    if (!dest_conn) {
+        std::string err = FORMAT_ERR(dest_conn, "Unable to connect to remote system for migration: virConnectOpen",&ret);
+        raiseException(session, event, err, STATUS_USER + ret);
+        return false;
+    }
+
+    const std::string newDomainName_arg(args["newDomainName"]);
+    const char *new_dom_name = NULL;
+    if (newDomainName_arg.size()>  0) {
+        new_dom_name = newDomainName_arg.c_str();
+    }
+
+    const std::string uri_arg(args["uri"]);
+    const char *uri = NULL;
+    if (uri_arg.size()>  0) {
+        uri = uri_arg.c_str();
+    }
+
+    uint32_t flags(args["flags"]);
+    uint32_t bandwidth(args["bandwidth"]);
+
+    printf ("calling migrate, new_dom_name: %s, uri: %s, flags: %d (live is %d)\n",
+            new_dom_name ? new_dom_name : "NULL",
+            uri ? uri : "NULL",
+            flags,
+            VIR_MIGRATE_LIVE);
+
+    rem_dom = virDomainMigrate(_domain_ptr, dest_conn, flags,
+                               new_dom_name,
+                               uri,
+                               bandwidth);

Ideally you want virDomainMigrateToURI, or even better
virDomainMigrateToURI2 here.

No objection at all to making these sorts of changes, lets just do
them as a separate work item.

Yep.


diff --git a/src/Error.h b/src/Error.h
index 388b41a..5bd7397 100644
--- a/src/Error.h
+++ b/src/Error.h
@@ -1,11 +1,19 @@
+#ifndef ERROR_H
+#define ERROR_H
+
+#include<libvirt/libvirt.h>
+#include<libvirt/virterror.h>
+

  #define REPORT_ERR(conn,msg) reportError(conn, msg, __func__, __LINE__, __FILE__)

  #define FORMAT_ERR(conn,msg,err_ret) formatError(conn, msg, err_ret, __func__, __LINE__, __FILE__)

+
  void
  reportError(virConnectPtr conn, const char *msg, const char *function, int line, const char *file);

  std::string
  formatError(virConnectPtr conn, const char *msg, int *err_ret, const char *function, int line, const char *file);

+#endif

We want to remove virConnectPtr from both these methods&  macros

diff --git a/src/Exception.cpp b/src/Exception.cpp
new file mode 100644
index 0000000..105df27
--- /dev/null
+++ b/src/Exception.cpp
@@ -0,0 +1,38 @@
+#include "Exception.h"
+#include "Error.h"
+#include<qmf/Schema.h>
+#include<qmf/SchemaProperty.h>
+#include<qmf/Data.h>
+
+
+#define ERROR_TEXT "error_text"
+#define ERROR_CODE "error_code"
+
+
+static qmf::Schema errorSchema(qmf::SCHEMA_TYPE_DATA,
+        "com.redhat.libvirt", "error");

This should be 'org.libvirt' really.

Fixed.

  generated_file_list = \
-     qmf/com/redhat/libvirt/Domain.cpp\
-     qmf/com/redhat/libvirt/Node.cpp\
-     qmf/com/redhat/libvirt/Package.cpp\
-     qmf/com/redhat/libvirt/Pool.cpp\
-     qmf/com/redhat/libvirt/Volume.cpp\
-     qmf/com/redhat/libvirt/ArgsDomainGetXMLDesc.h\
-     qmf/com/redhat/libvirt/ArgsDomainMigrate.h\
-     qmf/com/redhat/libvirt/ArgsDomainRestore.h\
-     qmf/com/redhat/libvirt/ArgsDomainSave.h\
-     qmf/com/redhat/libvirt/ArgsNodeDomainDefineXML.h\
-     qmf/com/redhat/libvirt/ArgsNodeStoragePoolCreateXML.h\
-     qmf/com/redhat/libvirt/ArgsNodeStoragePoolDefineXML.h\
-     qmf/com/redhat/libvirt/ArgsPoolCreateVolumeXML.h\
-     qmf/com/redhat/libvirt/ArgsPoolGetXMLDesc.h\
-     qmf/com/redhat/libvirt/ArgsVolumeGetXMLDesc.h\
-     qmf/com/redhat/libvirt/Domain.h\
-     qmf/com/redhat/libvirt/Node.h\
-     qmf/com/redhat/libvirt/Package.h\
-     qmf/com/redhat/libvirt/Pool.h\
-     qmf/com/redhat/libvirt/Volume.h
+     qmf/com/redhat/libvirt/QmfPackage.cpp\
+     qmf/com/redhat/libvirt/QmfPackage.h

Can we change these to  org/libvirt too, or will that be an
incompatible schema change ?

I believe it would be.  Ted would know for sure.

Incidentally, is this fully backwards compatible from the
POV of existing libvirt-qpid client apps, or are we starting
a v2 libvirt schema here ?

I believe he has some tests which both versions pass. So I'm pretty
sure the API is more than backwards compatible, it should be
identical.

That is correct (I will eventually post the test tool here too), and it goes some way to proving that it is, for now, possible to write a client that works with both the old and new versions. What it does not tell us is whether any client written against the previous version will work with the new matahari version.

There are definite differences you can see just by playing around with qmf-tool. For instance, with v1 you don't need to specify the package (the com.redhat.libvirt part) when you do a query, but with v2 you do. Then there's that bizarre thing where v1 appears to convert the class names to lower case, but v2 doesn't (which patch #2 fixes). There may be others. The number of hoops to jump through just to return error codes was... surprising.

From the other discussion in this thread it sounds like now is a good time to break backwards compatibility, change the package name to org.libvirt and drop that second patch (for consistency with other matahari agents).


  static void
@@ -521,7 +577,6 @@ print_usage()
      printf("\t-d | --daemon     run as a daemon.\n");
      printf("\t-h | --help       print this help message.\n");
      printf("\t-b | --broker     specify broker host name..\n");
-    printf("\t-g | --gssapi     force GSSAPI authentication.\n");
      printf("\t-u | --username   username to use for authentication purproses.\n");
      printf("\t-s | --service    service name to use for authentication purproses.\n");
      printf("\t-p | --port       specify broker port.\n");
@@ -536,8 +591,7 @@ int main(int argc, char** argv) {
      int idx = 0;
      bool verbose = false;
      bool daemonize = false;
-    bool gssapi = false;
-    char *host = NULL;
+    const char *host = NULL;
      char *username = NULL;
      char *service = NULL;
      int port = 5672;
@@ -546,14 +600,13 @@ int main(int argc, char** argv) {
          {"help", 0, 0, 'h'},
          {"daemon", 0, 0, 'd'},
          {"broker", 1, 0, 'b'},
-        {"gssapi", 0, 0, 'g'},
          {"username", 1, 0, 'u'},
          {"service", 1, 0, 's'},
          {"port", 1, 0, 'p'},
          {0, 0, 0, 0}
      };

-    while ((arg = getopt_long(argc, argv, "hdb:gu:s:p:", opt,&idx)) != -1) {
+    while ((arg = getopt_long(argc, argv, "hdb:u:s:p:", opt,&idx)) != -1) {
          switch (arg) {
              case 'h':
              case '?':
@@ -582,9 +635,6 @@ int main(int argc, char** argv) {
                      exit(1);
                  }
                  break;
-            case 'g':
-                gssapi = true;
-                break;
              case 'p':
                  if (optarg) {
                      port = atoi(optarg);
@@ -620,44 +670,42 @@ int main(int argc, char** argv) {
      // This prevents us from dying if libvirt disconnects.
      signal(SIGPIPE, SIG_IGN);

-    // Create the management agent
-    ManagementAgent::Singleton singleton;
-    ManagementAgent* agent = singleton.getInstance();
-
-    // Register the schema with the agent
-    _qmf::Package packageInit(agent);
-
-    // Start the agent.  It will attempt to make a connection to the
-    // management broker.  The third argument is the interval for sending
-    // updates to stats/properties to the broker.  The last is set to 'true'
-    // to keep this all single threaded.  Otherwise a second thread would be
-    // used to implement methods.
-
-    qpid::management::ConnectionSettings settings;
-    settings.host = host ? host : "127.0.0.1";
-    settings.port = port;
+    qpid::types::Variant::Map options;

      if (username != NULL) {
-        settings.username = username;
+        options["username"] = username;
      }
      if (service != NULL) {
-        settings.service = service;
+        options["service"] = service;
      }
-    if (gssapi == true) {
-        settings.mechanism = "GSSAPI";
+
+    if (host == NULL) {
+        host = "127.0.0.1";
      }

-    agent->setName("Red Hat", "libvirt-qpid");
-    agent->init(settings, 3, true);
+    std::stringstream url;
+
+    url<<  "amqp:"<<  "tcp"<<  ":"<<  host<<  ":"<<  port;
+
+    qpid::messaging::Connection amqp_connection(url.str(), options);
+    amqp_connection.open();
+
+    qmf::AgentSession session(amqp_connection);
+    session.setVendor("redhat.com");

IMHO, this should really be 'org.libvirt' too

FWIW once this becomes a matahari agent this gets changed to "matahariproject.org" anyway whether we like it or not. (Perhaps matahari ought to make this configurable?)

In fact, the previous vendor name was actually "Red Hat", and I had to change it because having a space in it borked something in v2 (I forget what).

Since backward compatibility with anything relying on the agent vendor is going to be broken anyway, I've gone ahead and changed this patch to make it "libvirt.org".

+    session.setProduct("libvirt-qpid");
+    session.setAttribute("hostname", host);

Regards,
Daniel
--
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
_______________________________________________
Matahari mailing list
Matahari lists fedorahosted org
https://fedorahosted.org/mailman/listinfo/matahari



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