[pve-devel] [PATCH pve-network 08/17] sdn: frr: add daemon status to frr helper

[Stefan Hanreich]

On 4/2/25 12:41, Fabian Grünbichler wrote:
> On March 28, 2025 6:13 pm, Gabriel Goller wrote:
>> From: Stefan Hanreich <s.hanreich at proxmox.com>
>>
>> Add functions that allow reading and manipulating values in the
>> /etc/frr/daemons file. We need this for en/disabling daemons depending
>> on which fabric types are configured. We only enable daemons which are
>> required for the configured fabrics. If a daemon is enabled but a
>> fabric gets deleted, we disable them.
>>
>> The helper works by iterating over the lines of the daemons file from
>> FRR, parsing the key and checking if the key is managed by the SDN
>> configuration, then sets it. As a safeguard, keys that can be changed
>> by SDN have to be explicitly configured in the respective hash of the
>> Frr module.
>>
>> Signed-off-by: Stefan Hanreich <s.hanreich at proxmox.com>
>> Co-authored-by: Gabriel Goller <g.goller at proxmox.com>
>> Signed-off-by: Gabriel Goller <g.goller at proxmox.com>
>> ---
>>  src/PVE/Network/SDN.pm         | 15 +++++++++++
>>  src/PVE/Network/SDN/Fabrics.pm | 18 +++++++++++++
>>  src/PVE/Network/SDN/Frr.pm     | 49 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 82 insertions(+)
>>
>> diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm
>> index b777a098a987..a0b61275e10b 100644
>> --- a/src/PVE/Network/SDN.pm
>> +++ b/src/PVE/Network/SDN.pm
>> @@ -258,12 +258,27 @@ sub generate_frr_raw_config {
>>      return $raw_config;
>>  }
>>  
>> +=head3 get_frr_daemon_status(\%running_config, \%fabric_config)
>> +
>> +Returns a hash that indicates the status of the FRR daemons managed by SDN.
>> +
>> +=cut
>> +
>> +sub get_frr_daemon_status {
>> +    my ($running_config, $fabric_config) = @_;
>> +
>> +    return PVE::Network::SDN::Fabrics::get_frr_daemon_status($fabric_config);
> 
> this takes but doesn't use the $running_config.. is this intentional?
> 

Yes, the idea was that we move handling of BGP and other daemons into
this function in the future as well (where we need the running config)
and this preserves the order of function parameters that is used in
other functions. I guess it's a bit premature and it wouldn't be hard to
change when tacking this on, so no hard feelings against dropping this.

>> +}
>> +
>>  sub generate_frr_config {
>>      my ($reload) = @_;
>>  
>>      my $running_config = PVE::Network::SDN::running_config();
>>      my $fabric_config = PVE::Network::SDN::Fabrics::config(1);
>>  
>> +    my $daemon_status = PVE::Network::SDN::get_frr_daemon_status($running_config, $fabric_config);
> 
> the getter has a top-level wrapper
> 
>> +    PVE::Network::SDN::Frr::set_daemon_status($daemon_status);
> 
> but the setter doesn't? seems inconsistent ;)
> 

The idea here was that the SDN module is responsible for generating the
daemon status, but the FRR module is responsible for actually writing
it. I have nothing against adding a respective helper to the SDN module
though.

>> +
>>      my $raw_config = PVE::Network::SDN::generate_frr_raw_config($running_config, $fabric_config);
>>      PVE::Network::SDN::Frr::write_raw_config($raw_config);
>>  
>> diff --git a/src/PVE/Network/SDN/Fabrics.pm b/src/PVE/Network/SDN/Fabrics.pm
>> index 6e3fa5234a5b..d716c68feac4 100644
>> --- a/src/PVE/Network/SDN/Fabrics.pm
>> +++ b/src/PVE/Network/SDN/Fabrics.pm
>> @@ -69,6 +69,24 @@ sub config_for_protocol {
>>      return $module->config($section_config);
>>  }
>>  
>> +sub get_frr_daemon_status {
>> +    my ($fabric_config) = @_;
>> +
>> +    my $daemon_status = {};
>> +    my $nodename = PVE::INotify::nodename();
>> +
>> +    for my $protocol (sort keys %$fabric_config) {
>> +	my $config = $fabric_config->{$protocol};
> 
> this could iterate over the values instead? ;)

will do

>> +	my $enabled_daemons = $config->enabled_daemons($nodename);
>> +
>> +	for my $daemon (@$enabled_daemons) {
>> +	    $daemon_status->{$daemon} = 1;
>> +	}
>> +    }
>> +
>> +    return $daemon_status;
>> +}
>> +
>>  sub generate_frr_raw_config {
>>      my ($fabric_config) = @_;
>>  
>> diff --git a/src/PVE/Network/SDN/Frr.pm b/src/PVE/Network/SDN/Frr.pm
>> index bb0f197d8dea..9ae302a9c25f 100644
>> --- a/src/PVE/Network/SDN/Frr.pm
>> +++ b/src/PVE/Network/SDN/Frr.pm
>> @@ -77,6 +77,55 @@ sub reload {
>>      }
>>  }
>>  
>> +my $SDN_DAEMONS_DEFAULT = {
>> +    ospfd => 0,
>> +    fabricd => 0,
>> +};
>> +
>> +=head3 set_daemon_status(\%daemons, $set_default)
>> +
>> +Sets the status of all daemons supplied in C<\%daemons>. This only works for
>> +daemons managed by SDN, as indicated in the C<$SDN_DAEMONS_DEFAULT> constant. If
>> +a daemon is supplied that isn't managed by SDN then this command will fail. If
>> +C<$set_default> is set, then additionally all sdn-managed daemons that are
>> +missing in C<\%daemons> are reset to their default value.
>> +
>> +=cut
>> +
>> +sub set_daemon_status {
>> +    my ($daemon_status, $set_default) = @_;
>> +
>> +    for my $daemon (keys %$daemon_status) {
>> +	die "$daemon is not SDN managed" if !defined $SDN_DAEMONS_DEFAULT->{$daemon};
>> +    }
>> +
>> +    my $daemons_file = "/etc/frr/daemons";
>> +
>> +    my $old_config = PVE::Tools::file_get_contents($daemons_file);
>> +    my $new_config = "";
>> +
>> +    my @lines = split(/\n/, $old_config);
>> +
>> +    for my $line (@lines) {
> 
> so we have three cases here
> - line contains one of our daemons as key (=> set status)
> - line contains something else as key (=> keep line as is)
> - line contains no key but something else entirely (=> keep line as is)
> 
> I think this could be structured so that it is a bit more readable/easy
> to follow along..
> 
> first, fill in the defaults if needed:
> 
> if ($set_default) {
>   for my $daemon (keys %$SDN_DAEMONS_DEFAULT) {
>     $daemon_status->{$daemon} = $SDN_DAEMONS_DEFAULT->{$daemon}
>       if !defined($daemon_status->{$daemon});
>   }
> }
> 
> and then simply override lines as needed:
> 
> for my $line (@lines) {
> 	if ($line =~ m/^([a-z_]+)=/) {
> 	   my $key = $1;
> 	   my $status = $daemon_status->{$key};
> 
> 	   if (defined($status)) {
>         my $value = status ? "yes" : "no"; 
>         $line = "$key=$value";
> 	   }
> 	}
> 
> 	$new_config .= "$line\n";
> }
>

makes sense, will convert!