Merge pull request #6920 from opensourcerouting/nb-errors-apply-phase

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

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;
 }
 
 //

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));
+				     errmsg_len);
-	nb_transaction_apply_finish(transaction, errmsg, sizeof(errmsg));
+	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;
 }

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

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,

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);
 	}
 

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)

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);
 	}