[ovs-dev,v8,3/3] ofctrl: Introduce ecmp_nexthop_monitor.

Introduce ecmp_nexthop_monitor in ovn-controller in order to track and
flush ecmp-symmetric reply ct entires when requested by the CMS (e.g
removing the related static routes).

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 controller/ofctrl.c         | 54 +++++++++++++++++++++++++++++++++++++
 controller/ofctrl.h         |  2 ++
 controller/ovn-controller.c |  2 ++
 tests/system-ovn.at         |  8 ++++++
 4 files changed, 66 insertions(+)

I only have one small note on this patch.

On 7/30/24 17:52, Lorenzo Bianconi wrote:
> Introduce ecmp_nexthop_monitor in ovn-controller in order to track and
> flush ecmp-symmetric reply ct entires when requested by the CMS (e.g
> removing the related static routes).
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>   controller/ofctrl.c         | 54 +++++++++++++++++++++++++++++++++++++
>   controller/ofctrl.h         |  2 ++
>   controller/ovn-controller.c |  2 ++
>   tests/system-ovn.at         |  8 ++++++
>   4 files changed, 66 insertions(+)
> 
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index f9387d375..3d450c0cd 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -389,9 +389,17 @@ struct meter_band_entry {
>   
>   static struct shash meter_bands;
>   
> +#define ECMP_NEXTHOP_IDS_LEN    65535

The number of ECMP nexthop IDs is defined both here and in northd.c This 
means that if we decide to expand the number of allowed IDs, we have to 
update the constant in both places.

I think it would make more sense to define this constant in a header 
file that is included by both northd.c and ofctrl.c.

> +static unsigned long *ecmp_nexthop_ids;
> +
>   static void ofctrl_meter_bands_destroy(void);
>   static void ofctrl_meter_bands_clear(void);
>   
> +static void ecmp_nexthop_monitor_run(
> +        const struct sbrec_ecmp_nexthop_table *enh_table,
> +        struct ovs_list *msgs);
> +
> +
>   /* MFF_* field ID for our Geneve option.  In S_TLV_TABLE_MOD_SENT, this is
>    * the option we requested (we don't know whether we obtained it yet).  In
>    * S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
> @@ -430,6 +438,7 @@ ofctrl_init(struct ovn_extend_table *group_table,
>       groups = group_table;
>       meters = meter_table;
>       shash_init(&meter_bands);
> +    ecmp_nexthop_ids = bitmap_allocate(ECMP_NEXTHOP_IDS_LEN);
>   }
>   
>   /* S_NEW, for a new connection.
> @@ -877,6 +886,7 @@ ofctrl_destroy(void)
>       expr_symtab_destroy(&symtab);
>       shash_destroy(&symtab);
>       ofctrl_meter_bands_destroy();
> +    bitmap_free(ecmp_nexthop_ids);
>   }
>   
>   uint64_t
> @@ -2306,6 +2316,47 @@ add_meter(struct ovn_extend_table_info *m_desired,
>       ofctrl_meter_bands_alloc(sb_meter, m_desired, msgs);
>   }
>   
> +static void
> +ecmp_nexthop_monitor_flush_ct_entry(uint64_t id, struct ovs_list *msgs)
> +{
> +    ovs_u128 mask = {
> +        /* ct_labels.label BITS[96-127] */
> +        .u64.hi = 0xffffffff00000000,
> +    };
> +    ovs_u128 nexthop = {
> +        .u64.hi = id << 32,
> +    };
> +    struct ofp_ct_match match = {
> +        .labels = nexthop,
> +        .labels_mask = mask,
> +    };
> +    struct ofpbuf *msg = ofp_ct_match_encode(&match, NULL,
> +                                             rconn_get_version(swconn));
> +    ovs_list_push_back(msgs, &msg->list_node);
> +}
> +
> +static void
> +ecmp_nexthop_monitor_run(const struct sbrec_ecmp_nexthop_table *enh_table,
> +                         struct ovs_list *msgs)
> +{
> +    unsigned long *ids = bitmap_allocate(ECMP_NEXTHOP_IDS_LEN);
> +
> +    const struct sbrec_ecmp_nexthop *sbrec_ecmp_nexthop;
> +    SBREC_ECMP_NEXTHOP_TABLE_FOR_EACH (sbrec_ecmp_nexthop, enh_table) {
> +        bitmap_set1(ids, sbrec_ecmp_nexthop->id);
> +    }
> +
> +    int id;
> +    BITMAP_FOR_EACH_1 (id, ECMP_NEXTHOP_IDS_LEN, ecmp_nexthop_ids) {
> +        if (!bitmap_is_set(ids, id)) {
> +            ecmp_nexthop_monitor_flush_ct_entry(id, msgs);
> +        }
> +    }
> +
> +    bitmap_free(ecmp_nexthop_ids);
> +    ecmp_nexthop_ids = ids;
> +}
> +
>   static void
>   installed_flow_add(struct ovn_flow *d,
>                      struct ofputil_bundle_ctrl_msg *bc,
> @@ -2664,6 +2715,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
>              struct shash *pending_ct_zones,
>              struct hmap *pending_lb_tuples,
>              struct ovsdb_idl_index *sbrec_meter_by_name,
> +           const struct sbrec_ecmp_nexthop_table *enh_table,
>              uint64_t req_cfg,
>              bool lflows_changed,
>              bool pflows_changed)
> @@ -2704,6 +2756,8 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
>       /* OpenFlow messages to send to the switch to bring it up-to-date. */
>       struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
>   
> +    ecmp_nexthop_monitor_run(enh_table, &msgs);
> +
>       /* Iterate through ct zones that need to be flushed. */
>       struct shash_node *iter;
>       SHASH_FOR_EACH(iter, pending_ct_zones) {
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index 129e3b6ad..33953a8a4 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -31,6 +31,7 @@ struct ofpbuf;
>   struct ovsrec_bridge;
>   struct ovsrec_open_vswitch_table;
>   struct sbrec_meter_table;
> +struct sbrec_ecmp_nexthop_table;
>   struct shash;
>   
>   struct ovn_desired_flow_table {
> @@ -59,6 +60,7 @@ void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
>                   struct shash *pending_ct_zones,
>                   struct hmap *pending_lb_tuples,
>                   struct ovsdb_idl_index *sbrec_meter_by_name,
> +                const struct sbrec_ecmp_nexthop_table *enh_table,
>                   uint64_t nb_cfg,
>                   bool lflow_changed,
>                   bool pflow_changed);
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index f5674184a..bbcaae4cb 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5720,6 +5720,8 @@ main(int argc, char *argv[])
>                                      &ct_zones_data->ctx.pending,
>                                      &lb_data->removed_tuples,
>                                      sbrec_meter_by_name,
> +                                   sbrec_ecmp_nexthop_table_get(
> +                                        ovnsb_idl_loop.idl),
>                                      ofctrl_seqno_get_req_cfg(),
>                                      engine_node_changed(&en_lflow_output),
>                                      engine_node_changed(&en_pflow_output));
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index bb43f9ca3..d3294c71f 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -6248,6 +6248,10 @@ sed -e 's/labels=0x[[0-9]]/labels=0x?/' | sort], [0], [dnl
>   tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x?000000000401020500000000,protoinfo=(state=<cleared>)
>   ])
>   
> +# Flush connection tracking entries
> +ovn-nbctl --wait=hv lr-route-del R1
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.1)])
> +
>   OVS_APP_EXIT_AND_WAIT([ovn-controller])
>   
>   as ovn-sb
> @@ -6470,6 +6474,10 @@ sed -e 's/labels=0x[[0-9]]/labels=0x?/' | sort], [0], [dnl
>   tcp,orig=(src=fd07::1,dst=fd01::2,sport=<cleared>,dport=<cleared>),reply=(src=fd01::2,dst=fd07::1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x?000000001001020400000000,protoinfo=(state=<cleared>)
>   ])
>   
> +# Flush connection tracking entries
> +check ovn-nbctl --wait=hv lr-route-del R1
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd01::2)])
> +
>   OVS_APP_EXIT_AND_WAIT([ovn-controller])
>   
>   as ovn-sb

