From aaaeec92eb2e4ff3b207aba0c720dd8b1510b240 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Thu, 29 Apr 2021 17:27:39 +0200 Subject: [PATCH] Internal RESTCONF changes: - Kill running process directly, not schedul a kill - fcgi: when kill, make exit more ordered, ensure fcgi-accept returns properly --- apps/restconf/restconf_main_fcgi.c | 30 +++++++++---- apps/restconf/restconf_main_native.c | 5 ++- lib/src/clixon_proc.c | 60 ++++++++++++++++--------- test/test_datastore.sh | 1 + test/test_feature.sh | 2 + test/test_nacm_default.sh | 1 + test/test_nacm_module_read.sh | 1 + test/test_perf_restconf.sh | 1 + test/test_restconf.sh | 3 +- test/test_restconf_internal.sh | 21 +++++---- test/test_restconf_internal_usecases.sh | 26 ++++++----- test/test_restconf_jukebox.sh | 8 ++-- test/test_restconf_op.sh | 1 + test/test_upgrade_failsafe.sh | 3 ++ test/test_xml_trees.sh | 5 ++- test/test_yang_default.sh | 2 + test/test_yang_namespace.sh | 2 + 17 files changed, 116 insertions(+), 56 deletions(-) diff --git a/apps/restconf/restconf_main_fcgi.c b/apps/restconf/restconf_main_fcgi.c index 85efdf17..d1f49e0c 100644 --- a/apps/restconf/restconf_main_fcgi.c +++ b/apps/restconf/restconf_main_fcgi.c @@ -125,9 +125,14 @@ fcgi_params_set(clicon_handle h, return retval; } -/* Need global variable to for signal handler XXX */ +/* XXX Need global variable to for SIGCHLD signal handler +*/ static clicon_handle _CLICON_HANDLE = NULL; +/* XXX Need global variable to break FCGI accept loop from signal handler see FCGX_Accept_r(req) + */ +static int _MYSOCK; + /*! Signall terminates process */ static void @@ -135,18 +140,21 @@ restconf_sig_term(int arg) { static int i=0; + clicon_debug(1, "%s", __FUNCTION__); if (i++ == 0) clicon_log(LOG_NOTICE, "%s: %s: pid: %u Signal %d", __PROGRAM__, __FUNCTION__, getpid(), arg); - else + else{ + clicon_debug(1, "%s done", __FUNCTION__); exit(-1); - if (_CLICON_HANDLE){ - stream_child_freeall(_CLICON_HANDLE); - restconf_terminate(_CLICON_HANDLE); } - clicon_exit_set(); /* checked in clixon_event_loop() */ - clicon_debug(1, "%s done", __FUNCTION__); - exit(-1); + + /* This should ensure no more accepts or incoming packets are processed because next time eventloop + * is entered, it will terminate. + * However there may be a case of sockets closing rather abruptly for clients + */ + clicon_exit_set(); + close(_MYSOCK); } /*! Reap stream child @@ -498,6 +506,7 @@ main(int argc, clicon_err(OE_CFG, errno, "FCGX_OpenSocket"); goto done; } + _MYSOCK = sock; /* Change group of fcgi sock fronting reverse proxy to WWWUSER, the effective group is clicon * which is backend. */ gid_t wgid = -1; @@ -581,6 +590,10 @@ main(int argc, goto done; if (finish) FCGX_Finish_r(req); + else if (clicon_exit_get()){ + FCGX_Finish_r(req); + break; + } else{ /* A handler is forked so we initiate a new request after instead of finishing the old */ if (FCGX_InitRequest(req, sock, 0) != 0){ @@ -588,7 +601,6 @@ main(int argc, goto done; } } - } /* while */ retval = 0; done: diff --git a/apps/restconf/restconf_main_native.c b/apps/restconf/restconf_main_native.c index bced28e2..13293d98 100644 --- a/apps/restconf/restconf_main_native.c +++ b/apps/restconf/restconf_main_native.c @@ -1326,7 +1326,6 @@ restconf_native_terminate(clicon_handle h) return 0; } - /*! Query backend of config. * Loop to wait for backend starting, try again if not done * @param[out] xrestconf XML restconf config, malloced (if retval = 1) @@ -1733,6 +1732,10 @@ restconf_sig_term(int arg) } else exit(-1); + /* This should ensure no more accepts or incoming packets are processed because next time eventloop + * is entered, it will terminate. + * However there may be a case of sockets closing rather abruptly for clients + */ clicon_exit_set(); } diff --git a/lib/src/clixon_proc.c b/lib/src/clixon_proc.c index 0798b835..6e0b9766 100644 --- a/lib/src/clixon_proc.c +++ b/lib/src/clixon_proc.c @@ -597,7 +597,8 @@ clixon_process_operation(clicon_handle h, process_entry_t *pe; proc_operation op; int sched = 0; /* If set, process action should be scheduled, register a timeout */ - + int isrunning = 0; + clicon_debug(1, "%s name:%s op:%s", __FUNCTION__, name, clicon_int2str(proc_operation_map, op0)); if (_proc_entry_list == NULL) goto ok; @@ -609,12 +610,31 @@ clixon_process_operation(clicon_handle h, if (wrapit && pe->pe_callback != NULL) if (pe->pe_callback(h, pe, &op) < 0) goto done; - if (op == PROC_OP_START || op == PROC_OP_STOP || op == PROC_OP_RESTART){ pe->pe_operation = op; clicon_debug(1, "%s scheduling name: %s pid:%d op: %s", __FUNCTION__, name, pe->pe_pid, clicon_int2str(proc_operation_map, pe->pe_operation)); + if (pe->pe_state==PROC_STATE_RUNNING && + (op == PROC_OP_STOP || op == PROC_OP_RESTART)){ + isrunning = 0; + if (proc_op_run(pe->pe_pid, &isrunning) < 0) + goto done; + if (isrunning) { + clicon_log(LOG_NOTICE, "Killing old process %s with pid: %d", + pe->pe_name, pe->pe_pid); /* XXX pid may be 0 */ + kill(pe->pe_pid, SIGTERM); + + } + clicon_debug(1, "%s %s(%d) %s --%s--> %s", __FUNCTION__, + pe->pe_name, pe->pe_pid, + clicon_int2str(proc_state_map, pe->pe_state), + clicon_int2str(proc_operation_map, pe->pe_operation), + clicon_int2str(proc_state_map, PROC_STATE_EXITING) + ); + pe->pe_state = PROC_STATE_EXITING; /* Keep operation stop/restart */ + // break; + } sched++; } else{ @@ -750,6 +770,7 @@ clixon_process_sched(int fd, int retval = -1; process_entry_t *pe; int isrunning; /* Process is actually running */ + int sched = 0; clicon_debug(1, "%s",__FUNCTION__); if (_proc_entry_list == NULL) @@ -762,6 +783,21 @@ clixon_process_sched(int fd, if (pe->pe_operation != PROC_OP_NONE){ switch (pe->pe_state){ case PROC_STATE_EXITING: + switch (pe->pe_operation){ + case PROC_OP_STOP: + case PROC_OP_RESTART: /* Kill again */ + isrunning = 0; + if (proc_op_run(pe->pe_pid, &isrunning) < 0) + goto done; + if (isrunning) { + clicon_log(LOG_NOTICE, "Killing old process %s with pid: %d", + pe->pe_name, pe->pe_pid); /* XXX pid may be 0 */ + kill(pe->pe_pid, SIGTERM); + sched++; + } + default: + break; + } break; /* only clixon_process_waitpid can change state in exiting */ case PROC_STATE_STOPPED: switch (pe->pe_operation){ @@ -794,24 +830,6 @@ clixon_process_sched(int fd, if (proc_op_run(pe->pe_pid, &isrunning) < 0) goto done; switch (pe->pe_operation){ - case PROC_OP_STOP: - clicon_debug(1, "%s stop pid:%d", __FUNCTION__, pe->pe_pid); - case PROC_OP_RESTART: - if (isrunning){ - clicon_log(LOG_NOTICE, "Killing old process %s with pid: %d", pe->pe_name, pe->pe_pid); - kill(pe->pe_pid, SIGTERM); - /* Cant wait here because it would block the backend and terminating may involve - * some protocol handling, instead SIGCHLD is receoved and - * clixon_process_waitpid is called that for waits/reaps the dead process */ - } - clicon_debug(1, "%s %s(%d) %s --%s--> %s", __FUNCTION__, - pe->pe_name, pe->pe_pid, - clicon_int2str(proc_state_map, pe->pe_state), - clicon_int2str(proc_operation_map, pe->pe_operation), - clicon_int2str(proc_state_map, PROC_STATE_EXITING) - ); - pe->pe_state = PROC_STATE_EXITING; /* Keep operation stop/restart */ - break; case PROC_OP_START: if (isrunning) /* Already runs */ break; @@ -835,6 +853,8 @@ clixon_process_sched(int fd, } pe = NEXTQ(process_entry_t *, pe); } while (pe != _proc_entry_list); + if (sched && clixon_process_sched_register(h) < 0) + goto done; ok: retval = 0; done: diff --git a/test/test_datastore.sh b/test/test_datastore.sh index 08fd8c42..fbe51466 100755 --- a/test/test_datastore.sh +++ b/test/test_datastore.sh @@ -163,6 +163,7 @@ expectpart "$($clixon_util_datastore $conf lock 756)" 0 "" # unset conditional parameters unset clixon_util_datastore +unset ret rm -rf $mydir diff --git a/test/test_feature.sh b/test/test_feature.sh index 721adc2d..f5e6d036 100755 --- a/test/test_feature.sh +++ b/test/test_feature.sh @@ -303,6 +303,8 @@ if [ $BE -ne 0 ]; then stop_backend -f $cfg fi +unset ret + endtest rm -rf $dir diff --git a/test/test_nacm_default.sh b/test/test_nacm_default.sh index ee515a08..725a11af 100755 --- a/test/test_nacm_default.sh +++ b/test/test_nacm_default.sh @@ -243,3 +243,4 @@ unset RESTCONFIG # unset conditional parameters unset format +unset ret diff --git a/test/test_nacm_module_read.sh b/test/test_nacm_module_read.sh index 5dd8b480..8e386d06 100755 --- a/test/test_nacm_module_read.sh +++ b/test/test_nacm_module_read.sh @@ -272,6 +272,7 @@ fi # Set by restconf_config unset RESTCONFIG +unset ret rm -rf $dir diff --git a/test/test_perf_restconf.sh b/test/test_perf_restconf.sh index 2c37d872..80f7c2ec 100755 --- a/test/test_perf_restconf.sh +++ b/test/test_perf_restconf.sh @@ -249,6 +249,7 @@ unset RESTCONFIG unset format unset perfnr unset perfreq +unset ret new "endtest" endtest diff --git a/test/test_restconf.sh b/test/test_restconf.sh index 481dd559..72330b3c 100755 --- a/test/test_restconf.sh +++ b/test/test_restconf.sh @@ -413,7 +413,7 @@ if [ "${WITH_RESTCONF}" = "native" ]; then protos="$protos https" fi for proto in $protos; do -# addrs="127.0.0.1" + addrs="127.0.0.1" if $IPv6 ; then addrs="$addrs \[::1\]" fi @@ -429,6 +429,7 @@ unset RCPROTO # Set by restconf_config unset RESTCONFIG unset RESTCONFIG1 +unset ret rm -rf $dir diff --git a/test/test_restconf_internal.sh b/test/test_restconf_internal.sh index 987a223a..557a61bf 100755 --- a/test/test_restconf_internal.sh +++ b/test/test_restconf_internal.sh @@ -78,7 +78,7 @@ function rpcstatus() sleep $DEMSLEEP new "send rpc status" - ret=$($clixon_netconf -qf $cfg< @@ -90,14 +90,14 @@ EOF ) # Check pid expect="[0-9]*" - match=$(echo "$ret" | grep --null -Go "$expect") + match=$(echo "$retx" | grep --null -Go "$expect") if [ -z "$match" ]; then pid=0 else pid=$(echo "$match" | awk -F'[<>]' '{print $3}') fi if [ -z "$pid" ]; then - err "No pid return value" "$ret" + err "No pid return value" "$retx" fi if $active; then expect="^$activeClixon RESTCONF process/www-data/clixon_restconf -f $cfg -D [0-9]$status20[0-9][0-9]\-[0-9][0-9]\-[0-9][0-9]T[0-9][0-9]:[0-9][0-9]:[0-9][0-9]\.[0-9]*Z$pid]]>]]>$" @@ -105,9 +105,9 @@ EOF # inactive, no startime or pid expect="^$activeClixon RESTCONF process/www-data/clixon_restconf -f $cfg -D [0-9]$status]]>]]>$" fi - match=$(echo "$ret" | grep --null -Go "$expect") + match=$(echo "$retx" | grep --null -Go "$expect") if [ -z "$match" ]; then - err "$expect" "$ret" + err "$expect" "$retx" fi } @@ -223,7 +223,7 @@ new "4. stop restconf RPC" rpcoperation stop if [ $? -ne 0 ]; then exit -1; fi -new "Wait for restrconf to stop" +new "Wait for restconf to stop" wait_restconf_stopped new "5. Get rpc status stopped" @@ -422,7 +422,7 @@ expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" -new "17. check status RPC off" +new "18. check status RPC off" rpcstatus false stopped if [ $pid -ne 0 ]; then err "Pid" "$pid"; fi @@ -448,16 +448,19 @@ fi #Start backend -s none should start -unset pid +new "kill restconf" +stop_restconf + sleep $DEMSLEEP # Lots of processes need to die before next test new "endtest" endtest # Set by restconf_config +unset pid unset RESTCONFIG unset RESTCONFDBG unset RCPROTO +unset retx rm -rf $dir - diff --git a/test/test_restconf_internal_usecases.sh b/test/test_restconf_internal_usecases.sh index 8b364f2d..3979adcc 100755 --- a/test/test_restconf_internal_usecases.sh +++ b/test/test_restconf_internal_usecases.sh @@ -92,7 +92,7 @@ function rpcstatus() sleep $DEMSLEEP new "send rpc status" - ret=$($clixon_netconf -qf $cfg< @@ -104,14 +104,14 @@ EOF ) # Check pid expect="[0-9]*" - match=$(echo "$ret" | grep --null -Go "$expect") + match=$(echo "$retx" | grep --null -Go "$expect") if [ -z "$match" ]; then pid=0 else pid=$(echo "$match" | awk -F'[<>]' '{print $3}') fi if [ -z "$pid" ]; then - err "No pid return value" "$ret" + err "No pid return value" "$retx" fi if $active; then expect="^$activeClixon RESTCONF process/www-data/clixon_restconf -f $cfg -D [0-9]$status20[0-9][0-9]\-[0-9][0-9]\-[0-9][0-9]T[0-9][0-9]:[0-9][0-9]:[0-9][0-9]\.[0-9]*Z$pid]]>]]>$" @@ -119,9 +119,9 @@ EOF # inactive, no startime or pid expect="^$activeClixon RESTCONF process/www-data/clixon_restconf -f $cfg -D [0-9]$status]]>]]>$" fi - match=$(echo "$ret" | grep --null -Go "$expect") + match=$(echo "$retx" | grep --null -Go "$expect") if [ -z "$match" ]; then - err "$expect" "$ret" + err "$expect" "$retx" fi } @@ -142,6 +142,7 @@ if [ $BE -ne 0 ]; then new "start backend -s init -f $cfg" start_backend -s init -f $cfg fi + new "wait backend" wait_backend @@ -250,9 +251,9 @@ expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" diff --git a/test/test_restconf_op.sh b/test/test_restconf_op.sh index 7ca3e2fd..be981c36 100755 --- a/test/test_restconf_op.sh +++ b/test/test_restconf_op.sh @@ -229,6 +229,7 @@ fi # Set by restconf_config unset RESTCONFIG +unset ret rm -rf $dir diff --git a/test/test_upgrade_failsafe.sh b/test/test_upgrade_failsafe.sh index 5fe6441f..35892d44 100755 --- a/test/test_upgrade_failsafe.sh +++ b/test/test_upgrade_failsafe.sh @@ -367,5 +367,8 @@ fi # valgrindtest rm -rf $dir new "endtest" + +unset ret + endtest diff --git a/test/test_xml_trees.sh b/test/test_xml_trees.sh index ef3176f3..d98e7e79 100755 --- a/test/test_xml_trees.sh +++ b/test/test_xml_trees.sh @@ -61,11 +61,11 @@ function testrun(){ x0=$2 x1=$3 xp=$4 - ret=$5 + retx=$5 res=$6 echo "$clixon_util_xml_mod -o $op -y $fyang -b "$x0" -x "$x1" -p $xp $OPTS" - expectpart "$($clixon_util_xml_mod -o $op -y $fyang -b "$x0" -x "$x1" -p $xp $OPTS)" $ret "$res" + expectpart "$($clixon_util_xml_mod -o $op -y $fyang -b "$x0" -x "$x1" -p $xp $OPTS)" $retx "$res" } new "test params: -y $fyang $OPTS" @@ -125,6 +125,7 @@ rm -rf $dir # unset conditional parameters unset clixon_util_xml_mod +unset retx new "endtest" endtest diff --git a/test/test_yang_default.sh b/test/test_yang_default.sh index f9aa7643..0b2a53e5 100755 --- a/test/test_yang_default.sh +++ b/test/test_yang_default.sh @@ -126,5 +126,7 @@ testrun rm -rf $dir +unset ret + new "endtest" endtest diff --git a/test/test_yang_namespace.sh b/test/test_yang_namespace.sh index 8e6967d3..5f59d525 100755 --- a/test/test_yang_namespace.sh +++ b/test/test_yang_namespace.sh @@ -149,5 +149,7 @@ unset RESTCONFIG rm -rf $dir +unset ret + new "endtest" endtest