Re: [pve-devel] [PATCH network] sdn: factor out frr config generation and writing

[Gabriel Goller <g.goller@proxmox.com>]

On 12.02.2025 12:17, Stefan Hanreich wrote:
>This still has some issues (see below), maybe we can look at it together
>next week (will be gone after today) and see if we can make some
>additional structural improvements to the whole controller / frr logic?

>There were also some regressions/bugs with FRR config generation while
>testing, so this series needs some work still.

We checked this together offliste, the patch doesn't change the output,
so no regressions.

>On 2/5/25 17:13, Gabriel Goller wrote:
>> diff --git a/src/PVE/Network/SDN/Controllers.pm b/src/PVE/Network/SDN/Controllers.pm
>> index fd7ad54ac38c..43f154b7338e 100644
>> --- a/src/PVE/Network/SDN/Controllers.pm
>> +++ b/src/PVE/Network/SDN/Controllers.pm
>> @@ -148,10 +149,22 @@ sub reload_controller {
>>
>>      return if !$controller_cfg;
>>
>> +    my $frr_reload = 0;
>> +
>>      foreach my $id (keys %{$controller_cfg->{ids}}) {
>>  	my $plugin_config = $controller_cfg->{ids}->{$id};
>> -	my $plugin = PVE::Network::SDN::Controllers::Plugin->lookup($plugin_config->{type});
>> -	$plugin->reload_controller();
>> +	my $type = $plugin_config->{type};
>> +	my @frr_types = ("bgp", "isis", "evpn");
>> +	if (grep {$type} @frr_types) {
>
>this doesn't work. you need:
>
>  grep {$_ eq $type} @frr_types
>
>(or what thomas said is even better - just for future reference)

Yup, my bad, will fix this.

>> +	    $frr_reload = 1;
>> +	} else {
>> +	    my $plugin = PVE::Network::SDN::Controllers::Plugin->lookup($type);
>> +	    $plugin->reload_controller();
>> +	}
>> +    }
>> +
>> +    if ($frr_reload) {
>> +	PVE::Network::SDN::Controllers::Frr::reload_controller();
>>      }
>>  }
>>
>> @@ -161,12 +174,22 @@ sub generate_controller_rawconfig {
>
>This actually never gets called (except in the tests, ..). It works, but
>still takes the old code path without going through the new FRR helper.
>Probably not the intention?

Yep, this function is only really called in the unit tests. With the
usual flow, we call write_controller_config, which resolves the plugin
and calls this function on the each plugin.

We could call the generate_controller_rawconfig function here though:
src/PVE/Network/SDN/Controllers.pm:212. So instead of getting the plugin
and calling the rawconfig implementations in write_controller_config,
just use this function.

>>      my $cfg = PVE::Network::SDN::running_config();
>>      my $controller_cfg = $cfg->{controllers};
>>      return if !$controller_cfg;
>> +    my $frr_generate = 0;
>>
>>      my $rawconfig = "";
>>      foreach my $id (keys %{$controller_cfg->{ids}}) {
>>  	my $plugin_config = $controller_cfg->{ids}->{$id};
>> -	my $plugin = PVE::Network::SDN::Controllers::Plugin->lookup($plugin_config->{type});
>> -	$rawconfig .= $plugin->generate_controller_rawconfig($plugin_config, $config);
>> +	my $type = $plugin_config->{type};
>> +	my @frr_types = ("bgp", "isis", "evpn");
>> +	if (grep {$type} @frr_types) {

>> @@ -323,6 +323,21 @@ sub vnet_update_hook {
>>      }
>>  }
>>
>> +sub reload_controller {
>> +    my ($class) = @_;
>> +    die "implemented in the Frr helper";
>> +}
>> +
>> +sub generate_controller_rawconfig {
>> +    my ($class, $config) = @_;
>> +    die "implemented in the Frr helper";
>> +}
>> +
>> +sub write_controller_config {
>> +    my ($class, $config) = @_;
>> +    die "implemented in the Frr helper";
>> +}
>> +
>>  1;
>
>Those methods are only needed in the Controllers, not Zones - they serve
>no purpose here afaict?

Yep, true, didn't check the directory I was in (zone vs controller) :)

Thanks for the review!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel