[RFC PATCH network 04/16] sdn: ovn: sync the northbound database on reload

["Rémi PAETA" <remi.paeta@comaite.com>]

The logical switches of the vnets get created, updated and deleted when
the SDN configuration is applied. Since the northbound database is
shared by all nodes, this is done once for the whole cluster in the
reload API call, before the per-node reload workers get started.

Only objects tagged with the pve-sdn-* external_ids are touched, so an
OVN deployment can be shared with other consumers. A logical switch
named like a vnet but not managed by PVE is refused instead of taken
over.

Ports of guests that do not exist anymore get removed, as do all
managed objects of an OVN controller that was removed from the
configuration. Ports whose interface is not on their vnet in the guest
configuration anymore, because it got removed or moved to another
bridge, get removed too, unless they are still up: on hotplug, the port
gets created before the interface is written to the guest
configuration.

Signed-off-by: Rémi PAETA <remi.paeta@comaite.com>
---
 src/PVE/API2/Network/SDN.pm |   8 ++
 src/PVE/Network/SDN.pm      |  16 +++
 src/PVE/Network/SDN/Ovn.pm  | 268 ++++++++++++++++++++++++++++++++++++
 src/test/run_test_ovn.pl    | 206 +++++++++++++++++++++++++++
 4 files changed, 498 insertions(+)

