From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 7CE121FF0E1 for ; Mon, 13 Jul 2026 10:58:22 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 9FD2921350; Mon, 13 Jul 2026 10:58:21 +0200 (CEST) From: Daniel Herzig To: Stefan Hanreich Subject: Re: [RFC pve-network 1/1] sdn: apply changes on all nodes in parallel In-Reply-To: <20260709140625.275618-1-s.hanreich@proxmox.com> References: <20260709140625.275618-1-s.hanreich@proxmox.com> Date: Mon, 13 Jul 2026 10:58:12 +0200 Message-ID: <87ldbfe063.fsf@proxmox.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783933079706 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.837 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 3KWYBDLYUO3CKQMLG3XQZ76MJBXFTCAK X-Message-ID-Hash: 3KWYBDLYUO3CKQMLG3XQZ76MJBXFTCAK X-MailFrom: d.herzig@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Thanks for working on this! I've tested the patch in 2 scenarios on a three-node cluster: (1) Worst case (by using VLAN tag 1 on a VNet backed by a VLAN aware management bridge with default settings) (2) Good case (doing the same, but using VLAN tag 3 on the same bridge) Apart from the (expected) instant breaking of the network in case (1) the patch leads to a more consistent state as previously. The earlier sequential approach lead to inconsistent configuration states across the clusternodes (depending on which node was reached first, or -- to which node access to the management-IP was lost first). So I'd see the parallel approach as an enhancement in the worst-case setup. Case (2) just worked nicely, and makes application quite somewhat faster. I haven't gone through further-reaching implications yet, but my first impression of the patch is quite good by empirical testing of the above situations. Stefan Hanreich writes: > Utilize the API client to spawn tasks on each node and then monitor > the progress of the tasks via the API as well. This allows for > starting the reload tasks in parallel. It is particularly useful for > large setups, where applying the SDN configuration can take a > considerable amount of time. > > Signed-off-by: Stefan Hanreich > --- > > Notes: > Sending this as an RFC, as this requires careful testing imo. > > An example where this change could backfire has been encountered > recently. A user applied the SDN configuration, which broke the > cluster / management network of the node that was carrying out the > reload network task. Since it was executed sequentially, only one node > was affected. In this scenario, with this patch applied, the whole > cluster would have been down. > > We could potentially provide an option that limits the amount of nodes > where the reload task runs in parallel and expose that in the UI? > > Also, currently the task could never fail and errors were shown in the > respective reload tasks executed on the nodes, I've kept that behavior > the same, but introduced warnings if there are any issues with API > calls. > > src/PVE/API2/Network/SDN.pm | 109 ++++++++++++++++++++++++++---------- > 1 file changed, 78 insertions(+), 31 deletions(-) > > 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']], > }, > + 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; > + } > + > + next if $task->{status} eq 'running'; > + print "$node: reload task finished\n"; > + delete $tasks->{$upid}; > + } > + > + sleep(1); > + } > }; > > return $rpcenv->fork_worker('reloadnetworkall', undef, $authuser, $code); > - > }, > });