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>

7 files changed, 62 insertions, 23 deletions

diff --git a/grpc/frr-northbound.proto b/grpc/frr-northbound.proto
index d18554df27..32544ba694 100644
--- 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 11007e4309..7a1a35593d 100644
--- 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 d5028ea7d2..fa5ac5616c 100644
--- 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 534b5128ee..2f6aef5398 100644
--- 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 a3aaf02f08..1f480f3d02 100644
--- 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 83d7e0ce95..f49a3b23f6 100644
--- 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 500203173c..2209b19c14 100644
--- 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);
 	}