| Message ID | ed97eb6fe7ec5ad062a529e2acf22aa579cadf0f.1721722209.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce ECMP_nexthop monitor in ovn-controller | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
On Tue, Jul 23, 2024 at 10:17 AM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> 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> > Hi Lorenzo, thank you for the patch, I have one comment down below. --- > 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 8a19106c2..33dc11f35 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 > +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); > +} > We should check if flush of ct_label is actually supported otherwise every flush will result in error messages in ovs logs and potentially ours. > + > +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 d6d001b1a..074576d77 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5704,6 +5704,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 2c4c3f5fb..9effa77b0 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -6229,6 +6229,10 @@ OVS_WAIT_UNTIL([ > test $(ovs-ofctl dump-flows br-int | grep -c 'table=OFTABLE_ECMP_NH, > n_packets') -eq 0 > ]) > > +# 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-ofctl dump-flows br-int > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > @@ -6437,6 +6441,10 @@ OVS_WAIT_UNTIL([ > test $(ovs-ofctl dump-flows br-int | grep -c 'table=OFTABLE_ECMP_NH, > n_packets') -eq 0 > ]) > > +# 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-ofctl dump-flows br-int > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > -- > 2.45.2 > > Thanks, Ales
> On Tue, Jul 23, 2024 at 10:17 AM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> 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> > > > > Hi Lorenzo, > > thank you for the patch, I have one comment down below. > > --- > > 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 8a19106c2..33dc11f35 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 > > +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); > > +} > > > > We should check if flush of ct_label is actually supported otherwise every > flush > will result in error messages in ovs logs and potentially ours. > please correct me if I am wrong, but the capability to send ofctrl msgs to check if we can flush ct entries via ct_label/ct_label_mask is not upstream yet. Since this would address just some corner cases I guess we can add it as a follow-up patch. What do you think? Regards, Lorenzo > > > + > > +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 d6d001b1a..074576d77 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -5704,6 +5704,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 2c4c3f5fb..9effa77b0 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -6229,6 +6229,10 @@ OVS_WAIT_UNTIL([ > > test $(ovs-ofctl dump-flows br-int | grep -c 'table=OFTABLE_ECMP_NH, > > n_packets') -eq 0 > > ]) > > > > +# 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-ofctl dump-flows br-int > > > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > @@ -6437,6 +6441,10 @@ OVS_WAIT_UNTIL([ > > test $(ovs-ofctl dump-flows br-int | grep -c 'table=OFTABLE_ECMP_NH, > > n_packets') -eq 0 > > ]) > > > > +# 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-ofctl dump-flows br-int > > > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > -- > > 2.45.2 > > > > > Thanks, > Ales > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com > <https://red.ht/sig>
diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 8a19106c2..33dc11f35 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 +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 d6d001b1a..074576d77 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5704,6 +5704,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 2c4c3f5fb..9effa77b0 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6229,6 +6229,10 @@ OVS_WAIT_UNTIL([ test $(ovs-ofctl dump-flows br-int | grep -c 'table=OFTABLE_ECMP_NH, n_packets') -eq 0 ]) +# 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-ofctl dump-flows br-int OVS_APP_EXIT_AND_WAIT([ovn-controller]) @@ -6437,6 +6441,10 @@ OVS_WAIT_UNTIL([ test $(ovs-ofctl dump-flows br-int | grep -c 'table=OFTABLE_ECMP_NH, n_packets') -eq 0 ]) +# 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-ofctl dump-flows br-int OVS_APP_EXIT_AND_WAIT([ovn-controller])
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(+)