From 8fb208222938c1e0cafb13596d2d40c9c446325b Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Fri, 31 Mar 2023 16:39:11 +0000 Subject: [PATCH 1/3] tests: when verifying a route is missing use expected=False Prior to this the full retry cycle was run with a "passing" negative result each time through Previous runtime ~5 minutes New runtime ~20 seconds. Signed-off-by: Christian Hopps --- tests/topotests/mgmt_tests/test_yang_mgmt.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/topotests/mgmt_tests/test_yang_mgmt.py b/tests/topotests/mgmt_tests/test_yang_mgmt.py index 06c18d7dfa..921b4e622c 100644 --- a/tests/topotests/mgmt_tests/test_yang_mgmt.py +++ b/tests/topotests/mgmt_tests/test_yang_mgmt.py @@ -217,7 +217,9 @@ def test_mgmt_commit_check(request): ] } } - result = verify_rib(tgen, "ipv4", dut, input_dict_4, protocol=protocol) + result = verify_rib( + tgen, "ipv4", dut, input_dict_4, protocol=protocol, expected=False + ) assert ( result is not True ), "Testcase {} : Failed" "Error: Routes is missing in RIB".format(tc_name) @@ -319,7 +321,9 @@ def test_mgmt_commit_abort(request): ] } } - result = verify_rib(tgen, "ipv4", dut, input_dict_4, protocol=protocol) + result = verify_rib( + tgen, "ipv4", dut, input_dict_4, protocol=protocol, expected=False + ) assert ( result is not True ), "Testcase {} : Failed" "Error: Routes is missing in RIB".format(tc_name) @@ -372,7 +376,7 @@ def test_mgmt_delete_config(request): assert ( result is True ), "Testcase {} : Failed" "Error: Routes is missing in RIB".format(tc_name) - + step("Mgmt delete config") raw_config = { "r1": { @@ -387,7 +391,9 @@ def test_mgmt_delete_config(request): assert result is True, "Testcase {} : Failed Error: {}".format(tc_name, result) step("Verify that the route is deleted from RIB") - result = verify_rib(tgen, "ipv4", dut, input_dict_4, protocol=protocol) + result = verify_rib( + tgen, "ipv4", dut, input_dict_4, protocol=protocol, expected=False + ) assert ( result is not True ), "Testcase {} : Failed" "Error: Routes is still present in RIB".format(tc_name) @@ -461,7 +467,9 @@ def test_mgmt_chaos_stop_start_frr(request): dut = "r1" protocol = "static" input_dict_4 = {"r1": {"static_routes": [{"network": "192.1.11.200/32"}]}} - result = verify_rib(tgen, "ipv4", dut, input_dict_4, protocol=protocol) + result = verify_rib( + tgen, "ipv4", dut, input_dict_4, protocol=protocol, expected=False + ) assert ( result is not True ), "Testcase {} : Failed" "Error: Routes still present in RIB".format(tc_name) From 803829e41e129ba2ea1b519a665c8732777e2008 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Tue, 28 Mar 2023 13:08:01 -0400 Subject: [PATCH 2/3] mgmtd: remove startup config feature for now The startup config and how it interacts with explicit commits, commit databases and implicit commits needs to be worked out in design first. For now remove the offending code. Signed-off-by: Christian Hopps --- mgmtd/mgmt_ds.c | 24 ------------------------ mgmtd/mgmt_ds.h | 2 -- 2 files changed, 26 deletions(-) diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index 1724afb182..10b3cecb92 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -87,7 +87,6 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, struct mgmt_ds_ctx *dst) { struct lyd_node *dst_dnode, *src_dnode; - struct ly_out *out; if (!src || !dst) return -1; @@ -117,13 +116,6 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, nb_config_diff_del_changes(&src->root.cfg_root->cfg_chgs); } - if (dst->ds_id == MGMTD_DS_RUNNING) { - if (ly_out_new_filepath(MGMTD_STARTUP_DS_FILE_PATH, &out) - == LY_SUCCESS) - mgmt_ds_dump_in_memory(dst, "", LYD_JSON, out); - ly_out_free(out, NULL, 0); - } - /* TODO: Update the versions if nb_config present */ return 0; @@ -134,7 +126,6 @@ static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src, { int ret; struct lyd_node **dst_dnode, *src_dnode; - struct ly_out *out; if (!src || !dst) return -1; @@ -159,13 +150,6 @@ static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src, nb_config_diff_del_changes(&src->root.cfg_root->cfg_chgs); } - if (dst->ds_id == MGMTD_DS_RUNNING) { - if (ly_out_new_filepath(MGMTD_STARTUP_DS_FILE_PATH, &out) - == LY_SUCCESS) - mgmt_ds_dump_in_memory(dst, "", LYD_JSON, out); - ly_out_free(out, NULL, 0); - } - return 0; } @@ -200,8 +184,6 @@ void mgmt_ds_reset_candidate(void) int mgmt_ds_init(struct mgmt_master *mm) { - struct lyd_node *root; - if (mgmt_ds_mm || mm->running_ds || mm->candidate_ds || mm->oper_ds) assert(!"MGMTD: Call ds_init only once!"); @@ -209,12 +191,6 @@ int mgmt_ds_init(struct mgmt_master *mm) if (!running_config) assert(!"MGMTD: Call ds_init after frr_init only!"); - if (mgmt_ds_load_cfg_from_file(MGMTD_STARTUP_DS_FILE_PATH, &root) - == 0) { - nb_config_free(running_config); - running_config = nb_config_new(root); - } - running.root.cfg_root = running_config; running.config_ds = true; running.ds_id = MGMTD_DS_RUNNING; diff --git a/mgmtd/mgmt_ds.h b/mgmtd/mgmt_ds.h index 89a2ea9425..8d01f3d5b1 100644 --- a/mgmtd/mgmt_ds.h +++ b/mgmtd/mgmt_ds.h @@ -24,8 +24,6 @@ #define MGMTD_DS_NAME_CANDIDATE "candidate" #define MGMTD_DS_NAME_OPERATIONAL "operational" -#define MGMTD_STARTUP_DS_FILE_PATH DAEMON_DB_DIR "/frr_startup.json" - #define FOREACH_MGMTD_DS_ID(id) \ for ((id) = MGMTD_DS_NONE; (id) < MGMTD_DS_MAX_ID; (id)++) From 8033bf3976d85c84f00a3ab320eca5fc8418d756 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Fri, 31 Mar 2023 16:34:49 +0000 Subject: [PATCH 3/3] mgmtd: lib: read transitioned daemons split config files in mgmtd When daemons transition to mgmtd they should stop reading their split config files, and let mgmtd do that, otherwise things can get out of sync. Signed-off-by: Christian Hopps --- lib/vty.c | 2 +- lib/vty.h | 1 + mgmtd/mgmt.h | 2 ++ mgmtd/mgmt_main.c | 7 +++++++ mgmtd/mgmt_vty.c | 31 +++++++++++++++++++++++++++++++ staticd/static_main.c | 16 ++++++++++++---- 6 files changed, 54 insertions(+), 5 deletions(-) diff --git a/lib/vty.c b/lib/vty.c index d0a6677788..974f5380d3 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -2400,7 +2400,7 @@ static void vty_timeout(struct event *thread) } /* Read up configuration file from file_name. */ -static void vty_read_file(struct nb_config *config, FILE *confp) +void vty_read_file(struct nb_config *config, FILE *confp) { int ret; struct vty *vty; diff --git a/lib/vty.h b/lib/vty.h index 66d3355329..5114238f6a 100644 --- a/lib/vty.h +++ b/lib/vty.h @@ -369,6 +369,7 @@ extern void vty_pass_fd(struct vty *vty, int fd); extern bool vty_read_config(struct nb_config *config, const char *config_file, char *config_default_dir); +extern void vty_read_file(struct nb_config *config, FILE *confp); extern void vty_time_print(struct vty *, int); extern void vty_serv_sock(const char *, unsigned short, const char *); extern void vty_close(struct vty *); diff --git a/mgmtd/mgmt.h b/mgmtd/mgmt.h index 4d186c176b..9579b02230 100644 --- a/mgmtd/mgmt.h +++ b/mgmtd/mgmt.h @@ -62,6 +62,8 @@ struct mgmt_master { }; extern struct mgmt_master *mm; +extern char const *const mgmt_daemons[]; +extern uint mgmt_daemons_count; /* Inline functions */ static inline unsigned long timeval_elapsed(struct timeval a, struct timeval b) diff --git a/mgmtd/mgmt_main.c b/mgmtd/mgmt_main.c index 7d176059f5..08c999260d 100644 --- a/mgmtd/mgmt_main.c +++ b/mgmtd/mgmt_main.c @@ -17,6 +17,13 @@ #include "routing_nb.h" +char const *const mgmt_daemons[] = { +#ifdef HAVE_STATICD + "staticd", +#endif +}; +uint mgmt_daemons_count = array_size(mgmt_daemons); + /* mgmt options, we use GNU getopt library. */ static const struct option longopts[] = { {"skip_runas", no_argument, NULL, 'S'}, diff --git a/mgmtd/mgmt_vty.c b/mgmtd/mgmt_vty.c index 79fa54a791..1ad60ba7cf 100644 --- a/mgmtd/mgmt_vty.c +++ b/mgmtd/mgmt_vty.c @@ -10,6 +10,8 @@ #include "command.h" #include "json.h" +#include "northbound_cli.h" + #include "mgmtd/mgmt.h" #include "mgmtd/mgmt_be_server.h" #include "mgmtd/mgmt_be_adapter.h" @@ -439,6 +441,33 @@ DEFPY(debug_mgmt, return CMD_SUCCESS; } +/* + * Analog of `frr_config_read_in()`, instead of our config file though we loop + * over all daemons that have transitioned to mgmtd, loading their configs + */ +static int mgmt_config_pre_hook(struct event_loop *loop) +{ + FILE *confp; + char *p; + + for (uint i = 0; i < mgmt_daemons_count; i++) { + p = asprintfrr(MTYPE_TMP, "%s/%s.conf", frr_sysconfdir, + mgmt_daemons[i]); + confp = fopen(p, "r"); + if (confp == NULL) { + if (errno != ENOENT) + zlog_err("%s: couldn't read config file %s: %s", + __func__, p, safe_strerror(errno)); + } else { + zlog_info("mgmtd: reading daemon config from %s", p); + vty_read_file(vty_shared_candidate_config, confp); + fclose(confp); + } + XFREE(MTYPE_TMP, p); + } + return 0; +} + void mgmt_vty_init(void) { /* @@ -452,6 +481,8 @@ void mgmt_vty_init(void) static_vty_init(); #endif + hook_register(frr_config_pre, mgmt_config_pre_hook); + install_node(&debug_node); install_element(VIEW_NODE, &show_mgmt_be_adapter_cmd); diff --git a/staticd/static_main.c b/staticd/static_main.c index 09f22318ff..9809d9751a 100644 --- a/staticd/static_main.c +++ b/staticd/static_main.c @@ -161,6 +161,10 @@ static const struct frr_yang_module_info *const staticd_yang_modules[] = { #define STATIC_VTY_PORT 2616 +/* + * NOTE: .flags == FRR_NO_SPLIT_CONFIG to avoid reading split config, mgmtd will + * do this for us now + */ FRR_DAEMON_INFO(staticd, STATIC, .vty_port = STATIC_VTY_PORT, .proghelp = "Implementation of STATIC.", @@ -170,7 +174,8 @@ FRR_DAEMON_INFO(staticd, STATIC, .vty_port = STATIC_VTY_PORT, .privs = &static_privs, .yang_modules = staticd_yang_modules, .n_yang_modules = array_size(staticd_yang_modules), -); + + .flags = FRR_NO_SPLIT_CONFIG); int main(int argc, char **argv, char **envp) { @@ -210,9 +215,12 @@ int main(int argc, char **argv, char **envp) routing_control_plane_protocols_register_vrf_dependency(); - snprintf(backup_config_file, sizeof(backup_config_file), - "%s/zebra.conf", frr_sysconfdir); - staticd_di.backup_config_file = backup_config_file; + /* + * We set FRR_NO_SPLIT_CONFIG flag to avoid reading our config, but we + * still need to write one if vtysh tells us to. Setting the host + * config filename does this. + */ + host_config_set(config_default); frr_config_fork(); frr_run(master);