lib: don't ignore error messages generated during the commit apply phase

While a configuration transaction can't be rejected once it reaches
the APPLY phase, we should allow NB callbacks to generate error
or warning messages when a configuration change is being applied.
That should be useful, for example, to return warnings back to
the user informing that the applied configuration has some kind of
inconsistency or is missing something in order to be effectively
activated. The infrastructure for this was already present, but the
northbound layer was ignoring all errors/warnings generated during
the apply/abort phases instead of returning them to the user. This
commit changes that.

In the gRPC plugin, extend the Commit() RPC adding a new
"error_message" field to the response type. This is necessary to
allow errors/warnings to be returned even when the commit operation
succeeds (since grpc::Status::OK doesn't support error messages
like the other status codes).

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>

diff --git a/grpc/frr-northbound.proto b/grpc/frr-northbound.proto
index d18554df2769228c9983507ed334d8d8472b0727..32544ba694e4303b8b74e9137f4eaf8561965669 100644 (file)
--- a/grpc/frr-northbound.proto
+++ b/grpc/frr-northbound.proto
@@ -287,6 +287,9 @@ message CommitResponse {
   // ID of the created configuration transaction (when the phase is APPLY
   // or ALL).
   uint32 transaction_id = 1;
+
+  // Human-readable error or warning message(s).
+  string error_message = 2;
 }
 
 //

diff --git a/lib/northbound.c b/lib/northbound.c
index 11007e4309115db626175c0768d1b59c46ea8ce3..7a1a35593d738e2598f2a68332adec544713ca85 100644 (file)
--- a/lib/northbound.c
+++ b/lib/northbound.c
@@ -707,23 +707,21 @@ int nb_candidate_commit_prepare(struct nb_context *context,
                                      errmsg_len);
 }
 
-void nb_candidate_commit_abort(struct nb_transaction *transaction)
+void nb_candidate_commit_abort(struct nb_transaction *transaction, char *errmsg,
+                              size_t errmsg_len)
 {
-       char errmsg[BUFSIZ] = {0};
-
        (void)nb_transaction_process(NB_EV_ABORT, transaction, errmsg,
-                                    sizeof(errmsg));
+                                    errmsg_len);
        nb_transaction_free(transaction);
 }
 
 void nb_candidate_commit_apply(struct nb_transaction *transaction,
-                              bool save_transaction, uint32_t *transaction_id)
+                              bool save_transaction, uint32_t *transaction_id,
+                              char *errmsg, size_t errmsg_len)
 {
-       char errmsg[BUFSIZ] = {0};
-
        (void)nb_transaction_process(NB_EV_APPLY, transaction, errmsg,
-                                    sizeof(errmsg));
-       nb_transaction_apply_finish(transaction, errmsg, sizeof(errmsg));
+                                    errmsg_len);
+       nb_transaction_apply_finish(transaction, errmsg, errmsg_len);
 
        /* Replace running by candidate. */
        transaction->config->version++;
@@ -754,9 +752,9 @@ int nb_candidate_commit(struct nb_context *context, struct nb_config *candidate,
         */
        if (ret == NB_OK)
                nb_candidate_commit_apply(transaction, save_transaction,
-                                         transaction_id);
+                                         transaction_id, errmsg, errmsg_len);
        else if (transaction != NULL)
-               nb_candidate_commit_abort(transaction);
+               nb_candidate_commit_abort(transaction, errmsg, errmsg_len);
 
        return ret;
 }

