From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 13AAD1FF139 for ; Tue, 24 Feb 2026 14:53:03 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C6E93C061; Tue, 24 Feb 2026 14:53:55 +0100 (CET) Message-ID: <146810ff-7103-4da0-a921-d1c570dc68e7@proxmox.com> Date: Tue, 24 Feb 2026 14:53:20 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-network 09/10] api: add dry-run endpoint for sdn apply to preview changes To: pve-devel@lists.proxmox.com References: <20260203160246.353351-1-g.goller@proxmox.com> <20260203160246.353351-21-g.goller@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20260203160246.353351-21-g.goller@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.343 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 1.179 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.717 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.236 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: B556J7WOH2AZ2FJ7I2KMWSDS7BNFAP6X X-Message-ID-Hash: B556J7WOH2AZ2FJ7I2KMWSDS7BNFAP6X 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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Another idea, related to the comments already sent in pve-manager: We could potentially return the output in a similar way to how the tests dp it (using Tests::Difference) and then show this verbatim in the UI? Of course, having a native diff viewer in the UI that can render diffs would be preferable, but that seems a bit unrealistic considering that we would like to roll this out relatively soon and if we're returning raw diff outputs anyway, we could opt for a flavor that's a bit more human-readable? On 2/3/26 5:04 PM, Gabriel Goller wrote: > Allows users to see the diff of frr configuration before applying > SDN changes. Previously this was not possible and the user had to apply > and then see what changed. Ideally this would also include the ifupdown2 > config, but that's a bit tricky since we add config lines in perl-rs as > well. > > Signed-off-by: Gabriel Goller > --- > src/PVE/API2/Network/SDN.pm | 67 +++++++++++++++++++++++++++++++++++++ > src/PVE/Network/SDN.pm | 9 +++-- > 2 files changed, 74 insertions(+), 2 deletions(-) > > diff --git a/src/PVE/API2/Network/SDN.pm b/src/PVE/API2/Network/SDN.pm > index b35a588d391d..9208d6f4e8b3 100644 > --- a/src/PVE/API2/Network/SDN.pm > +++ b/src/PVE/API2/Network/SDN.pm > @@ -3,6 +3,9 @@ package PVE::API2::Network::SDN; > use strict; > use warnings; > > +use File::Temp qw(tempfile); There's a wrapper PVE::File::tempfile(_contents) which we should probably use instead? > +use Encode qw(decode); > + > 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); > @@ -325,4 +328,68 @@ __PACKAGE__->register_method({ > }, > }); > > +sub get_diff { > + my ($filename_one, $filename_two) = @_; > + > + my $diff = ''; > + > + my $cmd = ['/usr/bin/diff', '-b', '-N', '-u', $filename_one, $filename_two]; > + PVE::Tools::run_command( > + $cmd, > + noerr => 1, > + outfunc => sub { > + my ($line) = @_; > + $diff .= decode('UTF-8', $line) . "\n"; > + }, > + ); > + > + $diff = undef if !$diff; > + > + return $diff; > +} > + > +__PACKAGE__->register_method({ > + name => 'dry-apply', > + path => 'dry-apply', We refer to the action as dry-run everywhere but the path is actually 'dry-apply'? I think dry-run would fit better. > + method => 'PUT', > + permissions => { > + check => ['perm', '/nodes/{node}', ['Sys.Modify']], > + }, > + description => "Dry-Run the SDN apply", make this more descriptive? > + protected => 1, > + proxyto => 'node', > + parameters => { > + additionalProperties => 0, > + properties => { > + node => get_standard_option('pve-node'), > + }, > + }, > + > + returns => { > + type => 'object', > + properties => { > + "frr-diff" => > + { type => 'string', description => 'The frr config generated by SDN.' }, description is kinda wrong? It's the difference/changes in the FRR configuration - not the config itself. > + }, > + }, > + code => sub { > + my ($param) = @_; > + > + my $config = PVE::Network::SDN::compile_running_cfg(); > + > + my $fabric_config = PVE::Network::SDN::Fabrics::config(0); > + my $frr_config = PVE::Network::SDN::generate_frr_raw_config($config, $fabric_config); > + my $new_config_frr = PVE::Network::SDN::Frr::raw_config_to_string($frr_config); > + > + my ($frr_tmp_fh, $frr_tmp_filename) = tempfile(); see above comment regarding PVE::File::tempfile > + print $frr_tmp_fh $new_config_frr; > + > + my $return_value = {}; > + $return_value->{"frr-diff"} = get_diff('/etc/frr/frr.conf', $frr_tmp_filename); > + > + close($frr_tmp_fh); > + return $return_value; > + }, > +}); > + > 1; > diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm > index c000bed498ec..18938d73ba70 100644 > --- a/src/PVE/Network/SDN.pm > +++ b/src/PVE/Network/SDN.pm > @@ -187,8 +187,7 @@ sub pending_config { > > } > > -sub commit_config { > - > +sub compile_running_cfg { > my $cfg = cfs_read_file($running_cfg); > my $version = $cfg->{version}; > > @@ -219,6 +218,12 @@ sub commit_config { > fabrics => $fabrics, > }; > > + return $cfg; > +} > + > +sub commit_config { > + my $cfg = compile_running_cfg(); > + > cfs_write_file($running_cfg, $cfg); > } >