Report OVSDB transaction errors and log

Address another LGTM issue in JsonRpcTransactMessage Signed-off-by: Tom Flynn <tom.flynn@gmail.com>
noironetworks · May 27, 2020 · 393b72d · 393b72d
1 parent a961def
commit 393b72d
Show file tree

Hide file tree

Showing 7 changed files with 64 additions and 10 deletions.
diff --git a/agent-ovs/ovs/OvsdbConnection.cpp b/agent-ovs/ovs/OvsdbConnection.cpp
@@ -64,7 +64,7 @@ void OvsdbConnection::start() {
 
 void OvsdbConnection::connect_cb(uv_async_t* handle) {
     unique_lock<mutex> lock(OvsdbConnection::ovsdbMtx);
-    OvsdbConnection* ocp = (OvsdbConnection*)handle->data;
+    auto* ocp = (OvsdbConnection*)handle->data;
     if (ocp->ovsdbUseLocalTcpPort) {
         ocp->peer = yajr::Peer::create("127.0.0.1",
                                        "6640",
@@ -134,7 +134,7 @@ void OvsdbConnection::disconnect() {
     // TODO
 }
 
-void OvsdbConnection::handleTransaction(uint64_t reqId,  const rapidjson::Document& payload) {
+void OvsdbConnection::handleTransaction(uint64_t reqId, const rapidjson::Document& payload) {
     unique_lock<mutex> lock(transactionMutex);
     auto iter = transactions.find(reqId);
     if (iter != transactions.end()) {
@@ -145,4 +145,21 @@ void OvsdbConnection::handleTransaction(uint64_t reqId,  const rapidjson::Docume
     }
 }
 
+void OvsdbConnection::handleTransactionError(uint64_t reqId, const rapidjson::Document& payload) {
+    unique_lock<mutex> lock(transactionMutex);
+    auto iter = transactions.find(reqId);
+    if (iter != transactions.end()) {
+        iter->second->handleTransaction(reqId, payload);
+        transactions.erase(iter);
+    }
+
+    if (payload.HasMember("error")) {
+        StringBuffer buffer;
+        Writer<StringBuffer> writer(buffer);
+        payload.Accept(writer);
+        LOG(WARNING) << "Received error response for reqId " << reqId << " - " << buffer.GetString();
+    } else {
+        LOG(WARNING) << "Received error response with no error element";
+    }
+}
 }
diff --git a/agent-ovs/ovs/include/JsonRpc.h b/agent-ovs/ovs/include/JsonRpc.h
@@ -87,6 +87,15 @@ class JsonRpc : public Transaction {
      */
     void handleTransaction(uint64_t reqId, const Document& payload);
 
+    /**
+     * callback for handling transaction errors
+     * @param[in] reqId request ID of the request for this response.
+     * @param[in] payload Document reference of the response body.
+     */
+    void handleTransactionError(uint64_t reqId, const rapidjson::Document& payload) {
+        // no-op
+    }
+
     /**
      * update the port list for the bridge
      * @param[in] brUuid bridge UUID

diff --git a/agent-ovs/ovs/include/JsonRpcTransactMessage.h b/agent-ovs/ovs/include/JsonRpcTransactMessage.h
@@ -172,6 +172,11 @@ class JsonRpcTransactMessage : public opflex::jsonrpc::JsonRpcMessage {
          conditions(copy.conditions), columns(copy.columns), rowData(copy.rowData), mutateRowData(copy.mutateRowData), kvPairs(copy.kvPairs),
          operation(copy.getOperation()), table(copy.getTable()) {}
 
+    /**
+     * Assignment operator
+     */
+    JsonRpcTransactMessage& operator=(const JsonRpcTransactMessage& rhs) = default;
+
      /**
       * Destructor
       */

diff --git a/agent-ovs/ovs/include/OvsdbConnection.h b/agent-ovs/ovs/include/OvsdbConnection.h
@@ -41,8 +41,11 @@ class Transaction {
     /**
      * pure virtual method for handling transactions
      */
-    virtual void handleTransaction(uint64_t reqId,
-            const rapidjson::Document& payload) = 0;
+    virtual void handleTransaction(uint64_t reqId, const rapidjson::Document& payload) = 0;
+    /**
+     * pure virtual method for handling transaction errors
+     */
+    virtual void handleTransactionError(uint64_t reqId, const rapidjson::Document& payload) = 0;
 };
 
 /**
@@ -145,8 +148,14 @@ class OvsdbConnection : public opflex::jsonrpc::RpcConnection {
      * @param[in] reqId request ID of the request for this response.
      * @param[in] payload rapidjson::Value reference of the response body.
      */
-    virtual void handleTransaction(uint64_t reqId,
-                const rapidjson::Document& payload);
+    virtual void handleTransaction(uint64_t reqId, const rapidjson::Document& payload);
+
+    /**
+     * call back for transaction error response
+     * @param[in] reqId request ID of the request for this response.
+     * @param[in] payload rapidjson::Value reference of the response body.
+     */
+    virtual void handleTransactionError(uint64_t reqId, const rapidjson::Document& payload);
 
     /**
      * condition variable used for synchronizing JSON/RPC

diff --git a/libopflex/comms/rpc/message/factory/inboundmessage.cpp b/libopflex/comms/rpc/message/factory/inboundmessage.cpp
@@ -93,13 +93,18 @@ MessageFactory::InboundMessage(
         }
 
         if (doc.HasMember(Message::kPayloadKey.result)) {
-            const rapidjson::Value & result = doc[Message::kPayloadKey.result];
-            return MessageFactory::InboundResult(peer, result, id);
+            const rapidjson::Value& result = doc[Message::kPayloadKey.result];
+            // OVSDB returns an error key inside the result instead of using the top-level error
+            if (result.IsArray() && result.Size() > 0 && result[0].IsObject() &&
+                result[0].HasMember("error")) {
+                return MessageFactory::InboundError(peer, result[0], id);
+            } else {
+                return MessageFactory::InboundResult(peer, result, id);
+            }
         }
 
         if (doc.HasMember("error")) {
-            const rapidjson::Value & error = doc["error"];
-
+            const rapidjson::Value& error = doc["error"];
             if (!error.IsObject()) {
                 LOG(ERROR) << &peer << " Received error frame with an error that is not an object.";
                 goto error;

diff --git a/libopflex/engine/OpflexHandler.cpp b/libopflex/engine/OpflexHandler.cpp
@@ -335,6 +335,8 @@ void InbReq<&yajr::rpc::method::transact>::process() const {
 
 template<>
 void InbErr<&yajr::rpc::method::transact>::process() const {
+    ((opflex::jsonrpc::RpcConnection*)getPeer()->getData())
+        ->handleTransactionError(getLocalId().id_, (rapidjson::Document&)getPayload());
 }
 
 

diff --git a/libopflex/include/opflex/rpc/JsonRpcConnection.h b/libopflex/include/opflex/rpc/JsonRpcConnection.h
@@ -40,6 +40,13 @@ class RpcConnection : private boost::noncopyable {
      */
     virtual void handleTransaction(uint64_t reqId, const rapidjson::Document& payload) {};
 
+    /**
+     * call back for transaction error response
+     * @param[in] reqId request ID of the request for this response.
+     * @param[in] payload rapidjson::Value reference of the response body.
+     */
+    virtual void handleTransactionError(uint64_t reqId, const rapidjson::Document& payload) {};
+
     /**
      * destructor
      */