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

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

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