Re: [pve-devel] [PATCH qemu-server v2 2/4] api: vm start: introduce nets-host-mtu parameter for migration compat

[Fiona Ebner <f.ebner@proxmox.com>]

Am 04.09.25 um 11:52 AM schrieb Fabian Grünbichler:
> On September 4, 2025 11:28 am, Fiona Ebner wrote:
>> Am 04.09.25 um 11:11 AM schrieb Fabian Grünbichler:
>>> On September 3, 2025 4:22 pm, Fiona Ebner wrote:
>>>> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
>>>> index b571e6c1..95db271b 100644
>>>> --- a/src/PVE/API2/Qemu.pm
>>>> +++ b/src/PVE/API2/Qemu.pm
>>>> @@ -1484,10 +1494,37 @@ sub print_netdevice_full {
>>>>  
>>>>      my $mtu = $net->{mtu};
>>>>  
>>>> -    if ($net->{model} eq 'virtio' && $net->{bridge}) {
>>>> +    if (defined($host_mtu_migration)) {
>>>> +        if ($host_mtu_migration) {
>>>> +            if (defined($mtu) && $mtu != 1) {
>>>> +                if ($mtu != $host_mtu_migration) {
>>>> +                    log_warn(
>>>> +                        "netdev $netid: using 'host_mtu=$host_mtu_migration' for migration compat,"
>>>> +                            . " but value different from value in configuration '$mtu'");
>>>> +                } # else avoid being overly verbose if there is an explicit setting
>>>
>>> can this happen in practice (without manually editing the config or
>>> manual QMP commands)?
>>
>> I don't think so. But since we have the info, I though it'd be good to
>> warn about it.
>>
> 
> I just figured it makes the code harder to parse ;)

Ack, yes, I guess it's not really that important.

>>>
>>>> +            } else {
>>>> +                print "netdev $netid: using 'host_mtu=$host_mtu_migration' for migration compat\n";
>>>> +            }
>>>> +        } else {
>>>> +            print "netdev $netid: not adding 'host_mtu' parameter for migration compat\n";
>>>> +        }
>>>> +    }
>>>
>>> this if here
>>>
>>>> +
>>>> +    if (
>>>> +        $net->{model} eq 'virtio'
>>>> +        && $net->{bridge}
>>>> +        && (!defined($host_mtu_migration) || $host_mtu_migration)
>>>> +    ) {
>>>>          my $bridge_mtu = PVE::Network::read_bridge_mtu($net->{bridge});
>>>>  
>>>> -        if (!defined($mtu) || $mtu == 1) {
>>>> +        if ($host_mtu_migration) {
>>>
>>> and this if here could be combined?
>>
>> How? You mean setting $mtu = $host_mtu_migration; early if there is a
>> non-zero value?
> 
> something like this (untested):
> 
> ----8<----
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 77b8e9c4..a76a134a 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1494,7 +1494,13 @@ sub print_netdevice_full {
>  
>      my $mtu = $net->{mtu};
>  
> -    if (defined($host_mtu_migration)) {
> +    if (
> +        $net->{model} eq 'virtio'
> +        && $net->{bridge}
> +        && (!defined($host_mtu_migration) || $host_mtu_migration)
> +    ) {
> +        my $bridge_mtu = PVE::Network::read_bridge_mtu($net->{bridge});
> +
>          if ($host_mtu_migration) {
>              if (defined($mtu) && $mtu != 1) {
>                  if ($mtu != $host_mtu_migration) {
> @@ -1505,26 +1511,10 @@ sub print_netdevice_full {
>              } else {
>                  print "netdev $netid: using 'host_mtu=$host_mtu_migration' for migration compat\n";
>              }
> -        } else {
> -            print "netdev $netid: not adding 'host_mtu' parameter for migration compat\n";
> -        }
> -    }
> -
> -    if (
> -        $net->{model} eq 'virtio'
> -        && $net->{bridge}
> -        && (!defined($host_mtu_migration) || $host_mtu_migration)
> -    ) {
> -        my $bridge_mtu = PVE::Network::read_bridge_mtu($net->{bridge});
> -
> -        if ($host_mtu_migration) {
>              $mtu = $host_mtu_migration;
> -            # TODO PVE 10 - upgrade to failure? Certain network traffic can break like
> -            # iperf3 -c 10.10.10.11 -u -l 2k when host_mtu=9000 and bridge MTU=1500
> -            if ($mtu > $bridge_mtu) {
> -                log_warn("netdev $netid: MTU '$mtu' is bigger than the bridge MTU '$bridge_mtu'");
> -            }
> -        } elsif (!defined($mtu) || $mtu == 1) {
> +        }
> +
> +        if (!defined($mtu) || $mtu == 1) {
>              $mtu = $bridge_mtu;
>          } elsif ($mtu < 576) {
>              die "netdev $netid: MTU '$mtu' is smaller than the IP minimum MTU '576'\n";
> ---->8----
> 
> I dropped the log statement about host_mtu_migration being an explicit
> zero, since that is logged further below already if $mtu is defined, but
> it could of course be added back there also for the $mtu == undef case
> with a final
> 
> } elsif (defined($host_mtu_migration) && $host_mtu_migration == 0) {
>     log_warn(..);
> }

Oh, so that's what you meant, heh :) I'll look into it.

> 
>>
>>>
>>>> +            $mtu = $host_mtu_migration;
>>>> +            # TODO PVE 10 - upgrade to failure? Certain network traffic can break like
>>>> +            # iperf3 -c 10.10.10.11 -u -l 2k when host_mtu=9000 and bridge MTU=1500
>>>> +            if ($mtu > $bridge_mtu) {
>>>> +                log_warn("netdev $netid: MTU '$mtu' is bigger than the bridge MTU '$bridge_mtu'");
>>>> +            }
>>>> +        } elsif (!defined($mtu) || $mtu == 1) {
>>>
>>> this could still be an if, then the newly added warning above can be
>>> dropped
>>>
>>>>              $mtu = $bridge_mtu;
>>>>          } elsif ($mtu < 576) {
>>>>              die "netdev $netid: MTU '$mtu' is smaller than the IP minimum MTU '576'\n";
>>>> @@ -1495,7 +1532,7 @@ sub print_netdevice_full {
>>>>              die "netdev $netid: MTU '$mtu' is bigger than the bridge MTU '$bridge_mtu'\n";
>>>
>>> since it is covered by this one here
>>
>> If we want to go for early failure when host_mtu > bridge MTU then yes.
>> Because this is die, the above is warn ;) Doing that is fine by me, but
>> I wanted to propose the non-breaking option first. If we consider that
>> it's problematic in more cases than not, then I'll go for early failure.
>> And maybe I'll add a little more context to the error message (at least
>> when it happens for migration).
> 
> could still handle it in one place, and warn if $host_mtu_migration
> (either inline, or via an elsif coming first that compares
> $host_mtu_migration to $bridge_mtu), die otherwise - but I think if we
> die on this condition when starting, then making it fatal for migration
> starts might make sense as well, since the die would happen early on (so
> no harm done).. might want to add a warning in that case as well that
> tells the user what to do to make the VM migrateable though?

Fair point. Yes, that was what I meant with "more context to the error
message".


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel