Re: [PATCH] migration: Apply migration specific keep-alive defaults to inet socket

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Sep 09, 2025 at 05:01:24PM +0200, Juraj Marcin wrote:
> From: Juraj Marcin <jmarcin@redhat.com>
> 
> Usual system defaults for TCP keep-alive options are too long for
> migration workload. On Linux, a TCP connection waits idle for 2 hours
> before it starts checking if the connection is not broken.
> 
> Now when InetSocketAddress supports keep-alive options [1], this patch
> applies migration specific defaults if they are not supplied by the user
> or the management software. With these defaults, a migration TCP stream
> waits idle for 1 minute and then sends 5 TCP keep-alive packets in 30
> second interval before considering the connection as broken.
> 
> System defaults can be still used by explicitly setting these parameters
> to 0.

IMHO this is not a good idea. This is a very short default, which
may be fine for the scenario where your network conn is permanently
dead, but it is going to cause undesirable failures when the network
conn is only temporarily dead.

Optimizing defaults for temporary outages is much more preferrable
as that maximises reliability of migration. In the case of permanent
outages, it is already possible to tear down the connection without
waiting for a keep-alive timeout, and liveliness checks can also be
perform by the mgmt app at a higher level too. The TCP keepalives
are just an eventual failsafe, and having those work on a long
timeframe is OK.

> 
> [1]: 1bd4237cb1 "util/qemu-sockets: Introduce inet socket options
>      controlling TCP keep-alive"
> 
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> ---
>  migration/migration.c | 39 +++++++++++++++++++++++++++++++++++++++
>  qapi/migration.json   |  6 ++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 10c216d25d..a1f1223946 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -74,6 +74,11 @@
>  
>  #define INMIGRATE_DEFAULT_EXIT_ON_ERROR true
>  
> +#define INET_SOCKET_DEFAULT_KEEP_ALIVE true
> +#define INET_SOCKET_DEFAULT_KEEP_ALIVE_COUNT 5
> +#define INET_SOCKET_DEFAULT_KEEP_ALIVE_IDLE 60
> +#define INET_SOCKET_DEFAULT_KEEP_ALIVE_INTERVAL 30
> +
>  static NotifierWithReturnList migration_state_notifiers[] = {
>      NOTIFIER_ELEM_INIT(migration_state_notifiers, MIG_MODE_NORMAL),
>      NOTIFIER_ELEM_INIT(migration_state_notifiers, MIG_MODE_CPR_REBOOT),
> @@ -718,6 +723,36 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
>      return true;
>  }
>  
> +static void migration_address_apply_defaults(MigrationAddress *addr)
> +{
> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET &&
> +        addr->u.socket.type == SOCKET_ADDRESS_TYPE_INET) {
> +        InetSocketAddress *inet = &addr->u.socket.u.inet;
> +        if (!inet->has_keep_alive) {
> +            inet->has_keep_alive = true;
> +            inet->keep_alive = INET_SOCKET_DEFAULT_KEEP_ALIVE;
> +        }
> +#ifdef HAVE_TCP_KEEPCNT
> +        if (!inet->has_keep_alive_count) {
> +            inet->has_keep_alive_count = true;
> +            inet->keep_alive_count = INET_SOCKET_DEFAULT_KEEP_ALIVE_COUNT;
> +        }
> +#endif
> +#ifdef HAVE_TCP_KEEPIDLE
> +        if (!inet->has_keep_alive_idle) {
> +            inet->has_keep_alive_idle = true;
> +            inet->keep_alive_idle = INET_SOCKET_DEFAULT_KEEP_ALIVE_IDLE;
> +        }
> +#endif
> +#ifdef HAVE_TCP_KEEPINTVL
> +        if (!inet->has_keep_alive_interval) {
> +            inet->has_keep_alive_interval = true;
> +            inet->keep_alive_interval = INET_SOCKET_DEFAULT_KEEP_ALIVE_INTERVAL;
> +        }
> +#endif
> +    }
> +}
> +
>  static bool
>  migration_incoming_state_setup(MigrationIncomingState *mis, Error **errp)
>  {
> @@ -775,6 +810,8 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>          addr = channel->addr;
>      }
>  
> +    migration_address_apply_defaults(addr);
> +
>      /* transport mechanism not suitable for migration? */
>      if (!migration_transport_compatible(addr, errp)) {
>          return;
> @@ -2232,6 +2269,8 @@ void qmp_migrate(const char *uri, bool has_channels,
>          addr = channel->addr;
>      }
>  
> +    migration_address_apply_defaults(addr);
> +
>      /* transport mechanism not suitable for migration? */
>      if (!migration_transport_compatible(addr, errp)) {
>          return;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 2387c21e9c..68d4acb5db 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1639,6 +1639,12 @@
>  #
>  # Migration endpoint configuration.
>  #
> +# If transport is socket of type inet, it has the following defaults:
> +# keep-alive: true, keep-alive-count: 5, keep-alive-idle: 60 seconds,
> +# keep-alive-interval: 30 seconds.  System defaults can be used by
> +# explicitly setting these parameters to 0.  See `InetSocketAddress` for
> +# more details.
> +#
>  # @transport: The migration stream transport mechanism
>  #
>  # Since: 8.2
> -- 
> 2.51.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|