diff --git a/lib/northbound.h b/lib/northbound.h
index d5028ea7d2fe2d4c467d126fd7aeee872540ed16..fa5ac5616c294cddd67598c0985d4c41ba7274c2 100644 (file)
--- a/lib/northbound.h
+++ b/lib/northbound.h
@@ -910,8 +910,15 @@ extern int nb_candidate_commit_prepare(struct nb_context *context,
  *
  * transaction
  *    Candidate configuration to abort. It's consumed by this function.
+ *
+ * errmsg
+ *    Buffer to store human-readable error message in case of error.
+ *
+ * errmsg_len
+ *    Size of errmsg.
  */
-extern void nb_candidate_commit_abort(struct nb_transaction *transaction);
+extern void nb_candidate_commit_abort(struct nb_transaction *transaction,
+                                     char *errmsg, size_t errmsg_len);
 
 /*
  * Commit a previously created configuration transaction.
@@ -925,10 +932,17 @@ extern void nb_candidate_commit_abort(struct nb_transaction *transaction);
  *
  * transaction_id
  *    Optional output parameter providing the ID of the committed transaction.
+ *
+ * errmsg
+ *    Buffer to store human-readable error message in case of error.
+ *
+ * errmsg_len
+ *    Size of errmsg.
  */
 extern void nb_candidate_commit_apply(struct nb_transaction *transaction,
                                      bool save_transaction,
-                                     uint32_t *transaction_id);
+                                     uint32_t *transaction_id, char *errmsg,
+                                     size_t errmsg_len);
 
 /*
  * Create a new transaction to commit a candidate configuration. This is a

diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c
index 534b5128ee52b100ac93ccf0ebb1491d007b3e53..2f6aef539866192778257b49328316efe272d7e5 100644 (file)
--- a/lib/northbound_cli.c
+++ b/lib/northbound_cli.c
@@ -74,7 +74,9 @@ static int nb_cli_classic_commit(struct vty *vty)
                /* Regenerate candidate for consistency. */
                nb_config_replace(vty->candidate_config, running_config, true);
                return CMD_WARNING_CONFIG_FAILED;
-       }
+       } else if (strlen(errmsg) > 0)
+               /* Successful commit. Print warnings (if any). */
+               vty_out(vty, "%s\n", errmsg);
 
        return CMD_SUCCESS;
 }
@@ -318,11 +320,14 @@ int nb_cli_confirmed_commit_rollback(struct vty *vty)
                &context, vty->confirmed_commit_rollback, true,
                "Rollback to previous configuration - confirmed commit has timed out",
                &transaction_id, errmsg, sizeof(errmsg));
