Re: [RFC pve-network 1/1] sdn: apply changes on all nodes in parallel

[Stefan Hanreich <s.hanreich@proxmox.com>]

On 9/18/26 11:50 AM, Gabriel Goller wrote:
> A few small nits inline, no blockers though.
> Otherwise LGTM
> 
> Consider:
> Reviewed-by: Gabriel Goller <g.goller@proxmox.com>
> 
>> diff --git a/src/PVE/API2/Network/SDN.pm b/src/PVE/API2/Network/SDN.pm
>> index e3c8d9dd..dd2cf05a 100644
>> --- a/src/PVE/API2/Network/SDN.pm
>> +++ b/src/PVE/API2/Network/SDN.pm
>> @@ -10,6 +10,7 @@ use PVE::Cluster qw(cfs_lock_file cfs_read_file cfs_write_file);
>>  use PVE::Exception qw(raise_param_exc);
>>  use PVE::JSONSchema qw(get_standard_option);
>>  use PVE::RESTHandler;
>> +use PVE::RESTEnvironment qw(log_warn);
>>  use PVE::RPCEnvironment;
>>  use PVE::SafeSyslog;
>>  use PVE::Tools qw(run_command extract_param);
>> @@ -107,30 +108,6 @@ __PACKAGE__->register_method({
>>      },
>>  });
>>  
>> -my $create_reload_network_worker = sub {
>> -    my ($nodename, $regenerate_frr) = @_;
>> -
>> -    my @command = ('pvesh', 'set', "/nodes/$nodename/network");
>> -    push(@command, '--regenerate-frr', $regenerate_frr);
>> -
>> -    # FIXME: how to proxy to final node ?
>> -    my $upid;
>> -    print "$nodename: reloading network config\n";
>> -    run_command(
>> -        \@command,
>> -        outfunc => sub {
>> -            my $line = shift;
>> -            if ($line =~ /["']?(UPID:[^\s"']+)["']?$/) {
>> -                $upid = $1;
>> -            }
>> -        },
>> -    );
>> -    #my $upid = PVE::API2::Network->reload_network_config({ node => $nodename });
>> -    my $res = PVE::Tools::upid_decode($upid);
>> -
>> -    return $res->{pid};
>> -};
>> -
>>  __PACKAGE__->register_method({
>>      name => 'lock',
>>      protected => 1,
>> @@ -282,6 +259,41 @@ __PACKAGE__->register_method({
>>      },
>>  });
>>  
>> +sub create_api_client {
>> +    my ($request_timeout) = @_;
>> +
>> +    my $rpcenv = PVE::RPCEnvironment::get();
>> +    my $authuser = $rpcenv->get_user();
>> +    my $credentials = $rpcenv->get_credentials();
>> +
>> +    my $api_token = $credentials->{api_token};
>> +    my $ticket = $credentials->{ticket};
>> +    my $csrf_token = $credentials->{token};
>> +
>> +    my $node = PVE::INotify::nodename();
>> +    my $fingerprint = PVE::Cluster::get_node_fingerprint($node);
>> +
>> +    my $conn_args = {
>> +        protocol => 'https',
>> +        host => 'localhost', # always call the api locally, let pveproxy handle the proxying
>> +        port => 8006,
>> +        username => $authuser,
>> +        ticket => $ticket,
>> +        apitoken => $api_token,
>> +        timeout => $request_timeout // 25, # default slightly shorter than the proxy->daemon timeout
>> +        cached_fingerprints => {
>> +            $fingerprint => 1,
>> +        },
>> +    };
>> +
>> +    my $api_client = PVE::APIClient::LWP->new($conn_args->%*);
>> +    if (defined($csrf_token)) {
>> +        $api_client->update_csrftoken($csrf_token);
>> +    }
>> +
>> +    return $api_client;
>> +}
>> +
>>  __PACKAGE__->register_method({
>>      name => 'reload',
>>      protected => 1,
>> @@ -291,6 +303,7 @@ __PACKAGE__->register_method({
>>      permissions => {
>>          check => ['perm', '/sdn', ['SDN.Allocate']],
> 
> Hmm this endpoint has SDN.Allocate and the PUT /network endpoint we are calling
> (in pve-manager) needs Sys.Modify. This shouldn't be an issue, but maybe we
> should require Sys.Modify here as well to short-circuit everything, instead of
> making a request to every node.

Hmm, pre-existing, but what about setups where a user has Sys.Modify only on some nodes? That's
currently possible but probably quite bad, since it allows applying the SDN configuration to 
some nodes only. So, I agree - but a breaking change?

>>      },
>> +    expose_credentials => 1,
>>      parameters => {
>>          additionalProperties => 0,
>>          properties => {
>> @@ -336,22 +349,56 @@ __PACKAGE__->register_method({
>>          my $regenerate_frr = ($previous_config_has_frr || $new_config_has_frr) ? 1 : 0;
>>  
>>          my $code = sub {
>> -            $rpcenv->{type} = 'priv'; # to start tasks in background
>>              PVE::Cluster::check_cfs_quorum();
>> +
>> +            my $api_client = create_api_client();
>>              my $nodelist = PVE::Cluster::get_nodelist();
>> +
>> +            my $tasks = {};
>> +
>>              for my $node (@$nodelist) {
>> -                my $pid = eval { $create_reload_network_worker->($node, $regenerate_frr) };
>> -                warn $@ if $@;
>> +                print "$node: reloading network config\n";
>> +
>> +                my $upid = eval {
>> +                    $api_client->put(
>> +                        "/nodes/$node/network",
>> +                        {
>> +                            'regenerate-frr' => $regenerate_frr,
>> +                        },
>> +                    );
>> +                };
>> +
>> +                if ($@) {
>> +                    log_warn("$node: could not reload network configuration: $@\n");
>> +                    next;
>> +                }
>> +
>> +                $tasks->{$upid} = $node;
>>              }
>>  
>> -            # FIXME: use libpve-apiclient (like in cluster join) to create
>> -            # tasks and moitor the tasks.
>> +            print "waiting for reload tasks to finish\n";
>>  
>> -            return;
>> +            while ($tasks->%*) {
>> +                for my $upid (keys $tasks->%*) {
>> +                    my $node = $tasks->{$upid};
>> +                    my $task = eval { $api_client->get("/nodes/$node/tasks/$upid/status") };
>> +
>> +                    if ($@) {
>> +                        log_warn("could not get status of reload task: $@\n");
>> +                        delete $tasks->{$upid};
>> +                        next;
>> +                    }
> 
> Hmm should we wait more than 1 second here maybe? What if the reload brings the
> network down shortly... Maybe add a counter and require a task to be gone for
> 3-4 seconds?

yes, potentially even a re-try logic before giving up completely...

>> +
>> +                    next if $task->{status} eq 'running';
> 
> Should we add a exitstatus check here and print a warning like "reload failed"
> or something?
> 
>> +                    print "$node: reload task finished\n";
>> +                    delete $tasks->{$upid};
>> +                }
>> +
>> +                sleep(1);
> 
> Do the pve tasks have a maximum duration where they get killed if they take
> longer? Otherwise a stuck task will make this loop forever.
> 
> Although this is a task as well isn't it -- it can just be force-stopped in
> the ui.

Should be fine imo, due to the possibility of cancelling the task itself.

>> +            }
>>          };
>>  
>>          return $rpcenv->fork_worker('reloadnetworkall', undef, $authuser, $code);
>> -
>>      },
>>  });
>>  
>> -- 
>> 2.47.3