On Aug 01, Mark Michelson wrote:
> I only have one small note on this patch.
> 
> On 7/30/24 17:52, Lorenzo Bianconi wrote:
> > Introduce ecmp_nexthop_monitor in ovn-controller in order to track and
> > flush ecmp-symmetric reply ct entires when requested by the CMS (e.g
> > removing the related static routes).
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> >   controller/ofctrl.c         | 54 +++++++++++++++++++++++++++++++++++++
> >   controller/ofctrl.h         |  2 ++
> >   controller/ovn-controller.c |  2 ++
> >   tests/system-ovn.at         |  8 ++++++
> >   4 files changed, 66 insertions(+)
> > 
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index f9387d375..3d450c0cd 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -389,9 +389,17 @@ struct meter_band_entry {
> >   static struct shash meter_bands;
> > +#define ECMP_NEXTHOP_IDS_LEN    65535
> 
> The number of ECMP nexthop IDs is defined both here and in northd.c This
> means that if we decide to expand the number of allowed IDs, we have to
> update the constant in both places.
> 
> I think it would make more sense to define this constant in a header file
> that is included by both northd.c and ofctrl.c.

ack, I will fix it in v9.

Regards,
Lorenzo

> 
> > +static unsigned long *ecmp_nexthop_ids;
> > +
> >   static void ofctrl_meter_bands_destroy(void);
> >   static void ofctrl_meter_bands_clear(void);
> > +static void ecmp_nexthop_monitor_run(
> > +        const struct sbrec_ecmp_nexthop_table *enh_table,
> > +        struct ovs_list *msgs);
> > +
> > +
> >   /* MFF_* field ID for our Geneve option.  In S_TLV_TABLE_MOD_SENT, this is
> >    * the option we requested (we don't know whether we obtained it yet).  In
> >    * S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
> > @@ -430,6 +438,7 @@ ofctrl_init(struct ovn_extend_table *group_table,
> >       groups = group_table;
> >       meters = meter_table;
> >       shash_init(&meter_bands);
> > +    ecmp_nexthop_ids = bitmap_allocate(ECMP_NEXTHOP_IDS_LEN);
> >   }
> >   
> >   /* S_NEW, for a new connection.
> > @@ -877,6 +886,7 @@ ofctrl_destroy(void)
> >       expr_symtab_destroy(&symtab);
> >       shash_destroy(&symtab);
> >       ofctrl_meter_bands_destroy();
> > +    bitmap_free(ecmp_nexthop_ids);
> >   }
> >   uint64_t
> > @@ -2306,6 +2316,47 @@ add_meter(struct ovn_extend_table_info *m_desired,
> >       ofctrl_meter_bands_alloc(sb_meter, m_desired, msgs);
> >   }
> > +static void
> > +ecmp_nexthop_monitor_flush_ct_entry(uint64_t id, struct ovs_list *msgs)
> > +{
> > +    ovs_u128 mask = {
> > +        /* ct_labels.label BITS[96-127] */
> > +        .u64.hi = 0xffffffff00000000,
> > +    };
> > +    ovs_u128 nexthop = {
> > +        .u64.hi = id << 32,
> > +    };
> > +    struct ofp_ct_match match = {
> > +        .labels = nexthop,
> > +        .labels_mask = mask,
> > +    };
> > +    struct ofpbuf *msg = ofp_ct_match_encode(&match, NULL,
> > +                                             rconn_get_version(swconn));
> > +    ovs_list_push_back(msgs, &msg->list_node);
> > +}
> > +
> > +static void
> > +ecmp_nexthop_monitor_run(const struct sbrec_ecmp_nexthop_table *enh_table,
> > +                         struct ovs_list *msgs)
> > +{
> > +    unsigned long *ids = bitmap_allocate(ECMP_NEXTHOP_IDS_LEN);
> > +
> > +    const struct sbrec_ecmp_nexthop *sbrec_ecmp_nexthop;
> > +    SBREC_ECMP_NEXTHOP_TABLE_FOR_EACH (sbrec_ecmp_nexthop, enh_table) {
> > +        bitmap_set1(ids, sbrec_ecmp_nexthop->id);
> > +    }
> > +
> > +    int id;
> > +    BITMAP_FOR_EACH_1 (id, ECMP_NEXTHOP_IDS_LEN, ecmp_nexthop_ids) {
> > +        if (!bitmap_is_set(ids, id)) {
> > +            ecmp_nexthop_monitor_flush_ct_entry(id, msgs);
> > +        }
> > +    }
> > +
> > +    bitmap_free(ecmp_nexthop_ids);
> > +    ecmp_nexthop_ids = ids;
> > +}
> > +
> >   static void
> >   installed_flow_add(struct ovn_flow *d,
> >                      struct ofputil_bundle_ctrl_msg *bc,
> > @@ -2664,6 +2715,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> >              struct shash *pending_ct_zones,
> >              struct hmap *pending_lb_tuples,
> >              struct ovsdb_idl_index *sbrec_meter_by_name,
> > +           const struct sbrec_ecmp_nexthop_table *enh_table,
> >              uint64_t req_cfg,
> >              bool lflows_changed,
> >              bool pflows_changed)
> > @@ -2704,6 +2756,8 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> >       /* OpenFlow messages to send to the switch to bring it up-to-date. */
> >       struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
> > +    ecmp_nexthop_monitor_run(enh_table, &msgs);
> > +
> >       /* Iterate through ct zones that need to be flushed. */
> >       struct shash_node *iter;
> >       SHASH_FOR_EACH(iter, pending_ct_zones) {
> > diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> > index 129e3b6ad..33953a8a4 100644
> > --- a/controller/ofctrl.h
> > +++ b/controller/ofctrl.h
> > @@ -31,6 +31,7 @@ struct ofpbuf;
> >   struct ovsrec_bridge;
> >   struct ovsrec_open_vswitch_table;
> >   struct sbrec_meter_table;
> > +struct sbrec_ecmp_nexthop_table;
> >   struct shash;
> >   struct ovn_desired_flow_table {
> > @@ -59,6 +60,7 @@ void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> >                   struct shash *pending_ct_zones,
> >                   struct hmap *pending_lb_tuples,
> >                   struct ovsdb_idl_index *sbrec_meter_by_name,
> > +                const struct sbrec_ecmp_nexthop_table *enh_table,
> >                   uint64_t nb_cfg,
> >                   bool lflow_changed,
> >                   bool pflow_changed);
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index f5674184a..bbcaae4cb 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -5720,6 +5720,8 @@ main(int argc, char *argv[])
> >                                      &ct_zones_data->ctx.pending,
> >                                      &lb_data->removed_tuples,
> >                                      sbrec_meter_by_name,
> > +                                   sbrec_ecmp_nexthop_table_get(
> > +                                        ovnsb_idl_loop.idl),
> >                                      ofctrl_seqno_get_req_cfg(),
> >                                      engine_node_changed(&en_lflow_output),
> >                                      engine_node_changed(&en_pflow_output));
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index bb43f9ca3..d3294c71f 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -6248,6 +6248,10 @@ sed -e 's/labels=0x[[0-9]]/labels=0x?/' | sort], [0], [dnl
> >   tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x?000000000401020500000000,protoinfo=(state=<cleared>)
> >   ])
> > +# Flush connection tracking entries
> > +ovn-nbctl --wait=hv lr-route-del R1
> > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.1)])
> > +
> >   OVS_APP_EXIT_AND_WAIT([ovn-controller])
> >   as ovn-sb
> > @@ -6470,6 +6474,10 @@ sed -e 's/labels=0x[[0-9]]/labels=0x?/' | sort], [0], [dnl
> >   tcp,orig=(src=fd07::1,dst=fd01::2,sport=<cleared>,dport=<cleared>),reply=(src=fd01::2,dst=fd07::1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x?000000001001020400000000,protoinfo=(state=<cleared>)
> >   ])
> > +# Flush connection tracking entries
> > +check ovn-nbctl --wait=hv lr-route-del R1
> > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd01::2)])
> > +
> >   OVS_APP_EXIT_AND_WAIT([ovn-controller])
> >   as ovn-sb
>