-       if (ret == NB_OK)
+       if (ret == NB_OK) {
                vty_out(vty,
                        "Rollback performed successfully (Transaction ID #%u).\n",
                        transaction_id);
-       else {
+               /* Print warnings (if any). */
+               if (strlen(errmsg) > 0)
+                       vty_out(vty, "%s\n", errmsg);
+       } else {
                vty_out(vty,
                        "Failed to rollback to previous configuration.\n\n");
                vty_show_nb_errors(vty, ret, errmsg);
@@ -407,6 +412,9 @@ static int nb_cli_commit(struct vty *vty, bool force,
                vty_out(vty,
                        "%% Configuration committed successfully (Transaction ID #%u).\n\n",
                        transaction_id);
+               /* Print warnings (if any). */
+               if (strlen(errmsg) > 0)
+                       vty_out(vty, "%s\n", errmsg);
                return CMD_SUCCESS;
        case NB_ERR_NO_CHANGES:
                vty_out(vty, "%% No configuration changes to commit.\n\n");
@@ -1666,6 +1674,9 @@ static int nb_cli_rollback_configuration(struct vty *vty,
        case NB_OK:
                vty_out(vty,
                        "%% Configuration was successfully rolled back.\n\n");
+               /* Print warnings (if any). */
+               if (strlen(errmsg) > 0)
+                       vty_out(vty, "%s\n", errmsg);
                return CMD_SUCCESS;
        case NB_ERR_NO_CHANGES:
                vty_out(vty,

diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c
index a3aaf02f08d84d18bc35ef8c69e8824da755520e..1f480f3d02e4f94de6d7b93f13b08be64ed02225 100644 (file)
--- a/lib/northbound_confd.c
+++ b/lib/northbound_confd.c
@@ -375,8 +375,10 @@ static int frr_confd_cdb_read_cb_commit(int fd, int *subp, int reslen)
        /* Apply the transaction. */
        if (transaction) {
                struct nb_config *candidate = transaction->config;
+               char errmsg[BUFSIZ] = {0};
 
-               nb_candidate_commit_apply(transaction, true, NULL);
+               nb_candidate_commit_apply(transaction, true, NULL, errmsg,
+                                         sizeof(errmsg));
                nb_config_free(candidate);
        }
 
@@ -400,8 +402,9 @@ static int frr_confd_cdb_read_cb_abort(int fd, int *subp, int reslen)
        /* Abort the transaction. */
        if (transaction) {
                struct nb_config *candidate = transaction->config;
+               char errmsg[BUFSIZ] = {0};
 
-               nb_candidate_commit_abort(transaction);
+               nb_candidate_commit_abort(transaction, errmsg, sizeof(errmsg));
                nb_config_free(candidate);
        }
 

diff --git a/lib/northbound_grpc.cpp b/lib/northbound_grpc.cpp
index 83d7e0ce95a65c3dedd408989ebd6eec6c5cfd79..f49a3b23f656b36f240232ff6711dd91d0da2ca7 100644 (file)
--- a/lib/northbound_grpc.cpp
+++ b/lib/northbound_grpc.cpp
@@ -690,12 +690,14 @@ class NorthboundImpl
                                break;
                        case frr::CommitRequest::ABORT:
                                nb_candidate_commit_abort(
-                                       candidate->transaction);
+                                       candidate->transaction, errmsg,
+                                       sizeof(errmsg));
                                break;
                        case frr::CommitRequest::APPLY:
                                nb_candidate_commit_apply(
                                        candidate->transaction, true,
-                                       &transaction_id);
+                                       &transaction_id, errmsg,
+                                       sizeof(errmsg));
                                break;
                        case frr::CommitRequest::ALL:
                                ret = nb_candidate_commit(
@@ -741,6 +743,8 @@ class NorthboundImpl
                                        tag->response.set_transaction_id(
                                                transaction_id);
                        }
+                       if (strlen(errmsg) > 0)
+                               tag->response.set_error_message(errmsg);
 
                        tag->responder.Finish(tag->response, status, tag);
                        tag->state = FINISH;
@@ -1281,10 +1285,13 @@ class NorthboundImpl
 
        void delete_candidate(struct candidate *candidate)
        {
+               char errmsg[BUFSIZ] = {0};
+
                _candidates.erase(candidate->id);
                nb_config_free(candidate->config);
                if (candidate->transaction)
-                       nb_candidate_commit_abort(candidate->transaction);
+                       nb_candidate_commit_abort(candidate->transaction,
+                                                 errmsg, sizeof(errmsg));
        }
 
        struct candidate *get_candidate(uint32_t candidate_id)

diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c
index 500203173cacb5fd6538323d60340afa393d286e..2209b19c1477ba4365b029c3fa9dea61a9248dd5 100644 (file)
--- a/lib/northbound_sysrepo.c
+++ b/lib/northbound_sysrepo.c
@@ -329,8 +329,10 @@ static int frr_sr_config_change_cb_apply(sr_session_ctx_t *session,
        /* Apply the transaction. */
        if (transaction) {
                struct nb_config *candidate = transaction->config;
+               char errmsg[BUFSIZ] = {0};
 
-               nb_candidate_commit_apply(transaction, true, NULL);
+               nb_candidate_commit_apply(transaction, true, NULL, errmsg,
+                                         sizeof(errmsg));
                nb_config_free(candidate);
        }
 
@@ -343,8 +345,9 @@ static int frr_sr_config_change_cb_abort(sr_session_ctx_t *session,
        /* Abort the transaction. */
        if (transaction) {
                struct nb_config *candidate = transaction->config;
+               char errmsg[BUFSIZ] = {0};
 
-               nb_candidate_commit_abort(transaction);
+               nb_candidate_commit_abort(transaction, errmsg, sizeof(errmsg));
                nb_config_free(candidate);
        }