Hi Frode,

 

First of all, thanks for the patch set and for starting works with OVN native

BGP integration!

 

I’ve got some questions/comments, please see inline.

 

regards,

Vladislav Odintsov




On 19 Jul 2024, at 09:10, Frode Nordahl <fnord...@ubuntu.com> wrote:

Add three new options for Logical Router Ports that control
ovn-controller route exchange for NAT addresses and LB VIPs.

Load Balancers already have structured data in the Southbound
database which the ovn-controller can use directly.

NAT addresses are however currently only expressed as specialized
rules in the Port_Binding table nat_addresses column on LSP peer
records for LRPs, used for (G)ARP processing, as well as
logical flow rules for OpenFlow processing.

Options considered for how to redistribute these addresses to the
ovn-controllers in a structured way:
* Introduce even more conditional processing of the lsp
 nat_addresses column.
* Parse ct_dnat records in the Logical_Flow table.
* Add column to the Port_Binding table.
* Copy the Northbound NAT table over to the Southbound database,
 similar to what is done with Load Balancers.
* Populate Port_Binding table nat_addresses column on LRPs peer
 record (the proposed approach).

The Port_Binding table LRP peer records nat_addresses column is
currently unused, populate it with NAT addresses for route
exchange, when the redistribute-nat LRP option is set to 'true'.

The options are only processed for gateway routers.

Signed-off-by: Frode Nordahl <fnord...@ubuntu.com>
---
controller/pinctrl.c |  8 +++++--
northd/northd.c      | 22 +++++++++++++++++++
ovn-nb.xml           | 45 ++++++++++++++++++++++++++++++++++++++
ovn-sb.xml           | 51 +++++++++++++++++++++++++++++++++++++++++++-
tests/ovn-northd.at  | 35 ++++++++++++++++++++++++++++++
5 files changed, 158 insertions(+), 3 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 708240e24..d9ef97ce1 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -6428,11 +6428,15 @@ get_nat_addresses_and_keys(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
        const struct sbrec_port_binding *pb;

        pb = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port);
