Commit e4ab38a3 authored by 杨博东's avatar 杨博东 Committed by Oran Agra
Browse files

Fix flock cluster config may cause failure to restart after kill -9 (#7674)



After fork, the child process(redis-aof-rewrite) will get the fd opened
by the parent process(redis), when redis killed by kill -9, it will not
graceful exit(call prepareForShutdown()), so redis-aof-rewrite thread may still
alive, the fd(lock) will still be held by redis-aof-rewrite thread, and
redis restart will fail to get lock, means fail to start.

This issue was causing failures in the cluster tests in github actions.
Co-authored-by: default avatarOran Agra <oran@redislabs.com>
(cherry picked from commit cbaf3c5b)
parent c86eabb0
...@@ -1578,7 +1578,7 @@ int rewriteAppendOnlyFileBackground(void) { ...@@ -1578,7 +1578,7 @@ int rewriteAppendOnlyFileBackground(void) {
char tmpfile[256]; char tmpfile[256];
/* Child */ /* Child */
closeListeningSockets(0); closeClildUnusedResourceAfterFork();
redisSetProcTitle("redis-aof-rewrite"); redisSetProcTitle("redis-aof-rewrite");
snprintf(tmpfile,256,"temp-rewriteaof-bg-%d.aof", (int) getpid()); snprintf(tmpfile,256,"temp-rewriteaof-bg-%d.aof", (int) getpid());
if (rewriteAppendOnlyFile(tmpfile) == C_OK) { if (rewriteAppendOnlyFile(tmpfile) == C_OK) {
......
...@@ -418,7 +418,15 @@ int clusterLockConfig(char *filename) { ...@@ -418,7 +418,15 @@ int clusterLockConfig(char *filename) {
return C_ERR; return C_ERR;
} }
/* Lock acquired: leak the 'fd' by not closing it, so that we'll retain the /* Lock acquired: leak the 'fd' by not closing it, so that we'll retain the
* lock to the file as long as the process exists. */ * lock to the file as long as the process exists.
*
* After fork, the child process will get the fd opened by the parent process,
* we need save `fd` to `cluster_config_file_lock_fd`, so that in redisFork(),
* it will be closed in the child process.
* If it is not closed, when the main process is killed -9, but the child process
* (redis-aof-rewrite) is still alive, the fd(lock) will still be held by the
* child process, and the main process will fail to get lock, means fail to start. */
server.cluster_config_file_lock_fd = fd;
#endif /* __sun */ #endif /* __sun */
return C_OK; return C_OK;
...@@ -468,6 +476,7 @@ void clusterInit(void) { ...@@ -468,6 +476,7 @@ void clusterInit(void) {
/* Lock the cluster config file to make sure every node uses /* Lock the cluster config file to make sure every node uses
* its own nodes.conf. */ * its own nodes.conf. */
server.cluster_config_file_lock_fd = -1;
if (clusterLockConfig(server.cluster_configfile) == C_ERR) if (clusterLockConfig(server.cluster_configfile) == C_ERR)
exit(1); exit(1);
......
...@@ -1340,7 +1340,7 @@ int rdbSaveBackground(char *filename, rdbSaveInfo *rsi) { ...@@ -1340,7 +1340,7 @@ int rdbSaveBackground(char *filename, rdbSaveInfo *rsi) {
int retval; int retval;
/* Child */ /* Child */
closeListeningSockets(0); closeClildUnusedResourceAfterFork();
redisSetProcTitle("redis-rdb-bgsave"); redisSetProcTitle("redis-rdb-bgsave");
retval = rdbSave(filename,rsi); retval = rdbSave(filename,rsi);
if (retval == C_OK) { if (retval == C_OK) {
...@@ -2350,7 +2350,7 @@ int rdbSaveToSlavesSockets(rdbSaveInfo *rsi) { ...@@ -2350,7 +2350,7 @@ int rdbSaveToSlavesSockets(rdbSaveInfo *rsi) {
rioInitWithFdset(&slave_sockets,fds,numfds); rioInitWithFdset(&slave_sockets,fds,numfds);
zfree(fds); zfree(fds);
closeListeningSockets(0); closeClildUnusedResourceAfterFork();
redisSetProcTitle("redis-rdb-to-slaves"); redisSetProcTitle("redis-rdb-to-slaves");
retval = rdbSaveRioWithEOFMark(&slave_sockets,NULL,rsi); retval = rdbSaveRioWithEOFMark(&slave_sockets,NULL,rsi);
......
...@@ -1675,7 +1675,7 @@ int ldbStartSession(client *c) { ...@@ -1675,7 +1675,7 @@ int ldbStartSession(client *c) {
* socket to make sure if the parent crashes a reset is sent * socket to make sure if the parent crashes a reset is sent
* to the clients. */ * to the clients. */
serverLog(LL_WARNING,"Redis forked for debugging eval"); serverLog(LL_WARNING,"Redis forked for debugging eval");
closeListeningSockets(0); closeClildUnusedResourceAfterFork();
} else { } else {
/* Parent */ /* Parent */
listAddNodeTail(ldb.children,(void*)(unsigned long)cp); listAddNodeTail(ldb.children,(void*)(unsigned long)cp);
......
...@@ -4035,6 +4035,16 @@ void setupSignalHandlers(void) { ...@@ -4035,6 +4035,16 @@ void setupSignalHandlers(void) {
return; return;
} }
/* After fork, the child process will inherit the resources
* of the parent process, e.g. fd(socket or flock) etc.
* should close the resources not used by the child process, so that if the
* parent restarts it can bind/lock despite the child possibly still running. */
void closeClildUnusedResourceAfterFork() {
closeListeningSockets(0);
if (server.cluster_enabled && server.cluster_config_file_lock_fd != -1)
close(server.cluster_config_file_lock_fd); /* don't care if this fails */
}
void memtest(size_t megabytes, int passes); void memtest(size_t megabytes, int passes);
/* Returns 1 if there is --sentinel among the arguments or if /* Returns 1 if there is --sentinel among the arguments or if
......
...@@ -1260,6 +1260,7 @@ struct redisServer { ...@@ -1260,6 +1260,7 @@ struct redisServer {
to set in order to suppress certain to set in order to suppress certain
native Redis Cluster features. Check the native Redis Cluster features. Check the
REDISMODULE_CLUSTER_FLAG_*. */ REDISMODULE_CLUSTER_FLAG_*. */
int cluster_config_file_lock_fd; /* cluster config fd, will be flock */
/* Scripting */ /* Scripting */
lua_State *lua; /* The Lua interpreter. We use just one for all clients */ lua_State *lua; /* The Lua interpreter. We use just one for all clients */
client *lua_client; /* The "fake client" to query Redis from Lua */ client *lua_client; /* The "fake client" to query Redis from Lua */
...@@ -1757,6 +1758,7 @@ void populateCommandTable(void); ...@@ -1757,6 +1758,7 @@ void populateCommandTable(void);
void resetCommandTableStats(void); void resetCommandTableStats(void);
void adjustOpenFilesLimit(void); void adjustOpenFilesLimit(void);
void closeListeningSockets(int unlink_unix_socket); void closeListeningSockets(int unlink_unix_socket);
void closeClildUnusedResourceAfterFork();
void updateCachedTime(int update_daylight_info); void updateCachedTime(int update_daylight_info);
void resetServerStats(void); void resetServerStats(void);
void activeDefragCycle(void); void activeDefragCycle(void);
......
...@@ -84,7 +84,9 @@ proc spawn_instance {type base_port count {conf {}}} { ...@@ -84,7 +84,9 @@ proc spawn_instance {type base_port count {conf {}}} {
# Check availability # Check availability
if {[server_is_up 127.0.0.1 $port 100] == 0} { if {[server_is_up 127.0.0.1 $port 100] == 0} {
abort_sentinel_test "Problems starting $type #$j: ping timeout" set logfile [file join $dirname log.txt]
puts [exec tail $logfile]
abort_sentinel_test "Problems starting $type #$j: ping timeout, maybe server start failed, check $logfile"
} }
# Push the instance into the right list # Push the instance into the right list
...@@ -462,12 +464,12 @@ proc kill_instance {type id} { ...@@ -462,12 +464,12 @@ proc kill_instance {type id} {
# Wait for the port it was using to be available again, so that's not # Wait for the port it was using to be available again, so that's not
# an issue to start a new server ASAP with the same port. # an issue to start a new server ASAP with the same port.
set retry 10 set retry 100
while {[incr retry -1]} { while {[incr retry -1]} {
set port_is_free [catch {set s [socket 127.0.01 $port]}] set port_is_free [catch {set s [socket 127.0.0.1 $port]}]
if {$port_is_free} break if {$port_is_free} break
catch {close $s} catch {close $s}
after 1000 after 100
} }
if {$retry == 0} { if {$retry == 0} {
error "Port $port does not return available after killing instance." error "Port $port does not return available after killing instance."
...@@ -494,7 +496,9 @@ proc restart_instance {type id} { ...@@ -494,7 +496,9 @@ proc restart_instance {type id} {
# Check that the instance is running # Check that the instance is running
if {[server_is_up 127.0.0.1 $port 100] == 0} { if {[server_is_up 127.0.0.1 $port 100] == 0} {
abort_sentinel_test "Problems starting $type #$id: ping timeout" set logfile [file join $dirname log.txt]
puts [exec tail $logfile]
abort_sentinel_test "Problems starting $type #$id: ping timeout, maybe server start failed, check $logfile"
} }
# Connect with it with a fresh link # Connect with it with a fresh link
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment