watchfrr, vtysh: do not write config during crash

If a daemon is restarting, crashed, or otherwise in the process of
reconnecting to watchfrr and a user issues "write memory" or "write
file" the resulting config will not include the configuration of that
daemon. This is problematic because this output will overwrite the
previous config, potentially causing unintentional loss of configuration
stored only in the config file based upon timing.

This patch remedies that by making watchfrr check that all daemons are
up before attempting a configuration write, and updating vtysh so that
its failsafe respects this condition as well.

Note that this issue only manifests when using integrated config.

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>

diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c
index efef106d978ac0e87f9bcc4ad185b8b001256e80..1308e1218be9b9957c4f8b2ba1f6a9364a32beae 100644 (file)
--- a/vtysh/vtysh.c
+++ b/vtysh/vtysh.c
@@ -2477,7 +2477,12 @@ DEFUN (vtysh_write_memory,
                                                   "do write integrated",
                                                   outputfile);
 
-               if (ret != CMD_SUCCESS) {
+               /*
+                * If watchfrr returns CMD_WARNING_CONFIG_FAILED this means
+                * that it could not write the config, but additionally
+                * indicates that we should not try either
+                */
+               if (ret != CMD_SUCCESS && ret != CMD_WARNING_CONFIG_FAILED) {
                        printf("\nWarning: attempting direct configuration write without "
                               "watchfrr.\nFile permissions and ownership may be "
                               "incorrect, or write may fail.\n\n");

diff --git a/watchfrr/watchfrr.c b/watchfrr/watchfrr.c
index dc3dcbf1e94096b58bf7433ff2279684bdde0b8b..264882e21f456ed9a76bea4edf022e563a48f1c9 100644 (file)
--- a/watchfrr/watchfrr.c
+++ b/watchfrr/watchfrr.c
@@ -899,6 +899,16 @@ static int wakeup_send_echo(struct thread *t_wakeup)
        return 0;
 }
 
+bool check_all_up(void)
+{
+       struct daemon *dmn;
+
+       for (dmn = gs.daemons; dmn; dmn = dmn->next)
+               if (dmn->state != DAEMON_UP)
+                       return false;
+       return true;
+}
+
 static void sigint(void)
 {
        zlog_notice("Terminating on signal");

diff --git a/watchfrr/watchfrr.h b/watchfrr/watchfrr.h
index 53b92bd8334219c4b78c9107e0dca66a71171eac..1a1c19056fa2ae35ba08bd18238b1a9ac90aba92 100644 (file)
--- a/watchfrr/watchfrr.h
+++ b/watchfrr/watchfrr.h
@@ -25,5 +25,12 @@ extern void watchfrr_vty_init(void);
 
 extern pid_t integrated_write_pid;
 extern void integrated_write_sigchld(int status);
+/*
+ * Check if all daemons we are monitoring are in the DAEMON_UP state.
+ *
+ * Returns:
+ *    True if they are all DAEMON_UP, false otherwise.
+ */
+extern bool check_all_up(void);
 
 #endif /* FRR_WATCHFRR_H */

diff --git a/watchfrr/watchfrr_vty.c b/watchfrr/watchfrr_vty.c
index 1f872c91ff08f796905d1d84ca52beb12d8bc810..1bfc41f255b90f08f73241e280d2ce3b70a72ddf 100644 (file)
--- a/watchfrr/watchfrr_vty.c
+++ b/watchfrr/watchfrr_vty.c
@@ -40,11 +40,24 @@ DEFUN(config_write_integrated,
        pid_t child;
        sigset_t oldmask, sigmask;
 
+       const char *e_inprog = "Configuration write already in progress.";
+       const char *e_dmn = "Not all daemons are up, cannot write config.";
+
        if (integrated_write_pid != -1) {
-               vty_out(vty, "%% configuration write already in progress.\n");
+               vty_out(vty, "%% %s\n", e_inprog);
                return CMD_WARNING;
        }
 
+       /* check that all daemons are up before clobbering config */
+       if (!check_all_up()) {
+               vty_out(vty, "%% %s\n", e_dmn);
+               /*
+                * vtysh interprets this return value to mean that it should
+                * not try to write the config itself
+                */
+               return CMD_WARNING_CONFIG_FAILED;
+       }
+
        fflush(stdout);
        fflush(stderr);