-        if (!pb) {
+        if (!pb || !pb->datapath) {
            continue;
        }

-        if (pb->n_nat_addresses) {
+        /* We only want to consider nat_addresses column for LS datapaths. */
+        const char *logical_switch = smap_get(&pb->datapath->external_ids,
+                                              "logical-switch");
+
+        if (pb->n_nat_addresses && logical_switch) {
            for (int i = 0; i < pb->n_nat_addresses; i++) {
                consider_nat_address(sbrec_port_binding_by_name,
                                     pb->nat_addresses[i], pb,
diff --git a/northd/northd.c b/northd/northd.c
index 6898daa00..10d78b561 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3939,6 +3939,28 @@ sync_pb_for_lrp(struct ovn_port *op,
        }
        if (chassis_name) {
            smap_add(&new, "l3gateway-chassis", chassis_name);
+            if (smap_get_bool(&op->nbrp->options, "maintain-vrf", false)) {
+                smap_add(&new, "maintain-vrf", "true");
+            }
+            if (smap_get_bool(&op->nbrp->options,
+                              "redistribute-nat", false)) {
+                smap_add(&new, "redistribute-nat", "true");
+
+                size_t n_nats = 0;
+                char **nats = NULL;
+                nats = get_nat_addresses(op, &n_nats, false, false, NULL);
+                sbrec_port_binding_set_nat_addresses(op->sb,
+                                                     (const char **) nats,
+                                                     n_nats);
+                for (size_t i = 0; i < n_nats; i++) {
+                    free(nats[i]);
+                }
+                free(nats);
+            }
+            if (smap_get_bool(&op->nbrp->options,
+                              "redistribute-lb-vips", false)) {
+                smap_add(&new, "redistribute-lb-vips", "true");
+            }
        }
    }

diff --git a/ovn-nb.xml b/ovn-nb.xml
index 9552534f6..7a5c1be57 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -3451,6 +3451,51 @@ or
          <ref column="options" key="gateway_mtu"/> option.
        </p>
      </column>
+
+      <column name="options" key="redistribute-lb-vips">
+        <p>
+          When configured the <code>ovn-controller</code> will redistribute
+          host routes to Load Balancer VIPs that are local to its chassis and
+          associated with the LR datapath.

 

Though I'm not a native English speaker, it seems that this sentence should be

adjusted because it is unclear what "host routes" are and why they should be

redistributed TO Load Balancer VIPs. Also, datapath is an internal term, should

we use just "Logical Router" instead?

 

Also, I’d propose to replace "redistribute" term with one of the next terms:

- announce

- advertise

- install

- export

since we just install static route to kernel route table and don't do any work

which is well-known with "redistribution" term from netops.  Personally I'd

vote for the "advertise", but "export" is a good option too.

IIUC the target architecture, the routing daemon (FRR or other) is expected to

make redistribution of static routes installed by the ovn-controller, right?

It will also be good not to shuffle terms here.

 

The final sentence could be like this (proposal):

 

"When configured the ovn-controller will advertise routes for Load Balancer

VIPs which are local to its chassis and associated with the Logical Router."

+
+          This option only works on gateway routers (routers that have
+          <ref column="options" key="chassis" table="Logical_Router"/> set)

 

From the last patch of this series I see that also ha_chassis_group is

supported, so DGPs also trigger this functionality.  Could you please note this

here in man page?

+          when OVN is built on a system with Netlink support.
+        </p>
+      </column>
+
+      <column name="options" key="redistribute-nat">
+        <p>
+          When configured the <code>ovn-controller</code> will redistribute
+          host routes to NAT records associated with logical ports local to
+          its chassis in relateion to the LR datapath.
+
+          This option only works on gateway routers (routers that have
+          <ref column="options" key="chassis" table="Logical_Router"/> set)
+          when OVN is built on a system with Netlink support.
+        </p>
+      </column>

 

Do we need a special configuration knob to selectively advertise either LB

addresses or NAT addresses?  Is there a usecase where it is possible and usable

to use both L2 and L3 methods of connecting OVN to outside world?

+
+      <column name="options" key="maintain-vrf">
+        <p>
+          When configured the <code>ovn-controller</code> will maintain a VRF
+          in the system for exporting host routes to NAT addresses (see
+          <ref column="options" key="redistribute-nat"/> option) and Load
+          Balancer VIPs (see <ref column="options" key="redistribute-lb-vips"/>
+          option).
+
+          Route table ID to use can be influenced by setting the
+          <ref column="options" key="requested-tnl-key"
+               table="Logical_Router"/> option in the
+          <ref table="Logical_Router"/> table. If not set,
+          <code>ovn-northd</code> will allocate one.
+
+          This option only works on gateway routers (routers that have
+          <ref column="options" key="chassis" table="Logical_Router"/> set)
+          when OVN is built on a system with Netlink support.
+        </p>
+      </column>
+
    </group>

 

In our scenario we'd like to advertise NAT and LB addresses from multiple LRs

into a single "public" VRF, which is configured by configuration management

tools like puppet or ansible or others.  In current implementation I see that a

single VRF must exist (or be created by ovn-controller) for each local datapath

(LR in other words).  Am I right?

If yes, is it possible to provide configuration options to supply either target

VRF name (preferred) or route table ID?  And then group all DPs routes to

resources (NAT and LB addresses) and then install all of them into that table?


    <group title="Attachment">
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 73a1be5ed..1e27933ee 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -3613,14 +3613,63 @@ tcp.flags = RST;
        The <code>chassis</code> in which the port resides.
      </column>

+      <column name="options" key="redistribute-lb-vips">
+        <p>
+          The value is copied from the column
+          <ref table="Logical_Router_Port" column="options"
+               db="OVN_Northbound"/> and when configured the
+          <code>ovn-controller</code> will redistribute host routes to Load
+          Balancer VIPs that are local to its chassis and associated with the
+          LR datapath.
+        </p>
+      </column>
+
+      <column name="options" key="redistribute-nat">
+        <p>
+          The value is copied from the column
+          <ref table="Logical_Router_Port" column="options"
+               db="OVN_Northbound"/> and when configured the
+          <code>ovn-controller</code> will redistribute host routes to
+          <ref table="NAT" db="OVN_Northbound"/> records associated with
+          logical ports local to this chassis in relateion to the LR datapath.
+        </p>
+      </column>
+
+      <column name="options" key="maintain-vrf">
+        <p>
+          The value is copied from the column
+          <ref table="Logical_Router_Port" column="options"
+               db="OVN_Northbound"/> and when configured the
+          <code>ovn-controller</code> will maintain a VRF in the system for
+          exporting host routes to NAT addresses (see
+          <ref column="options" key="redistribute-nat"/> option) and Load
+          Balancer VIPs (see <ref column="options" key="redistribute-lb-vips"/>
+          option).
+        </p>
+      </column>
+
      <column name="nat_addresses">
        MAC address of the <code>l3gateway</code> port followed by a list of
-        SNAT and DNAT external IP addresses.  This is used to send gratuitous
+        SNAT and DNAT external IP addresses.
+
+        Both sides of a LRP-LSP peer relationship have records in the
+        <ref table="Port_Binding"/> table.
+
+        When the record is associated with a
+        <ref table="Datapath_Binding" column="external_ids"
+             key="logical-switch"/> record, this is used to send gratuitous
        ARPs for SNAT and DNAT external IP addresses via <code>localnet</code>.
        Example: <code>80:fa:5b:06:72:b7 158.36.44.22 158.36.44.24</code>.
        This would result in generation of gratuitous ARPs for IP addresses
        158.36.44.22 and 158.36.44.24 with a MAC address of 80:fa:5b:06:72:b7.
        This is used in OVS version 2.8 and later versions.
+
+        When the record is associated with a
+        <ref table="Datapath_Binding" column="external_ids"
+             key="logical-router"/> record, this is used to redistribute host
+        routes to <ref table="NAT" db="OVN_Northbound"/> records associated
+        with logical ports local to this chassis in relateion to the LR
+        datapath.
      </column>
    </group>

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index a389d1988..57422af70 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -12721,3 +12721,38 @@ AT_CHECK([ovn-sbctl dump-flows lr | grep lr_in_dnat | 
ovn_strip_lflows], [0], [d

AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([check route-exchange redistribute-nat option])
+ovn_start
+
+ovn-nbctl lr-add R1
+ovn-nbctl set Logical_Router R1 options:chassis=hv1
+ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
+
+ovn-nbctl ls-add S1
+ovn-nbctl lsp-add S1 S1-R1
+ovn-nbctl lsp-set-type S1-R1 router
+ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
+ovn-nbctl --wait=sb lsp-set-options S1-R1 \
+    router-port=R1-S1 \
+    nat-addresses="router"
+
+ovn-nbctl lr-nat-add R1 snat 172.16.1.1 10.0.0.0/24
+ovn-nbctl lr-nat-add R1 dnat 172.16.1.2 10.0.0.1
+ovn-nbctl --wait=sb sync
+
+AT_CHECK([dnl
+ovn-sbctl get Port_Binding S1-R1 nat_addresses | grep -q 172.16.1.2], [0])
+AT_CHECK([dnl
+ovn-sbctl get Port_Binding R1-S1 nat_addresses | grep -q 172.16.1.2], [1])
+
+ovn-nbctl lrp-set-options R1-S1 redistribute-nat=true
+
+AT_CHECK([dnl
+ovn-sbctl get Port_Binding S1-R1 nat_addresses | grep -q 172.16.1.2], [0])
+AT_CHECK([dnl
+ovn-sbctl get Port_Binding R1-S1 nat_addresses | grep -q 172.16.1.2], [0])
+
+AT_CLEANUP
+])
-- 
2.45.2

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to