From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 72D571FF0C1 for ; Fri, 18 Sep 2026 13:21:24 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 1EFE6214D8; Fri, 18 Sep 2026 13:21:21 +0200 (CEST) Message-ID: <17ce3fca-ab64-426a-b72c-2cb6b12ea4fe@proxmox.com> Date: Fri, 18 Sep 2026 13:21:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC pve-network 1/1] sdn: apply changes on all nodes in parallel To: Gabriel Goller References: <20260709140625.275618-1-s.hanreich@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.643 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) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust RCVD_IN_MSPIKE_H2 0.001 Average reputation (+2) 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: TG5N76ECWTWN37CKWI337GLUC2DUL6RN X-Message-ID-Hash: TG5N76ECWTWN37CKWI337GLUC2DUL6RN X-MailFrom: s.hanreich@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: 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 > >> 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