On 12/19/24 9:43 AM, [email protected] wrote:
> On Wed, 2024-12-18 at 17:09 +0100, Felix Huettner wrote:
>> On Wed, Dec 18, 2024 at 04:30:32PM +0100, Dumitru Ceara wrote:
>>> On 12/18/24 3:50 PM, [email protected] wrote:
>>>> Hi Felix,
>>>
>>> Hi Martin, Felix,
>>>
>>>> I ran into an issue of advertising routes on gateway router ports
>>>> here,
>>>> I was wondering if I just misunderstood your intentions.
>>>>
>>>> On Tue, 2024-12-17 at 15:03 +0100, Felix Huettner via dev wrote:
>>>>> this engine node determines the routes that the ovn-controller
>>>>> should
>>>>> export.
>>>>>
>>>>> Signed-off-by: Felix Huettner <[email protected]>
>>>>> ---
>>>>>  controller/automake.mk      |   4 +-
>>>>>  controller/ovn-controller.c | 191
>>>>> +++++++++++++++++++++++++++++++++++-
>>>>>  controller/route.c          | 189
>>>>> +++++++++++++++++++++++++++++++++++
>>>>>  controller/route.h          |  73 ++++++++++++++
>>>>>  tests/automake.mk           |   1 +
>>>>>  5 files changed, 456 insertions(+), 2 deletions(-)
>>>>>  create mode 100644 controller/route.c
>>>>>  create mode 100644 controller/route.h
>>>>>
>>>>> diff --git a/controller/automake.mk b/controller/automake.mk
>>>>> index bb0bf2d33..a6a2c517a 100644
>>>>> --- a/controller/automake.mk
>>>>> +++ b/controller/automake.mk
>>>>> @@ -51,7 +51,9 @@ controller_ovn_controller_SOURCES = \
>>>>>   controller/ct-zone.h \
>>>>>   controller/ct-zone.c \
>>>>>   controller/ovn-dns.c \
>>>>> - controller/ovn-dns.h
>>>>> + controller/ovn-dns.h \
>>>>> + controller/route.h \
>>>>> + controller/route.c
>>>>>  
>>>>>  controller_ovn_controller_LDADD = lib/libovn.la
>>>>> $(OVS_LIBDIR)/libopenvswitch.la
>>>>>  man_MANS += controller/ovn-controller.8
>>>>> diff --git a/controller/ovn-controller.c b/controller/ovn-
>>>>> controller.c
>>>>> index 157def2a3..df96d77e3 100644
>>>>> --- a/controller/ovn-controller.c
>>>>> +++ b/controller/ovn-controller.c
>>>>> @@ -88,6 +88,7 @@
>>>>>  #include "lib/dns-resolve.h"
>>>>>  #include "ct-zone.h"
>>>>>  #include "ovn-dns.h"
>>>>> +#include "route.h"
>>>>>  
>>>>>  VLOG_DEFINE_THIS_MODULE(main);
>>>>>  
>>>>> @@ -864,7 +865,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl
>>>>> *ovs_idl)
>>>>>      SB_NODE(fdb, "fdb") \
>>>>>      SB_NODE(meter, "meter") \
>>>>>      SB_NODE(static_mac_binding, "static_mac_binding") \
>>>>> -    SB_NODE(chassis_template_var, "chassis_template_var")
>>>>> +    SB_NODE(chassis_template_var, "chassis_template_var") \
>>>>> +    SB_NODE(advertised_route, "advertised_route")
>>>>>  
>>>>>  enum sb_engine_node {
>>>>>  #define SB_NODE(NAME, NAME_STR) SB_##NAME,
>>>>> @@ -4804,6 +4806,175 @@
>>>>> pflow_lflow_output_sb_chassis_handler(struct
>>>>> engine_node *node,
>>>>>      return true;
>>>>>  }
>>>>>  
>>>>> +struct ed_type_route {
>>>>> +    /* Contains struct tracked_datapath entries for local
>>>>> datapaths
>>>>> subject to
>>>>> +     * route exchange. */
>>>>> +    struct hmap tracked_route_datapaths;
>>>>> +    /* Contains struct advertise_datapath_entry */
>>>>> +    struct hmap announce_routes;
>>>>> +};
>>>>> +
>>>>> +static void
>>>>> +en_route_run(struct engine_node *node, void *data)
>>>>> +{
>>>>> +    struct ed_type_route *re_data = data;
>>>>> +    route_cleanup(&re_data->announce_routes);
>>>>> +
>>>>> +    const struct ovsrec_open_vswitch_table *ovs_table =
>>>>> +        EN_OVSDB_GET(engine_get_input("OVS_open_vswitch",
>>>>> node));
>>>>> +    const char *chassis_id = get_ovs_chassis_id(ovs_table);
>>>>> +    ovs_assert(chassis_id);
>>>>> +
>>>>> +    struct ovsdb_idl_index *sbrec_chassis_by_name =
>>>>> +        engine_ovsdb_node_get_index(
>>>>> +                engine_get_input("SB_chassis", node),
>>>>> +                "name");
>>>>> +    const struct sbrec_chassis *chassis
>>>>> +        = chassis_lookup_by_name(sbrec_chassis_by_name,
>>>>> chassis_id);
>>>>> +    ovs_assert(chassis);
>>>>> +
>>>>> +    struct ovsdb_idl_index *sbrec_port_binding_by_name =
>>>>> +        engine_ovsdb_node_get_index(
>>>>> +                engine_get_input("SB_port_binding", node),
>>>>> +                "name");
>>>>> +    struct ed_type_runtime_data *rt_data =
>>>>> +        engine_get_input_data("runtime_data", node);
>>>>> +
>>>>> +    struct ovsdb_idl_index *sbrec_advertised_route_by_datapath
>>>>> =
>>>>> +        engine_ovsdb_node_get_index(
>>>>> +            engine_get_input("SB_advertised_route", node),
>>>>> +            "datapath");
>>>>> +
>>>>> +    struct route_ctx_in r_ctx_in = {
>>>>> +        .ovnsb_idl_txn = engine_get_context()->ovnsb_idl_txn,
>>>>> +        .sbrec_port_binding_by_name =
>>>>> sbrec_port_binding_by_name,
>>>>> +        .chassis = chassis,
>>>>> +        .active_tunnels = &rt_data->active_tunnels,
>>>>> +        .local_datapaths = &rt_data->local_datapaths,
>>>>> +        .local_lports = &rt_data->local_lports,
>>>>> +        .sbrec_advertised_route_by_datapath =
>>>>> +            sbrec_advertised_route_by_datapath,
>>>>> +    };
>>>>> +
>>>>> +    struct route_ctx_out r_ctx_out = {
>>>>> +        .tracked_re_datapaths = &re_data-
>>>>>> tracked_route_datapaths,
>>>>> +        .announce_routes = &re_data->announce_routes,
>>>>> +    };
>>>>> +
>>>>> +    route_run(&r_ctx_in, &r_ctx_out);
>>>>> +
>>>>> +    engine_set_node_state(node, EN_UPDATED);
>>>>> +}
>>>>> +
>>>>> +
>>>>> +static void *
>>>>> +en_route_init(struct engine_node *node OVS_UNUSED,
>>>>> +                       struct engine_arg *arg OVS_UNUSED)
>>>>> +{
>>>>> +    struct ed_type_route *data = xzalloc(sizeof *data);
>>>>> +
>>>>> +    hmap_init(&data->tracked_route_datapaths);
>>>>> +    hmap_init(&data->announce_routes);
>>>>> +
>>>>> +    return data;
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +en_route_cleanup(void *data)
>>>>> +{
>>>>> +    struct ed_type_route *re_data = data;
>>>>> +
>>>>> +    tracked_datapaths_destroy(&re_data-
>>>>>> tracked_route_datapaths);
>>>>> +    route_cleanup(&re_data->announce_routes);
>>>>> +    hmap_destroy(&re_data->announce_routes);
>>>>> +}
>>>>> +
>>>>> +static bool
>>>>> +route_runtime_data_handler(struct engine_node *node, void
>>>>> *data)
>>>>> +{
>>>>> +    struct ed_type_route *re_data = data;
>>>>> +    struct ed_type_runtime_data *rt_data =
>>>>> +        engine_get_input_data("runtime_data", node);
>>>>> +
>>>>> +    if (!rt_data->tracked) {
>>>>> +        return false;
>>>>> +    }
>>>>> +
>>>>> +    struct tracked_datapath *t_dp;
>>>>> +    HMAP_FOR_EACH (t_dp, node, &rt_data->tracked_dp_bindings)
>>>>> {
>>>>> +        struct tracked_datapath *re_t_dp =
>>>>> +            tracked_datapath_find(&re_data-
>>>>>> tracked_route_datapaths,
>>>>> t_dp->dp);
>>>>> +
>>>>> +        if (re_t_dp) {
>>>>> +            /* Until we get I-P support for route exchange we
>>>>> need
>>>>> to request
>>>>> +             * recompute. */
>>>>> +            return false;
>>>>> +        }
>>>>> +
>>>>> +        struct shash_node *shash_node;
>>>>> +        SHASH_FOR_EACH (shash_node, &t_dp->lports) {
>>>>> +            struct tracked_lport *lport = shash_node->data;
>>>>> +            if (route_exchange_relevant_port(lport->pb)) {
>>>>> +                /* Until we get I-P support for route exchange
>>>>> we
>>>>> need to
>>>>> +                 * request recompute. */
>>>>> +                return false;
>>>>> +            }
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return true;
>>>>> +}
>>>>> +
>>>>> +static bool
>>>>> +route_sb_port_binding_data_handler(struct engine_node *node,
>>>>> void
>>>>> *data)
>>>>> +{
>>>>> +    struct ed_type_route *re_data = data;
>>>>> +    const struct sbrec_port_binding_table *pb_table =
>>>>> +        EN_OVSDB_GET(engine_get_input("SB_port_binding",
>>>>> node));
>>>>> +
>>>>> +    const struct sbrec_port_binding *sbrec_pb;
>>>>> +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_pb,
>>>>> pb_table) {
>>>>> +        struct tracked_datapath *re_t_dp =
>>>>> +            tracked_datapath_find(&re_data-
>>>>>> tracked_route_datapaths,
>>>>> +                                  sbrec_pb->datapath);
>>>>> +        if (re_t_dp) {
>>>>> +            /* Until we get I-P support for route exchange we
>>>>> need
>>>>> to request
>>>>> +             * recompute. */
>>>>> +            return false;
>>>>> +        }
>>>>> +
>>>>> +        if (route_exchange_relevant_port(sbrec_pb)) {
>>>>> +            /* Until we get I-P support for route exchange we
>>>>> need
>>>>> to
>>>>> +             * request recompute. */
>>>>> +            return false;
>>>>> +        }
>>>>> +
>>>>> +    }
>>>>> +    return true;
>>>>> +}
>>>>> +
>>>>> +static bool
>>>>> +route_sb_advertised_route_data_handler(struct engine_node
>>>>> *node,
>>>>> void *data)
>>>>> +{
>>>>> +    struct ed_type_route *re_data = data;
>>>>> +    const struct sbrec_advertised_route_table
>>>>> *advertised_route_table =
>>>>> +        EN_OVSDB_GET(engine_get_input("SB_advertised_route",
>>>>> node));
>>>>> +
>>>>> +    const struct sbrec_advertised_route *sbrec_route;
>>>>> +    SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH_TRACKED
>>>>> (sbrec_route,
>>>>> +                                                  
>>>>> advertised_route_table) {
>>>>> +        struct tracked_datapath *re_t_dp =
>>>>> +            tracked_datapath_find(&re_data-
>>>>>> tracked_route_datapaths,
>>>>> +                                  sbrec_route->datapath);
>>>>> +        if (re_t_dp) {
>>>>> +            /* Until we get I-P support for route exchange we
>>>>> need
>>>>> to request
>>>>> +             * recompute. */
>>>>> +            return false;
>>>>> +        }
>>>>> +    }
>>>>> +    return true;
>>>>> +}
>>>>> +
>>>>>  /* Returns false if the northd internal version stored in
>>>>> SB_Global
>>>>>   * and ovn-controller internal version don't match.
>>>>>   */
>>>>> @@ -5012,6 +5183,9 @@ main(int argc, char *argv[])
>>>>>      struct ovsdb_idl_index
>>>>> *sbrec_chassis_template_var_index_by_chassis
>>>>>          = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
>>>>>                                   
>>>>> &sbrec_chassis_template_var_col_chassis);
>>>>> +    struct ovsdb_idl_index
>>>>> *sbrec_advertised_route_index_by_datapath
>>>>> +        = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
>>>>> +                                 
>>>>> &sbrec_advertised_route_col_datapath);
>>>>>  
>>>>>      ovsdb_idl_track_add_all(ovnsb_idl_loop.idl);
>>>>>      ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
>>>>> @@ -5095,6 +5269,7 @@ main(int argc, char *argv[])
>>>>>      ENGINE_NODE(mac_cache, "mac_cache");
>>>>>      ENGINE_NODE(bfd_chassis, "bfd_chassis");
>>>>>      ENGINE_NODE(dns_cache, "dns_cache");
>>>>> +    ENGINE_NODE(route, "route");
>>>>>  
>>>>>  #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME,
>>>>> NAME_STR);
>>>>>      SB_NODES
>>>>> @@ -5117,6 +5292,15 @@ main(int argc, char *argv[])
>>>>>      engine_add_input(&en_lb_data, &en_runtime_data,
>>>>>                       lb_data_runtime_data_handler);
>>>>>  
>>>>> +    engine_add_input(&en_route, &en_ovs_open_vswitch, NULL);
>>>>> +    engine_add_input(&en_route, &en_sb_chassis, NULL);
>>>>> +    engine_add_input(&en_route, &en_sb_port_binding,
>>>>> +                     route_sb_port_binding_data_handler);
>>>>> +    engine_add_input(&en_route, &en_runtime_data,
>>>>> +                     route_runtime_data_handler);
>>>>> +    engine_add_input(&en_route, &en_sb_advertised_route,
>>>>> +                     route_sb_advertised_route_data_handler);
>>>>> +
>>>>>      engine_add_input(&en_addr_sets, &en_sb_address_set,
>>>>>                       addr_sets_sb_address_set_handler);
>>>>>      engine_add_input(&en_port_groups, &en_sb_port_group,
>>>>> @@ -5302,6 +5486,9 @@ main(int argc, char *argv[])
>>>>>                       controller_output_mac_cache_handler);
>>>>>      engine_add_input(&en_controller_output, &en_bfd_chassis,
>>>>>                       controller_output_bfd_chassis_handler);
>>>>> +    /* This is just temporary until the route output is
>>>>> actually
>>>>> used. */
>>>>> +    engine_add_input(&en_controller_output, &en_route,
>>>>> +                     controller_output_bfd_chassis_handler);
>>>>>  
>>>>>      struct engine_arg engine_arg = {
>>>>>          .sb_idl = ovnsb_idl_loop.idl,
>>>>> @@ -5332,6 +5519,8 @@ main(int argc, char *argv[])
>>>>>                                 
>>>>> sbrec_static_mac_binding_by_datapath);
>>>>>      engine_ovsdb_node_add_index(&en_sb_chassis_template_var,
>>>>> "chassis",
>>>>>                                 
>>>>> sbrec_chassis_template_var_index_by_chassis);
>>>>> +    engine_ovsdb_node_add_index(&en_sb_advertised_route,
>>>>> "datapath",
>>>>> +                               
>>>>> sbrec_advertised_route_index_by_datapath);
>>>>>     
>>>>> engine_ovsdb_node_add_index(&en_ovs_flow_sample_collector_set,
>>>>> "id",
>>>>>                                 
>>>>> ovsrec_flow_sample_collector_set_by_id);
>>>>>      engine_ovsdb_node_add_index(&en_ovs_port, "qos",
>>>>> ovsrec_port_by_qos);
>>>>> diff --git a/controller/route.c b/controller/route.c
>>>>> new file mode 100644
>>>>> index 000000000..fdff7573a
>>>>> --- /dev/null
>>>>> +++ b/controller/route.c
>>>>> @@ -0,0 +1,189 @@
>>>>> +/*
>>>>> + * Licensed under the Apache License, Version 2.0 (the
>>>>> "License");
>>>>> + * you may not use this file except in compliance with the
>>>>> License.
>>>>> + * You may obtain a copy of the License at:
>>>>> + *
>>>>> + *     http://www.apache.org/licenses/LICENSE-2.0
>>>>> + *
>>>>> + * Unless required by applicable law or agreed to in writing,
>>>>> software
>>>>> + * distributed under the License is distributed on an "AS IS"
>>>>> BASIS,
>>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
>>>>> express or
>>>>> implied.
>>>>> + * See the License for the specific language governing
>>>>> permissions
>>>>> and
>>>>> + * limitations under the License.
>>>>> + */
>>>>> +
>>>>> +#include <config.h>
>>>>> +
>>>>> +#include <net/if.h>
>>>>> +
>>>>> +#include "openvswitch/vlog.h"
>>>>> +
>>>>> +#include "lib/ovn-sb-idl.h"
>>>>> +
>>>>> +#include "binding.h"
>>>>> +#include "ha-chassis.h"
>>>>> +#include "local_data.h"
>>>>> +#include "route.h"
>>>>> +
>>>>> +
>>>>> +VLOG_DEFINE_THIS_MODULE(exchange);
>>>>> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
>>>>> 20);
>>>>> +
>>>>> +/* While the linux kernel can handle 2^32 routing tables, only
>>>>> so
>>>>> many can fit
>>>>> + * in the corresponding VRF interface name. */
>>>>> +#define MAX_TABLE_ID 1000000000
>>>>> +
>>>>> +bool
>>>>> +route_exchange_relevant_port(const struct sbrec_port_binding
>>>>> *pb)
>>>>> +{
>>>>> +    return (pb && smap_get_bool(&pb->options, "dynamic-
>>>>> routing",
>>>>> false));
>>>>> +}
>>>>> +
>>>>> +uint32_t
>>>>> +advertise_route_hash(const struct in6_addr *dst, unsigned int
>>>>> plen)
>>>>> +{
>>>>> +    uint32_t hash = hash_bytes(dst->s6_addr, 16, 0);
>>>>> +    return hash_int(plen, hash);
>>>>> +}
>>>>> +
>>>>> +static const struct sbrec_port_binding*
>>>>> +find_local_crp(struct ovsdb_idl_index
>>>>> *sbrec_port_binding_by_name,
>>>>> +               const struct sbrec_chassis *chassis,
>>>>> +               const struct sset *active_tunnels,
>>>>> +               const struct sbrec_port_binding *pb)
>>>>> +{
>>>>> +    if (!pb) {
>>>>> +        return NULL;
>>>>> +    }
>>>>> +    const char *crp = smap_get(&pb->options, "chassis-
>>>>> redirect-
>>>>> port");
>>>>> +    if (!crp) {
>>>>> +        return NULL;
>>>>> +    }
>>>>> +    if (!lport_is_chassis_resident(sbrec_port_binding_by_name,
>>>>> chassis,
>>>>> +                                   active_tunnels, crp)) {
>>>>> +        return NULL;
>>>>> +    }
>>>>> +    return lport_lookup_by_name(sbrec_port_binding_by_name,
>>>>> crp);
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +advertise_datapath_cleanup(struct advertise_datapath_entry
>>>>> *ad)
>>>>> +{
>>>>> +    struct advertise_route_entry *ar;
>>>>> +    HMAP_FOR_EACH_SAFE (ar, node, &ad->routes) {
>>>>> +        hmap_remove(&ad->routes, &ar->node);
>>>>> +        free(ar);
>>>>> +    }
>>>>> +    hmap_destroy(&ad->routes);
>>>>> +    sset_destroy(&ad->bound_ports);
>>>>> +    free(ad);
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +route_run(struct route_ctx_in *r_ctx_in,
>>>>> +          struct route_ctx_out *r_ctx_out)
>>>>> +{
>>>>> +    const struct local_datapath *ld;
>>>>> +    HMAP_FOR_EACH (ld, hmap_node, r_ctx_in->local_datapaths) {
>>>>> +        if (!ld->n_peer_ports || ld->is_switch) {
>>>>> +            continue;
>>>>> +        }
>>>>> +
>>>>> +        bool relevant_datapath = false;
>>>>> +        struct advertise_datapath_entry *ad =
>>>>> xzalloc(sizeof(*ad));
>>>>> +        ad->key = ld->datapath->tunnel_key;
>>>>> +        ad->db = ld->datapath;
>>>>> +        hmap_init(&ad->routes);
>>>>> +        sset_init(&ad->bound_ports);
>>>>> +
>>>>> +        /* This is a LR datapath, find LRPs with route
>>>>> exchange
>>>>> options
>>>>> +         * that are bound locally. */
>>>>> +        for (size_t i = 0; i < ld->n_peer_ports; i++) {
>>>>> +            const struct sbrec_port_binding *local_peer
>>>>> +                = ld->peer_ports[i].local;
>>>>> +            const struct sbrec_port_binding *sb_crp =
>>>>> find_local_crp(
>>>>> +                r_ctx_in->sbrec_port_binding_by_name,
>>>>> +                r_ctx_in->chassis,
>>>>> +                r_ctx_in->active_tunnels,
>>>>> +                local_peer);
>>>>> +            if (!route_exchange_relevant_port(sb_crp)) {
>>>>> +                continue;
>>>>> +            }
>>>>> +
>>>>> +            ad->maintain_vrf |= smap_get_bool(&sb_crp-
>>>>>> options,
>>>>> +                                          "maintain-vrf",
>>>>> false);
>>>>> +            ad->use_netns |= smap_get_bool(&sb_crp->options,
>>>>> +                                       "use-netns", false);
>>>>> +            relevant_datapath = true;
>>>>> +            sset_add(&ad->bound_ports, local_peer-
>>>>>> logical_port);
>>>>> +        }
>>>>> +
>>>>> +        if (!relevant_datapath) {
>>>>> +            advertise_datapath_cleanup(ad);
>>
>> Hi Martin, Hi Dumitru,
>>
>>>>
>>>> In the for-loop above, we loop only through the chassis redirect
>>>> ports
>>>> (find_local_crp), which means that "relevant_datapath=true" is
>>>> never
>>>> set for gateway routers. It's bit hard to discern GW router ports
>>>> just
>>>
>>> +1 I think this needs to be fixed.  Gateway routers won't have
>>> chassis-redirect ports.  And, at least for ovn-kubernetes, that's
>>> exactly the use case we might want BGP integration for.
>>
>> Thats a very good point. I only tested it with chassis-redirect
>> ports.
>>>
>>>> from the data in SB port bindings, but I was wondering if we
>>>> could
>>>> perhaps pass more options from NB LRP to SB PB and decide only
>>>> based on
>>>> those options (and locality). Right now only "dynamic-routing"
>>>> option
>>>> is copied from LRP options to PB options, we could also pass
>>>> "dynamic-
>>>> routing-connected", "dynamic-routing-static (and in the future
>>>> work
>>>> "redistribute-nat") and decide based on those.
>>>
>>> I'm not sure I understand why we'd propagate
>>> "dynamic-routing-connected/static" to the Southbound port binding. 
>>> IIUC
>>> those options control what routes to be advertised for a given
>>> router
>>> (or port).  And routes to be advertised are written in the
>>> SB.Advertised_Route table by ovn-northd.  Why would ovn-controller
>>> care
>>> about those options?
>>
>> My current spontanious idea would be to set an option on all relevant
>> Port_Bindings based on LRs with "dynamic-routing" and if the LRP
>> would
>> be relevant for dynamic routing.
>> What i understood for this a dynamic-routing relevant LRP is one
>> where:
>>
>> "LRP is connected to a LS with a localnet LSP" &&
>>   ("LRP has ha_chassis_group/gateway_chassis set" || "LR has chassis
>> set")
>>
>> Would that be appropriate from your perspective or would that still
>> miss
>> something?
>>
>> Then the ovn-controller just checks the option on the Port_Binding
>> and
>> if the port is local.
>>
>> Thanks a lot
>> Felix
> 
> Given that northd already associates route with the right datapath,
> wouldn't it be sufficient to cycle through all routes associated with
> the DP and "do stuff" if DP is local? I found some other examples [0]
> of functions within ovn-controller that get the "local_datapaths"[1]
> from "runtime data".
> 

That was my initial thought too.  And I think that's fine but I didn't
try it out yet.

I'm still not sure why we'd need to add more information to the port
binding.  But maybe I misunderstood Felix's intention above.

> [0]https://github.com/ovn-org/ovn/blob/ebe5d70122ce0f74067858f5cb19276c852a81da/controller/ovn-controller.c#L5753
> [1]
> https://github.com/ovn-org/ovn/blob/ebe5d70122ce0f74067858f5cb19276c852a81da/controller/local_data.h#L44
> 
> Martin.
> 
>>
>>>
>>> Regards,
>>> Dumitru
>>>
>>>>
>>>> Martin.
>>>>
>>>>> +            continue;
>>>>> +        }
>>>>> +        tracked_datapath_add(ld->datapath,
>>>>> TRACKED_RESOURCE_NEW,
>>>>> +                             r_ctx_out->tracked_re_datapaths);
>>>>> +
>>>>> +        /* While tunnel_key would most likely never be
>>>>> negative, the
>>>>> compiler
>>>>> +         * has opinions if we don't check before using it in
>>>>> snprintf below. */
>>>>> +        if (ld->datapath->tunnel_key < 0 ||
>>>>> +            ld->datapath->tunnel_key > MAX_TABLE_ID) {
>>>>> +            VLOG_WARN_RL(&rl,
>>>>> +                         "skip route sync for datapath
>>>>> "UUID_FMT", "
>>>>> +                         "tunnel_key %"PRIi64" would make VRF
>>>>> interface name "
>>>>> +                         "overflow.",
>>>>> +                         UUID_ARGS(&ld->datapath-
>>>>>> header_.uuid),
>>>>> +                         ld->datapath->tunnel_key);
>>>>> +            goto cleanup;
>>>>> +        }
>>>>> +
>>>>> +        if (ad->maintain_vrf && ad->use_netns) {
>>>>> +            VLOG_WARN_RL(&rl,
>>>>> +                         "For Datapath %"PRIu64" both
>>>>> maintain-vrf
>>>>> and "
>>>>> +                         "use-netns are set, this will never
>>>>> work",
>>>>> +                         ld->datapath->tunnel_key);
>>>>> +            goto cleanup;
>>>>> +        }
>>>>> +
>>>>> +        struct sbrec_advertised_route *route_filter =
>>>>> +            sbrec_advertised_route_index_init_row(
>>>>> +                r_ctx_in->sbrec_advertised_route_by_datapath);
>>>>> +       
>>>>> sbrec_advertised_route_index_set_datapath(route_filter, ld-
>>>>>> datapath);
>>>>> +        struct sbrec_advertised_route *route;
>>>>> +        SBREC_ADVERTISED_ROUTE_FOR_EACH_EQUAL (route,
>>>>> route_filter,
>>>>> +                r_ctx_in->sbrec_advertised_route_by_datapath)
>>>>> {
>>>>> +            struct in6_addr prefix;
>>>>> +            unsigned int plen;
>>>>> +            if (!ip46_parse_cidr(route->ip_prefix, &prefix,
>>>>> &plen))
>>>>> {
>>>>> +                VLOG_WARN_RL(&rl, "bad 'ip_prefix' %s in route
>>>>> "
>>>>> +                             UUID_FMT, route->ip_prefix,
>>>>> +                             UUID_ARGS(&route->header_.uuid));
>>>>> +                continue;
>>>>> +            }
>>>>> +
>>>>> +            struct advertise_route_entry *ar =
>>>>> xzalloc(sizeof(*ar));
>>>>> +            hmap_insert(&ad->routes, &ar->node,
>>>>> +                        advertise_route_hash(&prefix, plen));
>>>>> +            ar->addr = prefix;
>>>>> +            ar->plen = plen;
>>>>> +        }
>>>>> +       
>>>>> sbrec_advertised_route_index_destroy_row(route_filter);
>>>>> +
>>>>> +        hmap_insert(r_ctx_out->announce_routes, &ad->node, ad-
>>>>>> key);
>>>>> +        continue;
>>>>> +
>>>>> +cleanup:
>>>>> +        advertise_datapath_cleanup(ad);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +route_cleanup(struct hmap *announce_routes)
>>>>> +{
>>>>> +    struct advertise_datapath_entry *ad;
>>>>> +    HMAP_FOR_EACH_SAFE (ad, node, announce_routes) {
>>>>> +        hmap_remove(announce_routes, &ad->node);
>>>>> +        advertise_datapath_cleanup(ad);
>>>>> +    }
>>>>> +}
>>>>> diff --git a/controller/route.h b/controller/route.h
>>>>> new file mode 100644
>>>>> index 000000000..2a54cf3e3
>>>>> --- /dev/null
>>>>> +++ b/controller/route.h
>>>>> @@ -0,0 +1,73 @@
>>>>> +/*
>>>>> + * Licensed under the Apache License, Version 2.0 (the
>>>>> "License");
>>>>> + * you may not use this file except in compliance with the
>>>>> License.
>>>>> + * You may obtain a copy of the License at:
>>>>> + *
>>>>> + *     http://www.apache.org/licenses/LICENSE-2.0
>>>>> + *
>>>>> + * Unless required by applicable law or agreed to in writing,
>>>>> software
>>>>> + * distributed under the License is distributed on an "AS IS"
>>>>> BASIS,
>>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
>>>>> express or
>>>>> implied.
>>>>> + * See the License for the specific language governing
>>>>> permissions
>>>>> and
>>>>> + * limitations under the License.
>>>>> + */
>>>>> +
>>>>> +#ifndef ROUTE_H
>>>>> +#define ROUTE_H 1
>>>>> +
>>>>> +#include <stdbool.h>
>>>>> +#include <netinet/in.h>
>>>>> +#include "openvswitch/hmap.h"
>>>>> +#include "sset.h"
>>>>> +
>>>>> +struct hmap;
>>>>> +struct ovsdb_idl_index;
>>>>> +struct sbrec_chassis;
>>>>> +struct sbrec_port_binding;
>>>>> +struct sset;
>>>>> +
>>>>> +struct route_ctx_in {
>>>>> +    struct ovsdb_idl_txn *ovnsb_idl_txn;
>>>>> +    struct ovsdb_idl_index *sbrec_port_binding_by_name;
>>>>> +    const struct sbrec_chassis *chassis;
>>>>> +    const struct sset *active_tunnels;
>>>>> +    struct hmap *local_datapaths;
>>>>> +    const struct sset *local_lports;
>>>>> +    struct ovsdb_idl_index
>>>>> *sbrec_advertised_route_by_datapath;
>>>>> +};
>>>>> +
>>>>> +struct route_ctx_out {
>>>>> +    struct hmap *tracked_re_datapaths;
>>>>> +    /* Contains struct advertise_datapath_entry */
>>>>> +    struct hmap *announce_routes;
>>>>> +};
>>>>> +
>>>>> +struct advertise_datapath_entry {
>>>>> +    struct hmap_node node;
>>>>> +    /* tunnel_key of the datapath */
>>>>> +    int64_t key;
>>>>> +    const struct sbrec_datapath_binding *db;
>>>>> +    bool maintain_vrf;
>>>>> +    bool use_netns;
>>>>> +    struct hmap routes;
>>>>> +    /* the name of the port bindings locally bound for this
>>>>> datapath
>>>>> and
>>>>> +     * running route exchange logic. */
>>>>> +    struct sset bound_ports;
>>>>> +};
>>>>> +
>>>>> +struct advertise_route_entry {
>>>>> +    struct hmap_node node;
>>>>> +    struct in6_addr addr;
>>>>> +    unsigned int plen;
>>>>> +    /* used by the route-exchange module to determine if the
>>>>> route
>>>>> is
>>>>> +     * already installed */
>>>>> +    bool installed;
>>>>> +};
>>>>> +
>>>>> +bool route_exchange_relevant_port(const struct
>>>>> sbrec_port_binding
>>>>> *pb);
>>>>> +uint32_t advertise_route_hash(const struct in6_addr *dst,
>>>>> unsigned
>>>>> int plen);
>>>>> +void route_run(struct route_ctx_in *,
>>>>> +               struct route_ctx_out *);
>>>>> +void route_cleanup(struct hmap *announce_routes);
>>>>> +
>>>>> +#endif /* ROUTE_H */
>>>>> diff --git a/tests/automake.mk b/tests/automake.mk
>>>>> index 3899c9e80..9244532fa 100644
>>>>> --- a/tests/automake.mk
>>>>> +++ b/tests/automake.mk
>>>>> @@ -305,6 +305,7 @@ tests_ovstest_LDADD =
>>>>> $(OVS_LIBDIR)/daemon.lo \
>>>>>   controller/ovsport.$(OBJEXT) \
>>>>>   controller/patch.$(OBJEXT) \
>>>>>   controller/vif-plug.$(OBJEXT) \
>>>>> + controller/route.$(OBJEXT) \
>>>>>   northd/ipam.$(OBJEXT)
>>>>>  
>>>>>  # Python tests.
>>>>
>>>> _______________________________________________
>>>> dev mailing list
>>>> [email protected]
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
> 

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to