matthieu/frr
1
0
Fork 0
forked from Mirror/frr
Code Pull requests Activity

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

Browse source 
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>
This commit is contained in:
Renato Westphal 2020-08-14 19:49:41 -03:00
parent 2ed8d0249d
commit 0fe5b904b7
7 changed files with 62 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
grpc/frr-northbound.proto
View file

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

20
lib/northbound.c
View file

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

18
lib/northbound.h
View file

@ -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

17
lib/northbound_cli.c
View file

@ -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,

7
lib/northbound_confd.c
View file

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

13
lib/northbound_grpc.cpp
View file

@ -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)

7
lib/northbound_sysrepo.c
View file

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