From 28c145ebd2f06f4b368e55297cd58ad5d2c066be Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Fri, 14 Jul 2023 16:13:54 -0400 Subject: [PATCH 1/3] lib: mgmtd: only clear pending for the in-progress command The lock/unlocks are being done short-circuit so they are never pending; however, the handling of the unlock notification was always resuming the command if pending was set. In all cases pending is set for another command. For example implicit commit locks then when notified its done unlocks which was clearing the set-config pending flag and resuming that command incorrectly. Signed-off-by: Christian Hopps --- lib/mgmt_fe_client.c | 5 +++++ lib/mgmt_fe_client.h | 6 ++++++ lib/vty.c | 6 +++++- mgmtd/mgmt_fe_adapter.c | 5 ++--- vtysh/vtysh.c | 1 + 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/lib/mgmt_fe_client.c b/lib/mgmt_fe_client.c index da19db463f..7e42e1c09e 100644 --- a/lib/mgmt_fe_client.c +++ b/lib/mgmt_fe_client.c @@ -629,6 +629,11 @@ uint mgmt_fe_client_session_count(struct mgmt_fe_client *client) return mgmt_sessions_count(&client->sessions); } +bool mgmt_fe_client_current_msg_short_circuit(struct mgmt_fe_client *client) +{ + return client->client.conn.is_short_circuit; +} + /* * Create a new Session for a Frontend Client connection. */ diff --git a/lib/mgmt_fe_client.h b/lib/mgmt_fe_client.h index 286141da44..349b7e4cf4 100644 --- a/lib/mgmt_fe_client.h +++ b/lib/mgmt_fe_client.h @@ -373,6 +373,12 @@ extern void mgmt_fe_client_destroy(struct mgmt_fe_client *client); */ extern uint mgmt_fe_client_session_count(struct mgmt_fe_client *client); +/* + * True if the current handled message is being short-circuited + */ +extern bool +mgmt_fe_client_current_msg_short_circuit(struct mgmt_fe_client *client); + #ifdef __cplusplus } #endif diff --git a/lib/vty.c b/lib/vty.c index c9de00a271..23aa2d1f38 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -3524,6 +3524,7 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client, char *errmsg_if_any) { struct vty *vty; + bool is_short_circuit = mgmt_fe_client_current_msg_short_circuit(client); vty = (struct vty *)session_ctx; @@ -3540,8 +3541,10 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client, vty->mgmt_locked_running_ds = lock_ds; } - if (vty->mgmt_req_pending_cmd) + if (!is_short_circuit && vty->mgmt_req_pending_cmd) { + assert(!strcmp(vty->mgmt_req_pending_cmd, "MESSAGE_LOCKDS_REQ")); vty_mgmt_resume_response(vty, success); + } } static void vty_mgmt_set_config_result_notified( @@ -3734,6 +3737,7 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit) } else if (vty_mgmt_lock_running_inline(vty)) { vty_out(vty, "%% command failed, could not lock running DS\n"); + vty_mgmt_unlock_candidate_inline(vty); return -1; } } diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index c12d8646b1..98b0e8bfcd 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -593,9 +593,8 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session, } if (lockds_req->lock) { - if (mgmt_fe_session_write_lock_ds(lockds_req->ds_id, - ds_ctx, session) - != 0) { + if (mgmt_fe_session_write_lock_ds(lockds_req->ds_id, ds_ctx, + session)) { fe_adapter_send_lockds_reply( session, lockds_req->ds_id, lockds_req->req_id, lockds_req->lock, false, diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c index ee52a98adb..6744bfe728 100644 --- a/vtysh/vtysh.c +++ b/vtysh/vtysh.c @@ -2356,6 +2356,7 @@ static int vtysh_exit(struct vty *vty) if (vty->node == CONFIG_NODE) { /* resync in case one of the daemons is somewhere else */ vtysh_execute("end"); + /* NOTE: a rather expensive thing to do, can we avoid it? */ vtysh_execute("configure terminal file-lock"); } return CMD_SUCCESS; From 59b69ae1f8b512481521de0669f2ea252c1e9f88 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Fri, 14 Jul 2023 18:21:55 -0400 Subject: [PATCH 2/3] vtysh: track and fix file-lock use in the workaround from 2004 There's a workaround in the code from a bug from back in 2004, it ends and re-enters config mode anytime an `exit` is done from a level below the top-level config node (e.g., from a `router isis` node). We need to re-enter config mode with or without a lock according to how we actually entered it to begin with. fixes #13920 Signed-off-by: Christian Hopps --- lib/vty.h | 4 ++++ vtysh/vtysh.c | 7 ++++++- vtysh/vtysh_config.c | 4 +++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/vty.h b/lib/vty.h index ac3d2e5019..a8654f8b69 100644 --- a/lib/vty.h +++ b/lib/vty.h @@ -231,6 +231,10 @@ struct vty { const char *mgmt_req_pending_cmd; bool mgmt_locked_candidate_ds; bool mgmt_locked_running_ds; + /* Need to track when we file-lock in vtysh to re-lock on end/conf t + * workaround + */ + bool vtysh_file_locked; }; static inline void vty_push_context(struct vty *vty, int node, uint64_t id) diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c index 6744bfe728..8b223d1aa4 100644 --- a/vtysh/vtysh.c +++ b/vtysh/vtysh.c @@ -2357,8 +2357,13 @@ static int vtysh_exit(struct vty *vty) /* resync in case one of the daemons is somewhere else */ vtysh_execute("end"); /* NOTE: a rather expensive thing to do, can we avoid it? */ - vtysh_execute("configure terminal file-lock"); + + if (vty->vtysh_file_locked) + vtysh_execute("configure terminal file-lock"); + else + vtysh_execute("configure terminal"); } + return CMD_SUCCESS; } diff --git a/vtysh/vtysh_config.c b/vtysh/vtysh_config.c index a5f790bbc6..593de7ffbc 100644 --- a/vtysh/vtysh_config.c +++ b/vtysh/vtysh_config.c @@ -607,7 +607,8 @@ static int vtysh_read_file(FILE *confp, bool dry_run) vty->node = CONFIG_NODE; vtysh_execute_no_pager("enable"); - vtysh_execute_no_pager("configure terminal file-lock"); + vtysh_execute_no_pager("conf term file-lock"); + vty->vtysh_file_locked = true; if (!dry_run) vtysh_execute_no_pager("XFRR_start_configuration"); @@ -619,6 +620,7 @@ static int vtysh_read_file(FILE *confp, bool dry_run) vtysh_execute_no_pager("XFRR_end_configuration"); vtysh_execute_no_pager("end"); + vty->vtysh_file_locked = false; vtysh_execute_no_pager("disable"); vty_close(vty); From 318e060cc075d0c8f4823f24e1e1b62376c4d544 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Fri, 14 Jul 2023 19:29:57 -0400 Subject: [PATCH 3/3] tests: add regression test for issue $13920 Signed-off-by: Christian Hopps --- tests/topotests/mgmt_config/r1/frr.conf | 15 ++++++ .../topotests/mgmt_config/test_regression.py | 53 +++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 tests/topotests/mgmt_config/r1/frr.conf create mode 100644 tests/topotests/mgmt_config/test_regression.py diff --git a/tests/topotests/mgmt_config/r1/frr.conf b/tests/topotests/mgmt_config/r1/frr.conf new file mode 100644 index 0000000000..076715c068 --- /dev/null +++ b/tests/topotests/mgmt_config/r1/frr.conf @@ -0,0 +1,15 @@ +debug northbound notifications +! debug northbound libyang +debug northbound events +debug northbound callbacks +debug mgmt backend datastore frontend transaction +debug mgmt client backend +debug mgmt client frontend + +log timestamp precision 6 +log file frr.log debug + +interface r1-eth0 + ip address 101.0.0.1/24 + ipv6 address 2101::1/64 +exit \ No newline at end of file diff --git a/tests/topotests/mgmt_config/test_regression.py b/tests/topotests/mgmt_config/test_regression.py new file mode 100644 index 0000000000..00c3e01724 --- /dev/null +++ b/tests/topotests/mgmt_config/test_regression.py @@ -0,0 +1,53 @@ +# -*- coding: utf-8 eval: (blacken-mode 1) -*- +# SPDX-License-Identifier: ISC +# +# July 13 2023, Christian Hopps +# +# Copyright (c) 2023, LabN Consulting, L.L.C. +# +""" +Test mgmtd regressions + +""" +import pytest +from lib.topogen import Topogen + +pytestmark = [pytest.mark.staticd] + + +@pytest.fixture(scope="module") +def tgen(request): + "Setup/Teardown the environment and provide tgen argument to tests" + + topodef = {"s1": ("r1",)} + tgen = Topogen(topodef, request.module.__name__) + tgen.start_topology() + tgen.gears["r1"].load_frr_config("frr.conf") + tgen.start_router() + yield tgen + tgen.stop_topology() + + +def test_regression_issue_13920(tgen): + """Issue #13920 + + ubuntu2204# conf t + ubuntu2204(config)# ip route 3.2.4.0/24 6.5.5.11 loop3 + ubuntu2204(config)# nexthop-group nh2 + ubuntu2204(config-nh-group)# nexthop 6.5.5.12 + ubuntu2204(config-nh-group)# exi + ubuntu2204(config)# ip route 3.22.4.0/24 6.5.5.12 + crash + """ + + r1 = tgen.gears["r1"] + r1.vtysh_multicmd( + """ + conf t + nexthop-group nh2 + exit + ip route 3.22.4.0/24 6.5.5.12 + """ + ) + output = r1.net.checkRouterCores() + assert not output.strip()