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
