From 88e635ee6397b93a3263dbe7951f637ae4936e39 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Fri, 11 Sep 2020 22:39:11 -0300 Subject: [PATCH 1/3] lib: postpone the sysrepo plugin initialization From Sysrepo's documentation: "Note: do not use fork() after creating a connection. Sysrepo internally stores PID of every created connection and this way a mismatch of PID and connection is created". Introduce a new "frr_very_late_init" hook in libfrr that is only called after the daemon is forked (when the '-d' option is used) and after the configuration is read. This way we can initialize the sysrepo plugin correctly even when the daemon is daemonized, and after the Sysrepo CLI commands are processed (only "debug northbound client sysrepo" for now). Fixes #7062 Signed-off-by: Renato Westphal --- lib/libfrr.c | 3 +++ lib/libfrr.h | 1 + lib/northbound_sysrepo.c | 9 ++++++++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/libfrr.c b/lib/libfrr.c index 500a02aacd..800596c563 100644 --- a/lib/libfrr.c +++ b/lib/libfrr.c @@ -45,6 +45,7 @@ #include "defaults.h" DEFINE_HOOK(frr_late_init, (struct thread_master * tm), (tm)) +DEFINE_HOOK(frr_very_late_init, (struct thread_master * tm), (tm)) DEFINE_KOOH(frr_early_fini, (), ()) DEFINE_KOOH(frr_fini, (), ()) @@ -913,6 +914,8 @@ static int frr_config_read_in(struct thread *t) __func__, nb_err_name(ret), errmsg); } + hook_call(frr_very_late_init, master); + return 0; } diff --git a/lib/libfrr.h b/lib/libfrr.h index 9d91ea9154..ab72299206 100644 --- a/lib/libfrr.h +++ b/lib/libfrr.h @@ -136,6 +136,7 @@ extern const char *frr_get_progname(void); extern enum frr_cli_mode frr_get_cli_mode(void); DECLARE_HOOK(frr_late_init, (struct thread_master * tm), (tm)) +DECLARE_HOOK(frr_very_late_init, (struct thread_master * tm), (tm)) extern void frr_config_fork(void); extern void frr_run(struct thread_master *master); diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index 3dec685927..145164b90d 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -742,7 +742,7 @@ static int frr_sr_finish(void) return 0; } -static int frr_sr_module_late_init(struct thread_master *tm) +static int frr_sr_module_very_late_init(struct thread_master *tm) { master = tm; @@ -753,6 +753,12 @@ static int frr_sr_module_late_init(struct thread_master *tm) } hook_register(frr_fini, frr_sr_finish); + + return 0; +} + +static int frr_sr_module_late_init(struct thread_master *tm) +{ frr_sr_cli_init(); return 0; @@ -761,6 +767,7 @@ static int frr_sr_module_late_init(struct thread_master *tm) static int frr_sr_module_init(void) { hook_register(frr_late_init, frr_sr_module_late_init); + hook_register(frr_very_late_init, frr_sr_module_very_late_init); return 0; } From 7dac19f7de94ab562d6075314ae3c0eb1604ba2f Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Fri, 11 Sep 2020 22:39:35 -0300 Subject: [PATCH 2/3] lib: fix handling of deleted nodes in the sysrepo plugin Make the sysrepo plugin ignore the deletion of configuration nodes that don't exist anymore instead of logging an error and rejecting the changes. This is necessary because Sysrepo delivers delete notifications for all nodes of a deleted data tree instead of delivering a single delete notification of the top-level subtree node (which would suffice for the northbound layer). Signed-off-by: Renato Westphal --- lib/northbound_sysrepo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index 145164b90d..856af306e6 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -223,7 +223,7 @@ static int frr_sr_process_change(struct nb_config *candidate, ret = nb_candidate_edit(candidate, nb_node, nb_op, xpath, NULL, data); yang_data_free(data); - if (ret != NB_OK) { + if (ret != NB_OK && ret != NB_ERR_NOT_FOUND) { flog_warn( EC_LIB_NB_CANDIDATE_EDIT_ERROR, "%s: failed to edit candidate configuration: operation [%s] xpath [%s]", From bbeaa0333c28c1fbe73f81ff44f1ce27adda3789 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Fri, 11 Sep 2020 22:39:50 -0300 Subject: [PATCH 3/3] lib: simplify handling of the sysrepo startup configuration In the new Sysrepo, all SR_EV_ENABLED notifications are followed by SR_EV_DONE notifications (assuming no errors occur), so there's no need to special case the SR_EV_ENABLED event anymore (e.g. do full transactions in one step). While here, add a few more guarded debug messages to facilitate troubleshooting. Signed-off-by: Renato Westphal --- lib/northbound_sysrepo.c | 57 ++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 35 deletions(-) diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index 856af306e6..b5ef040a3f 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -174,6 +174,9 @@ static int frr_sr_process_change(struct nb_config *candidate, xpath = sr_data->xpath; + DEBUGD(&nb_dbg_client_sysrepo, "sysrepo: processing change [xpath %s]", + xpath); + /* Non-presence container - nothing to do. */ if (sr_data->type == SR_CONTAINER_T) return NB_OK; @@ -235,8 +238,7 @@ static int frr_sr_process_change(struct nb_config *candidate, } static int frr_sr_config_change_cb_prepare(sr_session_ctx_t *session, - const char *module_name, - bool startup_config) + const char *module_name) { sr_change_iter_t *it; int ret; @@ -275,33 +277,17 @@ static int frr_sr_config_change_cb_prepare(sr_session_ctx_t *session, transaction = NULL; context.client = NB_CLIENT_SYSREPO; - if (startup_config) { - /* - * sysrepod sends the entire startup configuration using a - * single event (SR_EV_ENABLED). This means we need to perform - * the full two-phase commit protocol in one go here. - */ - ret = nb_candidate_commit(&context, candidate, true, NULL, NULL, - errmsg, sizeof(errmsg)); - if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) - flog_warn( - EC_LIB_LIBSYSREPO, - "%s: failed to apply startup configuration: %s (%s)", - __func__, nb_err_name(ret), errmsg); - } else { - /* - * Validate the configuration changes and allocate all resources - * required to apply them. - */ - ret = nb_candidate_commit_prepare(&context, candidate, NULL, - &transaction, errmsg, - sizeof(errmsg)); - if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) - flog_warn( - EC_LIB_LIBSYSREPO, - "%s: failed to prepare configuration transaction: %s (%s)", - __func__, nb_err_name(ret), errmsg); - } + /* + * Validate the configuration changes and allocate all resources + * required to apply them. + */ + ret = nb_candidate_commit_prepare(&context, candidate, NULL, + &transaction, errmsg, sizeof(errmsg)); + if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) + flog_warn( + EC_LIB_LIBSYSREPO, + "%s: failed to prepare configuration transaction: %s (%s)", + __func__, nb_err_name(ret), errmsg); if (!transaction) nb_config_free(candidate); @@ -360,11 +346,8 @@ static int frr_sr_config_change_cb(sr_session_ctx_t *session, { switch (sr_ev) { case SR_EV_ENABLED: - return frr_sr_config_change_cb_prepare(session, module_name, - true); case SR_EV_CHANGE: - return frr_sr_config_change_cb_prepare(session, module_name, - false); + return frr_sr_config_change_cb_prepare(session, module_name); case SR_EV_DONE: return frr_sr_config_change_cb_apply(session, module_name); case SR_EV_ABORT: @@ -563,6 +546,10 @@ static void frr_sr_subscribe_config(struct yang_module *module) { int ret; + DEBUGD(&nb_dbg_client_sysrepo, + "sysrepo: subscribing for configuration changes made in the '%s' module", + module->name); + ret = sr_module_change_subscribe( session, module->name, NULL, frr_sr_config_change_cb, NULL, 0, SR_SUBSCR_DEFAULT | SR_SUBSCR_ENABLED | SR_SUBSCR_NO_THREAD, @@ -586,7 +573,7 @@ static int frr_sr_subscribe_state(const struct lys_node *snode, void *arg) nb_node = snode->priv; - DEBUGD(&nb_dbg_client_sysrepo, "%s: providing data to '%s'", __func__, + DEBUGD(&nb_dbg_client_sysrepo, "sysrepo: providing data to '%s'", nb_node->xpath); ret = sr_oper_get_items_subscribe( @@ -610,7 +597,7 @@ static int frr_sr_subscribe_rpc(const struct lys_node *snode, void *arg) nb_node = snode->priv; - DEBUGD(&nb_dbg_client_sysrepo, "%s: providing RPC to '%s'", __func__, + DEBUGD(&nb_dbg_client_sysrepo, "sysrepo: providing RPC to '%s'", nb_node->xpath); ret = sr_rpc_subscribe(session, nb_node->xpath, frr_sr_config_rpc_cb,