diff --git a/src/PVE/API2/Network/SDN.pm b/src/PVE/API2/Network/SDN.pm
index e3c8d9d..c4522ad 100644
--- a/src/PVE/API2/Network/SDN.pm
+++ b/src/PVE/API2/Network/SDN.pm
@@ -318,9 +318,11 @@ __PACKAGE__->register_method({
 
         my $previous_config_has_frr;
         my $new_config_has_frr;
+        my $previous_running_config;
 
         PVE::Network::SDN::lock_sdn_config(
             sub {
+                $previous_running_config = PVE::Network::SDN::running_config();
                 $previous_config_has_frr = PVE::Network::SDN::running_config_has_frr();
                 PVE::Network::SDN::commit_config();
                 $new_config_has_frr = PVE::Network::SDN::running_config_has_frr();
@@ -338,6 +340,12 @@ __PACKAGE__->register_method({
         my $code = sub {
             $rpcenv->{type} = 'priv'; # to start tasks in background
             PVE::Cluster::check_cfs_quorum();
+
+            # the OVN northbound database is shared by all nodes, so it is
+            # synced once for the whole cluster
+            eval { PVE::Network::SDN::sync_ovn_northbound($previous_running_config) };
+            warn "syncing OVN northbound database failed - $@" if $@;
+
             my $nodelist = PVE::Cluster::get_nodelist();
             for my $node (@$nodelist) {
                 my $pid = eval { $create_reload_network_worker->($node, $regenerate_frr) };
diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm
index aeb9b4c..d601cd0 100644
--- a/src/PVE/Network/SDN.pm
+++ b/src/PVE/Network/SDN.pm
@@ -25,6 +25,7 @@ use PVE::Network::SDN::Subnets;
 use PVE::Network::SDN::Dhcp;
 use PVE::Network::SDN::Frr;
 use PVE::Network::SDN::Fabrics;
+use PVE::Network::SDN::Ovn;
 use PVE::Network::SDN::RouteMaps;
 use PVE::Network::SDN::PrefixLists;
 
@@ -503,6 +504,21 @@ sub generate_dhcp_config {
     PVE::Network::SDN::Dhcp::regenerate_config($reload);
 }
 
+=head3 sync_ovn_northbound(\%previous_running_config)
+
+Reconciles the OVN northbound databases with the current running
+configuration. C<\%previous_running_config> is the running configuration before
+the last commit, so that the objects of removed OVN controllers get cleaned up.
+
+=cut
+
+sub sync_ovn_northbound {
+    my ($previous_running_config) = @_;
+
+    my $running_config = PVE::Network::SDN::running_config();
+    PVE::Network::SDN::Ovn::sync_northbound($previous_running_config, $running_config);
+}
+
 sub encode_value {
     my ($type, $key, $value) = @_;
 
diff --git a/src/PVE/Network/SDN/Ovn.pm b/src/PVE/Network/SDN/Ovn.pm
index 93235e8..142477b 100644
--- a/src/PVE/Network/SDN/Ovn.pm
+++ b/src/PVE/Network/SDN/Ovn.pm
@@ -2,6 +2,9 @@ package PVE::Network::SDN::Ovn;
 
 use v5.36;
 
+use JSON;
+
+use PVE::Cluster;
 use PVE::Network;
 use PVE::Tools qw(run_command);
 
@@ -19,6 +22,180 @@ my $EXT_ZONE = 'pve-sdn-zone';
 my $EXT_VNET = 'pve-sdn-vnet';
 my $EXT_VMID = 'pve-sdn-vmid';
 
+=head3 ovn_controllers(\%running_cfg)
+
+Returns a hash of all controllers of type C<ovn> in C<\%running_cfg>, keyed by
+their ID.
+
+=cut
+
+sub ovn_controllers {
+    my ($running_cfg) = @_;
+
+    my $controllers = $running_cfg->{controllers}->{ids} // {};
+
+    return {
+        map { $_ => $controllers->{$_} }
+        grep { $controllers->{$_}->{type} eq 'ovn' } keys %$controllers
+    };
+}
+
+=head3 desired_state(\%running_cfg)
+
+Computes the northbound objects that should exist for C<\%running_cfg>, keyed by
+OVN controller ID. Every vnet of an C<ovn> zone maps to a logical switch of the
+same name.
+
+=cut
+
+sub desired_state {
+    my ($running_cfg) = @_;
+
+    my $zones = $running_cfg->{zones}->{ids} // {};
+    my $vnets = $running_cfg->{vnets}->{ids} // {};
+
+    my $state = {};
+    $state->{$_} = { switches => {} } for keys ovn_controllers($running_cfg)->%*;
+
+    for my $vnetid (sort keys %$vnets) {
+        my $zoneid = $vnets->{$vnetid}->{zone} or next;
+        my $zone = $zones->{$zoneid} or next;
+        next if $zone->{type} ne 'ovn';
+
+        my $controllerid = $zone->{controller};
+        next if !$controllerid || !$state->{$controllerid};
+
+        $state->{$controllerid}->{switches}->{$vnetid} = {
+            external_ids => {
+                $EXT_CONTROLLER => $controllerid,
+                $EXT_ZONE => $zoneid,
+                $EXT_VNET => $vnetid,
+            },
+        };
+    }
+
+    return $state;
+}
+
+=head3 sync_commands($controllerid, \%desired, \@switches, \@ports, \%vmlist, \%nics)
+
+Returns the list of C<ovn-nbctl> commands (each an array reference) that turn
+the current northbound content (C<\@switches> and C<\@ports>, as returned by
+C<list_rows>) into C<\%desired>, the entry of C<desired_state> for
+C<$controllerid>.
+
+Ports of guests that are not in C<\%vmlist> anymore get removed too.
+
+If C<\%nics> is given, as returned by C<guest_nics>, ports of guest
+interfaces that are not on their vnet anymore get removed too, unless they are
+still up, as a port gets created right before its interface is written to the
+guest config on hotplug.
+
+=cut
+
+sub sync_commands {
+    my ($controllerid, $desired, $switches, $ports, $vmlist, $nics) = @_;
+
+    my $wanted = $desired->{switches} // {};
+
+    my $managed = {};
+    for my $switch (@$switches) {
+        my $name = $switch->{name};
+        my $external_ids = $switch->{external_ids} // {};
+
+        if (($external_ids->{$EXT_CONTROLLER} // '') ne $controllerid) {
+            if ($wanted->{$name}) {
+                die "logical switch '$name' already exists in the OVN northbound database, but"
+                    . " is not managed by SDN controller '$controllerid'\n";
+            }
+            next;
+        }
+
+        $managed->{$name} = $external_ids;
+    }
+
+    my $commands = [];
+
+    for my $name (sort keys %$managed) {
+        push @$commands, ['ls-del', $name] if !$wanted->{$name};
+    }
+
+    for my $name (sort keys %$wanted) {
+        my $external_ids = $wanted->{$name}->{external_ids};
+
+        if (!$managed->{$name}) {
+            push @$commands, ['ls-add', $name];
+        } elsif (
+            !grep { ($managed->{$name}->{$_} // '') ne $external_ids->{$_} }
+            keys %$external_ids
+        ) {
+            next;
+        }
+
+        push @$commands,
+            [
+                'set',
+                'Logical_Switch',
+                $name,
+                map { "external_ids:$_=$external_ids->{$_}" } sort keys %$external_ids,
+            ];
+    }
+
+    for my $port (sort { $a->{name} cmp $b->{name} } @$ports) {
+        my $external_ids = $port->{external_ids} // {};
+        next if ($external_ids->{$EXT_CONTROLLER} // '') ne $controllerid;
+
+        # ports of deleted switches are removed together with their switch
+        my $vnetid = $external_ids->{$EXT_VNET};
+        next if defined($vnetid) && $managed->{$vnetid} && !$wanted->{$vnetid};
+
+        my $vmid = $external_ids->{$EXT_VMID};
+        if (defined($vmid) && !$vmlist->{$vmid}) {
+            push @$commands, ['lsp-del', $port->{name}];
+            next;
+        }
+
+        if (defined($vmid) && $nics && $port->{name} =~ m/^(?:tap|veth)\d+i(\d+)$/) {
+            my $bridge = $nics->{$vmid}->{"net$1"};
+            if ((!defined($bridge) || $bridge ne ($vnetid // '')) && !ovsdb_true($port->{up})) {
+                push @$commands, ['lsp-del', $port->{name}];
+                next;
+            }
+        }
+    }
+
+    return $commands;
+}
+
+# optional booleans are an empty set when unset
+sub ovsdb_true {
+    my ($value) = @_;
+
+    $value = $value->[0] if ref($value) eq 'ARRAY';
+    return $value ? 1 : 0;
+}
+
+=head3 guest_nics()
+
+Returns the bridge of every network interface of all guests, keyed by VMID and
+interface (C<net0>, C<net1>, ...).
+
+=cut
+
+sub guest_nics {
+    my $properties = PVE::Cluster::get_guest_config_properties([map { "net$_" } 0 .. 31]);
+
+    my $nics = {};
+    for my $vmid (keys %$properties) {
+        for my $net (keys $properties->{$vmid}->%*) {
+            my ($bridge) = $properties->{$vmid}->{$net} =~ m/(?:^|,)bridge=([^,]+)/;
+            $nics->{$vmid}->{$net} = $bridge if defined($bridge);
+        }
+    }
+
+    return $nics;
+}
+
 sub nbctl {
     my ($controller, @args) = @_;
 
@@ -47,6 +224,97 @@ sub nbctl_transaction {
     nbctl($controller, map { ('--', @$_) } @$commands);
 }
 
+# converts an OVSDB JSON value (RFC 7047) to plain perl data
+sub ovsdb_to_perl {
+    my ($value) = @_;
+
+    return $value if ref($value) ne 'ARRAY';
+
+    my ($type, $data) = @$value;
+    return { map { $_->[0] => $_->[1] } @$data } if $type eq 'map';
+    return [map { ovsdb_to_perl($_) } @$data] if $type eq 'set';
+    return $data; # uuid
+}
+
+=head3 list_rows($controller, $table, @columns)
+
+Returns all rows of the northbound C<$table> as list of hashes containing
+C<@columns>.
+
+=cut
+
+sub list_rows {
+    my ($controller, $table, @columns) = @_;
+
+    my $output = nbctl(
+        $controller,
+        '--format=json',
+        '--data=json',
+        '--columns=' . join(',', @columns),
+        'list',
+        $table,
+    );
+    my $result = decode_json($output);
+
+    my @headings = $result->{headings}->@*;
+
+    my $rows = [];
+    for my $data ($result->{data}->@*) {
+        push @$rows, { map { $headings[$_] => ovsdb_to_perl($data->[$_]) } 0 .. $#headings };
+    }
+
+    return $rows;
+}
+
+=head3 sync_northbound(\%previous_cfg, \%running_cfg)
+
+Reconciles the northbound database of every OVN controller with
+C<\%running_cfg>. Controllers only present in C<\%previous_cfg> get all their
+managed objects removed.
+
+Each controller is synced independently, errors are collected and raised at the
+end.
+
+=cut
+
+sub sync_northbound {
+    my ($previous_cfg, $running_cfg) = @_;
+
+    my $desired = desired_state($running_cfg);
+    my $controllers = ovn_controllers($running_cfg);
+
+    my $previous_controllers = ovn_controllers($previous_cfg // {});
+    for my $id (keys %$previous_controllers) {
+        next if $controllers->{$id};
+        $controllers->{$id} = $previous_controllers->{$id};
+        $desired->{$id} = { switches => {} };
+    }
+
+    return if !%$controllers;
+
+    my $vmlist = PVE::Cluster::get_vmlist()->{ids} // {};
+    my $guest_nics = guest_nics();
+
+    my @errors;
+    for my $id (sort keys %$controllers) {
+        my $controller = $controllers->{$id};
+
+        eval {
+            my $switches = list_rows($controller, 'Logical_Switch', 'name', 'external_ids');
+            my $ports = list_rows(
+                $controller, 'Logical_Switch_Port', 'name', 'external_ids', 'up',
+            );
+
+            my $commands =
+                sync_commands($id, $desired->{$id}, $switches, $ports, $vmlist, $guest_nics);
+            nbctl_transaction($controller, $commands);
+        };
+        push @errors, "OVN controller '$id': $@" if $@;
+    }
+
+    die join('', @errors) if @errors;
+}
+
 =head3 port_external_ids($controllerid, $zoneid, $vnetid, $iface)
 
 Returns the external_ids identifying the logical switch port of the guest
diff --git a/src/test/run_test_ovn.pl b/src/test/run_test_ovn.pl
index 411c60e..b12ca0b 100755
--- a/src/test/run_test_ovn.pl
+++ b/src/test/run_test_ovn.pl
@@ -5,7 +5,9 @@ use warnings;
 
 use lib qw(..);
 
+use JSON;
 use Test::More;
+use Test::MockModule;
 
 use PVE::Network::SDN;
 use PVE::Network::SDN::Ovn;
@@ -57,6 +59,163 @@ sub ext {
     return $ext;
 }
 
+sub set_switch {
+    my ($vnet, $zone) = @_;
+    return [
+        'set',
+        'Logical_Switch',
+        $vnet,
+        'external_ids:pve-sdn-controller=ovnctl',
+        "external_ids:pve-sdn-vnet=$vnet",
+        "external_ids:pve-sdn-zone=$zone",
+    ];
+}
+
+# desired_state
+
+my $desired = PVE::Network::SDN::Ovn::desired_state($running_cfg);
+is_deeply(
+    $desired,
+    {
+        ovnctl => {
+            switches => {
+                vnet1 => { external_ids => ext('zone1', 'vnet1') },
+                vnet2 => { external_ids => ext('zone2', 'vnet2') },
+            },
+        },
+    },
+    'desired_state: one logical switch per ovn vnet',
+);
+
+is_deeply(
+    PVE::Network::SDN::Ovn::desired_state({}), {}, 'desired_state: empty running config',
+);
+
+# sync_commands
+
+is_deeply(
+    PVE::Network::SDN::Ovn::sync_commands('ovnctl', $desired->{ovnctl}, [], [], {}),
+    [
+        ['ls-add', 'vnet1'],
+        set_switch('vnet1', 'zone1'),
+        ['ls-add', 'vnet2'],
+        set_switch('vnet2', 'zone2'),
+    ],
+    'sync_commands: create missing switches',
+);
+
+my $switches = [
+    { name => 'vnet1', external_ids => ext('zone1', 'vnet1') },
+    { name => 'vnet2', external_ids => ext('oldzone', 'vnet2') },
+    { name => 'oldvnet', external_ids => ext('zone1', 'oldvnet') },
+    { name => 'foreign', external_ids => { 'neutron:network_name' => 'foo' } },
+];
+my $ports = [
+    { name => 'tap100i0', external_ids => ext('zone1', 'vnet1', 100) },
+    { name => 'tap101i0', external_ids => ext('zone1', 'vnet1', 101) },
+    { name => 'tap102i0', external_ids => ext('zone1', 'oldvnet', 102) },
+    { name => 'manual', external_ids => ext('zone1', 'vnet1') },
+    { name => 'foreignport', external_ids => {} },
+];
+
+is_deeply(
+    PVE::Network::SDN::Ovn::sync_commands(
+        'ovnctl',
+        $desired->{ovnctl},
+        $switches,
+        $ports,
+        { 100 => {} },
+    ),
+    [['ls-del', 'oldvnet'], set_switch('vnet2', 'zone2'), ['lsp-del', 'tap101i0']],
+    'sync_commands: delete, update and garbage collect, leave the rest alone',
+);
+
+eval {
+    PVE::Network::SDN::Ovn::sync_commands(
+        'ovnctl',
+        $desired->{ovnctl},
+        [{ name => 'vnet1', external_ids => {} }],
+        [],
+        {},
+    );
+};
+like($@, qr/logical switch 'vnet1' already exists/, 'sync_commands: refuse unmanaged switch');
+
+is_deeply(
+    PVE::Network::SDN::Ovn::sync_commands(
+        'ovnctl',
+        { switches => {} },
+        [{ name => 'vnet1', external_ids => ext('zone1', 'vnet1') }],
+        [{ name => 'tap100i0', external_ids => ext('zone1', 'vnet1', 100) }],
+        {},
+    ),
+    [['ls-del', 'vnet1']],
+    'sync_commands: deleted switch takes its ports along',
+);
+
+# sync_northbound, with ovn-nbctl mocked
+
+my $nb_switches = {
+    ovnctl => [[
+        'vnet1',
+        [
+            'map',
+            [map { [$_, ext('zone1', 'vnet1')->{$_}] } sort keys ext('zone1', 'vnet1')->%*],
+        ],
+    ]],
+    gonectl => [['gone', ['map', [['pve-sdn-controller', 'gonectl']]]]],
+};
+
+my $nbctl_calls = [];
+my $ovn_module = Test::MockModule->new('PVE::Network::SDN::Ovn');
+$ovn_module->mock(
+    run_command => sub {
+        my ($cmd, %param) = @_;
+
+        die "unexpected command @$cmd\n" if $cmd->[0] ne $PVE::Network::SDN::Ovn::NBCTL;
+        my ($db) = $cmd->[1] =~ m/^--db=tcp:192\.168\.0\.(\d+)/;
+        my $id = $db == 1 ? 'ovnctl' : 'gonectl';
+
+        my @args = $cmd->@[3 .. $#$cmd];
+        if ($args[-2] eq 'list') {
+            my $data = $args[-1] eq 'Logical_Switch' ? $nb_switches->{$id} : [];
+            $param{outfunc}
+                ->(encode_json({ headings => ['name', 'external_ids'], data => $data }));
+        } else {
+            push @$nbctl_calls, [$id, @args];
+        }
+    },
+);
+my $cluster_module = Test::MockModule->new('PVE::Cluster');
+$cluster_module->mock(
+    get_vmlist => sub { return { ids => {} } },
+    get_guest_config_properties => sub { return {} },
+);
+
+my $previous_cfg = {
+    controllers => {
+        ids => {
+            gonectl => {
+                type => 'ovn',
+                'nb-connection' => 'tcp:192.168.0.9:6641',
+                'sb-connection' => 'tcp:192.168.0.9:6642',
+            },
+        },
+    },
+};
+
+PVE::Network::SDN::Ovn::sync_northbound($previous_cfg, $running_cfg);
+is_deeply(
+    $nbctl_calls,
+    [
+        ['gonectl', '--', 'ls-del', 'gone'],
+        ['ovnctl', '--', 'ls-add', 'vnet2', '--', set_switch('vnet2', 'zone2')->@*],
+    ],
+    'sync_northbound: one transaction per controller, removed controllers get cleaned up',
+);
+
+$ovn_module->unmock('run_command');
+
 # ports
 
 is_deeply(
@@ -130,6 +289,53 @@ is(
     'firewall_port_name: container',
 );
 
+# guest interfaces removed from their vnet
+
+is_deeply(
+    PVE::Network::SDN::Ovn::sync_commands(
+        'ovnctl',
+        $desired->{ovnctl},
+        [{ name => 'vnet1', external_ids => ext('zone1', 'vnet1') }],
+        [
+            {
+                name => 'tap100i0',
+                external_ids => ext('zone1', 'vnet1', 100),
+                up => ['set', []]->[1],
+            },
+            { name => 'tap100i1', external_ids => ext('zone1', 'vnet1', 100), up => 0 },
+            { name => 'tap100i2', external_ids => ext('zone1', 'vnet1', 100), up => 0 },
+            { name => 'tap100i3', external_ids => ext('zone1', 'vnet1', 100), up => 1 },
+        ],
+        { 100 => {} },
+        { 100 => { net0 => 'vnet1', net1 => 'vmbr0' } },
+    ),
+    [
+        ['ls-add', 'vnet2'],
+        set_switch('vnet2', 'zone2'),
+        ['lsp-del', 'tap100i1'],
+        ['lsp-del', 'tap100i2'],
+    ],
+    'sync_commands: remove ports of interfaces moved or removed, unless still up',
+);
+
+ok(!PVE::Network::SDN::Ovn::ovsdb_true([]), 'ovsdb_true: unset optional boolean');
+ok(PVE::Network::SDN::Ovn::ovsdb_true(JSON::true), 'ovsdb_true: true');
+
+$cluster_module->mock(
+    get_guest_config_properties => sub {
+        return {
+            100 => { net0 => 'virtio=BC:24:11:00:00:01,bridge=vnet1,firewall=1' },
+            101 => { net0 => 'name=eth0,bridge=vmbr0,hwaddr=BC:24:11:00:00:02,type=veth' },
+            102 => { net1 => 'virtio=BC:24:11:00:00:03' },
+        };
+    },
+);
+is_deeply(
+    PVE::Network::SDN::Ovn::guest_nics(),
+    { 100 => { net0 => 'vnet1' }, 101 => { net0 => 'vmbr0' } },
+    'guest_nics: bridges of the guest interfaces',
+);
+
 # unsupported options
 
 eval {
-- 
2.47.3