From 5e6bcaea33270eaeff31f7bacf92f0710544e04d Mon Sep 17 00:00:00 2001 From: Bram Matthys Date: Wed, 30 Jul 2025 08:57:40 +0200 Subject: [PATCH] After netsplit, wait for class::connfreq seconds before connecting to server. Isn't that what it was supposed to do? Well, yes and no, previously it only guaranteed that between reconnects (so the 2nd try not being before class::connfreq than the 1st try), but there were no guarantees for the first time period directly after a squit. * When a netsplit happens and [set::server-linking::autoconnect-strategy](https://www.unrealircd.org/docs/Set_block#set::server-linking) is `sequential` (which is the default) or `sequential-fallback` (which is a good value for leafs) then we now consistently wait for [class::connfreq](https://www.unrealircd.org/docs/Class_block) seconds before trying to connect to the (same or next) server. By default this is 15 seconds in the example configuration server class. The reason for this is to provide a consistent behavior. Previously we waited semi-randomly for 0 to class::connfreq seconds. The previous behavior caused the picking of 'next server to try' to be inconsistent, which especially caused issues for `sequential-fallback`. If you want quicker recovery times in case of a netsplit, simply lower the value of [class::connfreq](https://www.unrealircd.org/docs/Class_block) in your configuration file, e.g. to 5 instead of 15 seconds. Oh yeah and for connect-strategy 'parallel' things stay as is, with the wait of 0 to class::connfreq per-server, which seems fine for that. Unless you want a 'BOOM!' effect of mass reconnects instantly, in which case you can just set class::connfreq very low. --- doc/RELEASE-NOTES.md | 14 +++++++++++ src/modules/server.c | 57 ++++++++++++++++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/doc/RELEASE-NOTES.md b/doc/RELEASE-NOTES.md index 667469ce9..5acfff566 100644 --- a/doc/RELEASE-NOTES.md +++ b/doc/RELEASE-NOTES.md @@ -94,6 +94,20 @@ open to users, .. will work fine if you use `cloak_sha256`). ### Changes: +* When a netsplit happens and + [set::server-linking::autoconnect-strategy](https://www.unrealircd.org/docs/Set_block#set::server-linking) + is `sequential` (which is the default) or `sequential-fallback` + (which is a good value for leafs) then we now consistently wait for + [class::connfreq](https://www.unrealircd.org/docs/Class_block) + seconds before trying to connect to the (same or next) server. + By default this is 15 seconds in the example configuration file + server class. The reason for this is to provide a consistent behavior. + Previously we waited semi-randomly for 0 to class::connfreq seconds. + The previous behavior caused the picking of 'next server to try' to + be inconsistent, which especially caused issues for `sequential-fallback`. + If you want quicker recovery times in case of a netsplit, simply lower + the value of [class::connfreq](https://www.unrealircd.org/docs/Class_block) + in your configuration file, e.g. to 5 instead of 15 seconds. * Currently it is still possible to link servers without certificate verification. This would be rare, since our [server linking guide](https://www.unrealircd.org/docs/Tutorial:_Linking_servers) diff --git a/src/modules/server.c b/src/modules/server.c index c8f3b4935..9bb61e7ef 100644 --- a/src/modules/server.c +++ b/src/modules/server.c @@ -72,6 +72,7 @@ int _is_services_but_not_ulined(Client *client); const char *_check_deny_link(ConfigItem_link *link, int auto_connect); int server_stats_denylink_all(Client *client, const char *para); int server_stats_denylink_auto(Client *client, const char *para); +int server_quit_reset_autoconnect_time(Client *client, MessageTag *mtags); /* Global variables */ static cfgstruct cfg; @@ -112,6 +113,7 @@ MOD_INIT() HookAdd(modinfo->handle, HOOKTYPE_POST_SERVER_CONNECT, 0, server_post_connect); HookAdd(modinfo->handle, HOOKTYPE_STATS, 0, server_stats_denylink_all); HookAdd(modinfo->handle, HOOKTYPE_STATS, 0, server_stats_denylink_auto); + HookAdd(modinfo->handle, HOOKTYPE_SERVER_QUIT, 0, server_quit_reset_autoconnect_time); CommandAdd(modinfo->handle, "SERVER", cmd_server, MAXPARA, CMD_UNREGISTERED|CMD_SERVER); CommandAdd(modinfo->handle, "SID", cmd_sid, MAXPARA, CMD_SERVER); @@ -449,7 +451,9 @@ int server_needs_linking(ConfigItem_link *aconf) if (!(aconf->outgoing.options & CONNECT_OUTGOING_AUTO) || (!aconf->outgoing.hostname && !aconf->outgoing.file) || (aconf->flag.temporary == 1)) + { return 0; + } class = aconf->class; @@ -457,7 +461,14 @@ int server_needs_linking(ConfigItem_link *aconf) if ((aconf->hold > TStime())) return 0; - aconf->hold = TStime() + class->connfreq; + /* For parallel we maintain the old algorithm where the first + * reconnect is somewhere between 0 and class::connfreq + * For other strategies we don't, see comment in + * server_quit_reset_autoconnect_time() for more info, + * or the whole commit actually. + */ + if (cfg.autoconnect_strategy == AUTOCONNECT_PARALLEL) + aconf->hold = TStime() + class->connfreq; client = find_client(aconf->servername, NULL); if (client) @@ -471,6 +482,9 @@ int server_needs_linking(ConfigItem_link *aconf) return 0; /* Yes, this server is a linking candidate */ + + /* Set the hold time. */ + aconf->hold = TStime() + class->connfreq; return 1; } @@ -497,7 +511,7 @@ ConfigItem_link *find_first_autoconnect_server(void) for (aconf = conf_link; aconf; aconf = aconf->next) { - if (!server_needs_linking(aconf)) + if (aconf->flag.temporary || !server_needs_linking(aconf)) continue; return aconf; /* found! */ } @@ -526,6 +540,8 @@ ConfigItem_link *find_next_autoconnect_server(char *current) /* Otherwise, walk the list up to 'current' */ for (aconf = conf_link; aconf; aconf = aconf->next) { + if (aconf->flag.temporary) + continue; if (!strcmp(aconf->servername, current)) break; } @@ -536,6 +552,7 @@ ConfigItem_link *find_next_autoconnect_server(char *current) * removed of a server that we just happened to * try to link to before, so we can afford to do * it this way. + * Oh and this could return NULL (no linking needed). */ if (!aconf) return find_first_autoconnect_server(); @@ -546,6 +563,8 @@ ConfigItem_link *find_next_autoconnect_server(char *current) */ for (aconf = aconf->next; aconf; aconf = aconf->next) { + if (aconf->flag.temporary) + continue; if (!server_needs_linking(aconf)) continue; return aconf; /* found! */ @@ -559,12 +578,10 @@ ConfigItem_link *find_next_autoconnect_server(char *current) */ for (aconf = conf_link; aconf; aconf = aconf->next) { - if (!server_needs_linking(aconf)) - { - if (!strcmp(aconf->servername, current)) - break; /* need to stop here */ + if (aconf->flag.temporary) + continue; + if (!server_needs_linking(aconf)) continue; - } return aconf; /* found! */ } @@ -1998,6 +2015,32 @@ int server_post_connect(Client *client) { return 0; } +int server_quit_reset_autoconnect_time(Client *client, MessageTag *mtags) +{ + if ((cfg.autoconnect_strategy == AUTOCONNECT_SEQUENTIAL) || + (cfg.autoconnect_strategy == AUTOCONNECT_SEQUENTIAL_FALLBACK)) + { + /* If the connect strategy is sequential or sequential-fallback, + * because we don't reset aconf->hold in the loop in + * server_needs_linking(), we reset the hold time of all servers + * here. If we wouldn't do that then servers would (re)connect + * immediately within like <2 seconds after a split. + * Which can be nice, but also be hard for IRCOps to fight a bad + * server link. Also, it feels like violating the connfreq + * if we don't do this. + * More importantly, this is overall change was needed because + * otherwise the "try next server" or "try first server" aspect + * with autoconnect strategy "sequential" and "sequential-fallback" + * was not working properly (was rather inconsistent). + */ + ConfigItem_link *aconf; + for (aconf = conf_link; aconf; aconf = aconf->next) + aconf->hold = TStime() + aconf->class->connfreq; + } + + return 0; +} + /** Start an outgoing connection to a server, for server linking. * @param aconf Configuration attached to this server * @param by The user initiating the connection (can be NULL)