From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 8CE6A1FF13E for ; Fri, 06 Mar 2026 10:23:56 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 827071C62E; Fri, 6 Mar 2026 10:25:01 +0100 (CET) Message-ID: Date: Fri, 6 Mar 2026 10:24:22 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-network v3 9/9] api: add dry-run endpoint for sdn apply to preview changes To: Gabriel Goller , pve-devel@lists.proxmox.com References: <20260305100331.80741-1-g.goller@proxmox.com> <20260305100331.80741-19-g.goller@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20260305100331.80741-19-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_MSPIKE_H2 0.001 Average reputation (+2) RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.617 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.892 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.622 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: YA25ATSBUYZ3UN7Y64FX2MILMFPPRE5P X-Message-ID-Hash: YA25ATSBUYZ3UN7Y64FX2MILMFPPRE5P 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: On 3/5/26 11:04 AM, Gabriel Goller wrote: > Allows users to see the diff of frr and interfaces configuration files > before applying SDN changes. Previously this was not possible and the > user had to apply and then see what changed. For the ifupdown2 dry-run > to work, pull out the running_config generation to the parent functions. > > Also add an option to the compile_running_cfg function so that we can > skip the /etc/network/interfaces.d/sdn file version bump. This means > when nothing has been changed and dry-run is pressed, nothing will be > shown. > > Rename a few constants so that they don't clash with local variables. > > Signed-off-by: Gabriel Goller > --- > src/PVE/API2/Network/SDN.pm | 88 ++++++++++++++++++++++++++++++++ > src/PVE/Network/SDN.pm | 36 ++++++++----- > src/PVE/Network/SDN/Fabrics.pm | 10 +++- > src/PVE/Network/SDN/Zones.pm | 3 +- > src/test/debug/generateconfig.pl | 3 +- > src/test/run_test_zones.pl | 2 +- > 6 files changed, 124 insertions(+), 18 deletions(-) > > diff --git a/src/PVE/API2/Network/SDN.pm b/src/PVE/API2/Network/SDN.pm > index b35a588d391d..31159c27a3ac 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 Encode qw(decode); > +use JSON qw(from_json); > + > 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); > @@ -11,6 +14,7 @@ use PVE::RPCEnvironment; > use PVE::SafeSyslog; > use PVE::Tools qw(run_command extract_param); > use PVE::Network::SDN; > +use PVE::File; > > use PVE::API2::Network::SDN::Controllers; > use PVE::API2::Network::SDN::Vnets; > @@ -325,4 +329,88 @@ __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-run', > + path => 'dry-run', > + method => 'PUT', Should this really be a PUT method? GET would imo be better suited, since it doesn't have any side-effects. The node param could just be part of the path instead of sending it in the body. > + permissions => { > + check => ['perm', '/nodes/{node}', ['Sys.Modify']], > + }, > + description => > + "Dry-run the SDN apply action and return the difference between the current configuration and the pending configuration", > + protected => 1, > + proxyto => 'node', > + parameters => { > + additionalProperties => 0, > + properties => { > + node => get_standard_option('pve-node'), > + }, > + }, > + > + returns => { > + type => 'object', > + properties => { > + "frr-diff" => { > + type => 'string', > + description => > + 'The difference between the current and pending FRR configuration.', > + }, > + "interfaces-diff" => { > + type => 'string', > + description => > + 'The difference between the current and pending /etc/network/interfaces.d/sdn configuration.', > + }, > + }, > + }, > + code => sub { > + my ($param) = @_; > + > + my $cfg = PVE::Network::SDN::compile_running_cfg(); > + > + my $fabric_cfg = PVE::Network::SDN::Fabrics::config(0); > + my $frr_cfg = PVE::Network::SDN::generate_frr_raw_config($cfg, $fabric_cfg); > + my $new_cfg_frr = PVE::Network::SDN::Frr::raw_config_to_string($frr_cfg); > + > + # compile running config and skip version bump > + my $running_cfg = PVE::Network::SDN::compile_running_cfg(1); > + > + my $new_interfaces_cfg = > + PVE::Network::SDN::generate_raw_etc_network_config($running_cfg); > + > + my ($frr_tmp_filename, $frr_tmp_fh) = PVE::File::tempfile_contents($new_cfg_frr, 700); > + > + my ($interfaces_tmp_filename, $interfaces_tmp_fh) = > + PVE::File::tempfile_contents($new_interfaces_cfg, 700); > + > + my $return_value = {}; > + $return_value->{"frr-diff"} = get_diff('/etc/frr/frr.conf', $frr_tmp_filename); > + $return_value->{"interfaces-diff"} = > + get_diff('/etc/network/interfaces.d/sdn', $interfaces_tmp_filename); > + > + close($frr_tmp_fh); > + close($interfaces_tmp_fh); > + return $return_value; > + }, > +}); > + > 1; > diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm > index 037182bc5ae9..0bb36bf2d07d 100644 > --- a/src/PVE/Network/SDN.pm > +++ b/src/PVE/Network/SDN.pm > @@ -26,7 +26,7 @@ use PVE::Network::SDN::Dhcp; > use PVE::Network::SDN::Frr; > use PVE::Network::SDN::Fabrics; > > -my $running_cfg = "sdn/.running-config"; > +my $RUNNING_CFG_FILENAME = "sdn/.running-config"; > > my $parse_running_cfg = sub { > my ($filename, $raw) = @_; > @@ -49,7 +49,7 @@ my $write_running_cfg = sub { > return $json; > }; > > -PVE::Cluster::cfs_register_file($running_cfg, $parse_running_cfg, $write_running_cfg); > +PVE::Cluster::cfs_register_file($RUNNING_CFG_FILENAME, $parse_running_cfg, $write_running_cfg); > > my $LOCK_TOKEN_FILE = "/etc/pve/sdn/.lock"; > > @@ -105,7 +105,7 @@ sub status { > } > > sub running_config { > - return cfs_read_file($running_cfg); > + return cfs_read_file($RUNNING_CFG_FILENAME); > } > > =head3 running_config_has_frr(\%running_config) > @@ -187,13 +187,17 @@ sub pending_config { > > } > > -sub commit_config { > +sub compile_running_cfg { > + my ($skip_version_bump) = @_; > + $skip_version_bump = $skip_version_bump // 0; > > - my $cfg = cfs_read_file($running_cfg); > + my $cfg = cfs_read_file($RUNNING_CFG_FILENAME); > my $version = $cfg->{version}; > > if ($version) { > - $version++; > + if (!$skip_version_bump) { > + $version++; > + } > } else { > $version = 1; > } > @@ -219,7 +223,13 @@ sub commit_config { > fabrics => $fabrics, > }; > > - cfs_write_file($running_cfg, $cfg); > + return $cfg; > +} > + > +sub commit_config { > + my $cfg = compile_running_cfg(); > + > + cfs_write_file($RUNNING_CFG_FILENAME, $cfg); > } > > sub has_pending_changes { > @@ -342,20 +352,21 @@ sub get_local_vnets { > return $vnets; > } > > -=head3 generate_raw_etc_network_config() > +=head3 generate_raw_etc_network_config(\%running_cfg) > > Generate the /etc/network/interfaces.d/sdn config file from the Zones > -and Fabrics configuration and return it as a String. > +and Fabrics running configuration passed and return it as a String. > > =cut > > sub generate_raw_etc_network_config { > + my ($running_cfg) = @_; > my $raw_config = ""; > > - my $zone_config = PVE::Network::SDN::Zones::generate_etc_network_config(); > + my $zone_config = PVE::Network::SDN::Zones::generate_etc_network_config($running_cfg); > $raw_config .= $zone_config if $zone_config; > > - my $fabric_config = PVE::Network::SDN::Fabrics::generate_etc_network_config(); > + my $fabric_config = PVE::Network::SDN::Fabrics::generate_etc_network_config($running_cfg); > $raw_config .= $fabric_config if $fabric_config; > > return $raw_config; > @@ -396,7 +407,8 @@ interfaces files (/etc/network/interfaces.d/sdn). > =cut > > sub generate_etc_network_config { > - my $raw_config = PVE::Network::SDN::generate_raw_etc_network_config(); > + my $running_cfg = PVE::Network::SDN::running_config(); > + my $raw_config = PVE::Network::SDN::generate_raw_etc_network_config($running_cfg); > PVE::Network::SDN::write_raw_etc_network_config($raw_config); > } > > diff --git a/src/PVE/Network/SDN/Fabrics.pm b/src/PVE/Network/SDN/Fabrics.pm > index 3ca362a02660..9be7f0215bef 100644 > --- a/src/PVE/Network/SDN/Fabrics.pm > +++ b/src/PVE/Network/SDN/Fabrics.pm > @@ -102,10 +102,16 @@ sub get_frr_daemon_status { > } > > sub generate_etc_network_config { > + my ($running_cfg) = @_; > + > my $nodename = PVE::INotify::nodename(); > - my $fabric_config = PVE::Network::SDN::Fabrics::config(1); > + # if the config hasn't yet been applied after the introduction of > + # fabrics then the key does not exist in the running config so we > + # default to an empty hash > + my $fabrics_config = $running_cfg->{fabrics}->{ids} // {}; > + my $fabric_object = PVE::RS::SDN::Fabrics->running_config($fabrics_config); > > - return $fabric_config->get_interfaces_etc_network_config($nodename); > + return $fabric_object->get_interfaces_etc_network_config($nodename); > } > > sub node_properties { > diff --git a/src/PVE/Network/SDN/Zones.pm b/src/PVE/Network/SDN/Zones.pm > index 4da94580e07d..4c1468cf8ef8 100644 > --- a/src/PVE/Network/SDN/Zones.pm > +++ b/src/PVE/Network/SDN/Zones.pm > @@ -107,8 +107,7 @@ sub get_vnets { > } > > sub generate_etc_network_config { > - > - my $cfg = PVE::Network::SDN::running_config(); > + my ($cfg) = @_; > > my $version = $cfg->{version}; > my $vnet_cfg = $cfg->{vnets}; > diff --git a/src/test/debug/generateconfig.pl b/src/test/debug/generateconfig.pl > index 250db4368186..99a5d59cba72 100644 > --- a/src/test/debug/generateconfig.pl > +++ b/src/test/debug/generateconfig.pl > @@ -9,7 +9,8 @@ use PVE::Network::SDN::Controllers; > use Data::Dumper; > > PVE::Network::SDN::commit_config(); > -my $network_config = PVE::Network::SDN::Zones::generate_etc_network_config(); > +my $running_cfg = PVE::Network::SDN::running_config(); > +my $network_config = PVE::Network::SDN::Zones::generate_etc_network_config($running_cfg); > > PVE::Network::SDN::Zones::write_etc_network_config($network_config); > print "/etc/network/interfaces.d/sdn\n"; > diff --git a/src/test/run_test_zones.pl b/src/test/run_test_zones.pl > index 806225735e6b..8986c5c52c9f 100755 > --- a/src/test/run_test_zones.pl > +++ b/src/test/run_test_zones.pl > @@ -145,7 +145,7 @@ foreach my $test (@tests) { > my $name = $test; > my $expected = read_file("./$test/expected_sdn_interfaces"); > > - my $result = eval { PVE::Network::SDN::generate_raw_etc_network_config() }; > + my $result = eval { PVE::Network::SDN::generate_raw_etc_network_config($sdn_config) }; > > if (my $err = $@) { > diag("got unexpected error - $err");