Re: [PATCH pve-network v3 9/9] api: add dry-run endpoint for sdn apply to preview changes

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

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 <g.goller@proxmox.com>
> ---
>  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");