| Message ID | 20201120001343.1485936-1-mmichels@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] Clear port binding flows when datapath CT zone changes. | expand |
On Fri, Nov 20, 2020 at 5:44 AM Mark Michelson <mmichels@redhat.com> wrote: > > In commit f9cab11d5fabe2ae321a3b4bad5972b61df958c0, a LOG_TO_PHY flow > was changed so that it was no longer associated with a particular port > binding. The logic there was that the particular flow contains data > pertaining to the port binding's peer's datapath, so it didn't make > sense to associate the flow with the port binding. This change was > necessary in order for flows to be recalculated properly if the > requested SNAT CT zone on a gateway router was changed. Since the > datapath was changed but no port bindings were changed, that particular > flow needed to be cleared so it could be recalculated with the new CT > zones put in place. > > Unfortunately, that change broke some other behavior. Specifically, if a > router was changed from a distributed router to a gateway router, then > its port bindings and its port bindings' peers would be updated so that > they were no longer type "patch" but instead type "l3gateway". They > would attempt to remove all associated physical flows and then install > the newly relevant ones. Since the LOG_TO_PHY flow was no longer > associated with a port binding, that flow would remain. The result was > that traffic could be sent to the gateway router on chassis where the > gateway router was not pinned. > > This commit seeks to fix both behaviors. Now if CT zones are changed on > a particular datapath, then all port bindings on that datapath, as well > as all of those port bindings' peers will have their physical flows > removed. When physical flows are recomputed, all of the appropriate > flows will be added. > > Signed-off-by: Mark Michelson <mmichels@redhat.com> > --- > controller/ovn-controller.c | 31 ++++++++++++++++++-- > controller/physical.c | 58 +++++++++++++++++++++++++++++-------- > controller/physical.h | 4 +++ > 3 files changed, 79 insertions(+), 14 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 25de0c72f..eceb804ac 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -64,6 +64,7 @@ > #include "timer.h" > #include "stopwatch.h" > #include "lib/inc-proc-eng.h" > +#include "hmapx.h" > > VLOG_DEFINE_THIS_MODULE(main); > > @@ -549,7 +550,7 @@ add_pending_ct_zone_entry(struct shash *pending_ct_zones, > static void > update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, > struct simap *ct_zones, unsigned long *ct_zone_bitmap, > - struct shash *pending_ct_zones) > + struct shash *pending_ct_zones, struct hmapx *updated_dps) > { > struct simap_node *ct_zone, *ct_zone_next; > int scan_start = 1; > @@ -563,6 +564,7 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, > } > > /* Local patched datapath (gateway routers) need zones assigned. */ > + struct shash all_lds = SHASH_INITIALIZER(&all_lds); > const struct local_datapath *ld; > HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > /* XXX Add method to limit zone assignment to logical router > @@ -571,6 +573,8 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, > char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat"); > sset_add(&all_users, dnat); > sset_add(&all_users, snat); > + shash_add(&all_lds, dnat, ld); > + shash_add(&all_lds, snat, ld); > > int req_snat_zone = datapath_snat_ct_zone(ld->datapath); > if (req_snat_zone >= 0) { > @@ -636,6 +640,11 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, > > bitmap_set1(ct_zone_bitmap, snat_req_node->data); > simap_put(ct_zones, snat_req_node->name, snat_req_node->data); > + struct shash_node *ld_node = shash_find(&all_lds, snat_req_node->name); > + if (ld_node) { > + struct local_datapath *dp = ld_node->data; > + hmapx_add(updated_dps, (void *) dp->datapath); > + } > } > > /* xxx This is wasteful to assign a zone to each port--even if no > @@ -664,10 +673,17 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, > > bitmap_set1(ct_zone_bitmap, zone); > simap_put(ct_zones, user, zone); > + > + struct shash_node *ld_node = shash_find(&all_lds, user); > + if (ld_node) { > + struct local_datapath *dp = ld_node->data; > + hmapx_add(updated_dps, (void *) dp->datapath); > + } > } > > simap_destroy(&req_snat_zones); > sset_destroy(&all_users); > + shash_destroy(&all_lds); > } > > static void > @@ -1098,6 +1114,9 @@ struct ed_type_runtime_data { > bool tracked; > bool local_lports_changed; > struct hmap tracked_dp_bindings; > + > + /* CT zone data. Contains datapaths that had updated CT zones */ > + struct hmapx ct_updated_datapaths; Thanks for the fix. The patch overall looks fine to me, except the above hmax - ct_updated_datapaths. Since this hmapx is modified by en_ct_zone_run(), it should be part of ct_zone engine data. Since flow_output engine node also has ct_zone as input, It can access the hmapx directly. I went ahead and applied this patch to fix the 159 test case failure with the below changes. I also added Fixes tag in the commit message. **** diff --git a/controller/physical.h b/controller/physical.h index 0a4c3bab80..0bf13f2683 100644 --- a/controller/physical.h +++ b/controller/physical.h @@ -64,8 +64,8 @@ void physical_run(struct physical_ctx *, struct ovn_desired_flow_table *); void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *); void physical_clear_dp_flows(struct physical_ctx *p_ctx, - struct hmapx *ct_updated_datapaths, - struct ovn_desired_flow_table *flow_table); + struct hmapx *ct_updated_datapaths, + struct ovn_desired_flow_table *flow_table); void physical_handle_port_binding_changes(struct physical_ctx *, struct ovn_desired_flow_table *); void physical_handle_mc_group_changes(struct physical_ctx *, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 53cd1be869..b0f197550c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1269,6 +1269,7 @@ en_runtime_data_cleanup(void *data) } hmap_destroy(&rt_data->local_datapaths); local_bindings_destroy(&rt_data->local_bindings); + hmapx_destroy(&rt_data->ct_updated_datapaths); } ***** Valgrind reported a memory leak. So I added hmapx_destroy during cleanup. I will submit a follow up patch to move the hmapx to ct_zone engine. Thanks Numan > }; > > /* struct ed_type_runtime_data has the below members for tracking the > @@ -1189,6 +1208,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, > /* Init the tracked data. */ > hmap_init(&data->tracked_dp_bindings); > > + hmapx_init(&data->ct_updated_datapaths); > + > return data; > } > > @@ -1348,6 +1369,7 @@ en_runtime_data_run(struct engine_node *node, void *data) > sset_init(&rt_data->egress_ifaces); > smap_init(&rt_data->local_iface_ids); > local_bindings_init(&rt_data->local_bindings); > + hmapx_clear(&rt_data->ct_updated_datapaths); > } > > struct binding_ctx_in b_ctx_in; > @@ -1486,9 +1508,11 @@ en_ct_zones_run(struct engine_node *node, void *data) > struct ed_type_runtime_data *rt_data = > engine_get_input_data("runtime_data", node); > > + hmapx_clear(&rt_data->ct_updated_datapaths); > update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths, > &ct_zones_data->current, ct_zones_data->bitmap, > - &ct_zones_data->pending); > + &ct_zones_data->pending, &rt_data->ct_updated_datapaths); > + > > engine_set_node_state(node, EN_UPDATED); > } > @@ -1698,6 +1722,7 @@ static void init_physical_ctx(struct engine_node *node, > p_ctx->ct_zones = ct_zones; > p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; > p_ctx->local_bindings = &rt_data->local_bindings; > + p_ctx->ct_updated_datapaths = &rt_data->ct_updated_datapaths; > } > > static void init_lflow_ctx(struct engine_node *node, > @@ -2125,6 +2150,8 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > if (pfc_data->recompute_physical_flows) { > /* This indicates that we need to recompute the physical flows. */ > physical_clear_unassoc_flows_with_db(&fo->flow_table); > + physical_clear_dp_flows(&p_ctx, &rt_data->ct_updated_datapaths, > + &fo->flow_table); > physical_run(&p_ctx, &fo->flow_table); > return true; > } > diff --git a/controller/physical.c b/controller/physical.c > index 00c4ca4fd..fa5d0d692 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -44,6 +44,7 @@ > #include "sset.h" > #include "util.h" > #include "vswitch-idl.h" > +#include "hmapx.h" > > VLOG_DEFINE_THIS_MODULE(physical); > > @@ -860,6 +861,28 @@ load_logical_ingress_metadata(const struct sbrec_port_binding *binding, > put_load(port_key, MFF_LOG_INPORT, 0, 32, ofpacts_p); > } > > +static const struct sbrec_port_binding * > +get_binding_peer(struct ovsdb_idl_index *sbrec_port_binding_by_name, > + const struct sbrec_port_binding *binding) > +{ > + const char *peer_name = smap_get(&binding->options, "peer"); > + if (!peer_name) { > + return NULL; > + } > + > + const struct sbrec_port_binding *peer = lport_lookup_by_name( > + sbrec_port_binding_by_name, peer_name); > + if (!peer || strcmp(peer->type, binding->type)) { > + return NULL; > + } > + const char *peer_peer_name = smap_get(&peer->options, "peer"); > + if (!peer_peer_name || strcmp(peer_peer_name, binding->logical_port)) { > + return NULL; > + } > + > + return peer; > +} > + > static void > consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > enum mf_field_id mff_ovn_geneve, > @@ -882,18 +905,10 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > if (!strcmp(binding->type, "patch") > || (!strcmp(binding->type, "l3gateway") > && binding->chassis == chassis)) { > - const char *peer_name = smap_get(&binding->options, "peer"); > - if (!peer_name) { > - return; > - } > > - const struct sbrec_port_binding *peer = lport_lookup_by_name( > - sbrec_port_binding_by_name, peer_name); > - if (!peer || strcmp(peer->type, binding->type)) { > - return; > - } > - const char *peer_peer_name = smap_get(&peer->options, "peer"); > - if (!peer_peer_name || strcmp(peer_peer_name, binding->logical_port)) { > + const struct sbrec_port_binding *peer = get_binding_peer( > + sbrec_port_binding_by_name, binding); > + if (!peer) { > return; > } > > @@ -926,7 +941,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100, > binding->header_.uuid.parts[0], > - &match, ofpacts_p, hc_uuid); > + &match, ofpacts_p, &binding->header_.uuid); > return; > } > > @@ -1874,3 +1889,22 @@ physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *flow_table) > ofctrl_remove_flows(flow_table, hc_uuid); > } > } > + > +void > +physical_clear_dp_flows(struct physical_ctx *p_ctx, > + struct hmapx *ct_updated_datapaths, > + struct ovn_desired_flow_table *flow_table) > +{ > + const struct sbrec_port_binding *binding; > + SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, p_ctx->port_binding_table) { > + if (!hmapx_find(ct_updated_datapaths, binding->datapath)) { > + continue; > + } > + const struct sbrec_port_binding *peer = > + get_binding_peer(p_ctx->sbrec_port_binding_by_name, binding); > + ofctrl_remove_flows(flow_table, &binding->header_.uuid); > + if (peer) { > + ofctrl_remove_flows(flow_table, &peer->header_.uuid); > + } > + } > +} > diff --git a/controller/physical.h b/controller/physical.h > index feab41df4..0a4c3bab8 100644 > --- a/controller/physical.h > +++ b/controller/physical.h > @@ -56,12 +56,16 @@ struct physical_ctx { > const struct simap *ct_zones; > enum mf_field_id mff_ovn_geneve; > struct shash *local_bindings; > + struct hmapx *ct_updated_datapaths; > }; > > void physical_register_ovs_idl(struct ovsdb_idl *); > void physical_run(struct physical_ctx *, > struct ovn_desired_flow_table *); > void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *); > +void physical_clear_dp_flows(struct physical_ctx *p_ctx, > + struct hmapx *ct_updated_datapaths, > + struct ovn_desired_flow_table *flow_table); > void physical_handle_port_binding_changes(struct physical_ctx *, > struct ovn_desired_flow_table *); > void physical_handle_mc_group_changes(struct physical_ctx *, > -- > 2.25.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 25de0c72f..eceb804ac 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -64,6 +64,7 @@ #include "timer.h" #include "stopwatch.h" #include "lib/inc-proc-eng.h" +#include "hmapx.h" VLOG_DEFINE_THIS_MODULE(main); @@ -549,7 +550,7 @@ add_pending_ct_zone_entry(struct shash *pending_ct_zones, static void update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, struct simap *ct_zones, unsigned long *ct_zone_bitmap, - struct shash *pending_ct_zones) + struct shash *pending_ct_zones, struct hmapx *updated_dps) { struct simap_node *ct_zone, *ct_zone_next; int scan_start = 1; @@ -563,6 +564,7 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, } /* Local patched datapath (gateway routers) need zones assigned. */ + struct shash all_lds = SHASH_INITIALIZER(&all_lds); const struct local_datapath *ld; HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { /* XXX Add method to limit zone assignment to logical router @@ -571,6 +573,8 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat"); sset_add(&all_users, dnat); sset_add(&all_users, snat); + shash_add(&all_lds, dnat, ld); + shash_add(&all_lds, snat, ld); int req_snat_zone = datapath_snat_ct_zone(ld->datapath); if (req_snat_zone >= 0) { @@ -636,6 +640,11 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, bitmap_set1(ct_zone_bitmap, snat_req_node->data); simap_put(ct_zones, snat_req_node->name, snat_req_node->data); + struct shash_node *ld_node = shash_find(&all_lds, snat_req_node->name); + if (ld_node) { + struct local_datapath *dp = ld_node->data; + hmapx_add(updated_dps, (void *) dp->datapath); + } } /* xxx This is wasteful to assign a zone to each port--even if no @@ -664,10 +673,17 @@ update_ct_zones(const struct sset *lports, const struct hmap *local_datapaths, bitmap_set1(ct_zone_bitmap, zone); simap_put(ct_zones, user, zone); + + struct shash_node *ld_node = shash_find(&all_lds, user); + if (ld_node) { + struct local_datapath *dp = ld_node->data; + hmapx_add(updated_dps, (void *) dp->datapath); + } } simap_destroy(&req_snat_zones); sset_destroy(&all_users); + shash_destroy(&all_lds); } static void @@ -1098,6 +1114,9 @@ struct ed_type_runtime_data { bool tracked; bool local_lports_changed; struct hmap tracked_dp_bindings; + + /* CT zone data. Contains datapaths that had updated CT zones */ + struct hmapx ct_updated_datapaths; }; /* struct ed_type_runtime_data has the below members for tracking the @@ -1189,6 +1208,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, /* Init the tracked data. */ hmap_init(&data->tracked_dp_bindings); + hmapx_init(&data->ct_updated_datapaths); + return data; } @@ -1348,6 +1369,7 @@ en_runtime_data_run(struct engine_node *node, void *data) sset_init(&rt_data->egress_ifaces); smap_init(&rt_data->local_iface_ids); local_bindings_init(&rt_data->local_bindings); + hmapx_clear(&rt_data->ct_updated_datapaths); } struct binding_ctx_in b_ctx_in; @@ -1486,9 +1508,11 @@ en_ct_zones_run(struct engine_node *node, void *data) struct ed_type_runtime_data *rt_data = engine_get_input_data("runtime_data", node); + hmapx_clear(&rt_data->ct_updated_datapaths); update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths, &ct_zones_data->current, ct_zones_data->bitmap, - &ct_zones_data->pending); + &ct_zones_data->pending, &rt_data->ct_updated_datapaths); + engine_set_node_state(node, EN_UPDATED); } @@ -1698,6 +1722,7 @@ static void init_physical_ctx(struct engine_node *node, p_ctx->ct_zones = ct_zones; p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; p_ctx->local_bindings = &rt_data->local_bindings; + p_ctx->ct_updated_datapaths = &rt_data->ct_updated_datapaths; } static void init_lflow_ctx(struct engine_node *node, @@ -2125,6 +2150,8 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) if (pfc_data->recompute_physical_flows) { /* This indicates that we need to recompute the physical flows. */ physical_clear_unassoc_flows_with_db(&fo->flow_table); + physical_clear_dp_flows(&p_ctx, &rt_data->ct_updated_datapaths, + &fo->flow_table); physical_run(&p_ctx, &fo->flow_table); return true; } diff --git a/controller/physical.c b/controller/physical.c index 00c4ca4fd..fa5d0d692 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -44,6 +44,7 @@ #include "sset.h" #include "util.h" #include "vswitch-idl.h" +#include "hmapx.h" VLOG_DEFINE_THIS_MODULE(physical); @@ -860,6 +861,28 @@ load_logical_ingress_metadata(const struct sbrec_port_binding *binding, put_load(port_key, MFF_LOG_INPORT, 0, 32, ofpacts_p); } +static const struct sbrec_port_binding * +get_binding_peer(struct ovsdb_idl_index *sbrec_port_binding_by_name, + const struct sbrec_port_binding *binding) +{ + const char *peer_name = smap_get(&binding->options, "peer"); + if (!peer_name) { + return NULL; + } + + const struct sbrec_port_binding *peer = lport_lookup_by_name( + sbrec_port_binding_by_name, peer_name); + if (!peer || strcmp(peer->type, binding->type)) { + return NULL; + } + const char *peer_peer_name = smap_get(&peer->options, "peer"); + if (!peer_peer_name || strcmp(peer_peer_name, binding->logical_port)) { + return NULL; + } + + return peer; +} + static void consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, enum mf_field_id mff_ovn_geneve, @@ -882,18 +905,10 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, if (!strcmp(binding->type, "patch") || (!strcmp(binding->type, "l3gateway") && binding->chassis == chassis)) { - const char *peer_name = smap_get(&binding->options, "peer"); - if (!peer_name) { - return; - } - const struct sbrec_port_binding *peer = lport_lookup_by_name( - sbrec_port_binding_by_name, peer_name); - if (!peer || strcmp(peer->type, binding->type)) { - return; - } - const char *peer_peer_name = smap_get(&peer->options, "peer"); - if (!peer_peer_name || strcmp(peer_peer_name, binding->logical_port)) { + const struct sbrec_port_binding *peer = get_binding_peer( + sbrec_port_binding_by_name, binding); + if (!peer) { return; } @@ -926,7 +941,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100, binding->header_.uuid.parts[0], - &match, ofpacts_p, hc_uuid); + &match, ofpacts_p, &binding->header_.uuid); return; } @@ -1874,3 +1889,22 @@ physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *flow_table) ofctrl_remove_flows(flow_table, hc_uuid); } } + +void +physical_clear_dp_flows(struct physical_ctx *p_ctx, + struct hmapx *ct_updated_datapaths, + struct ovn_desired_flow_table *flow_table) +{ + const struct sbrec_port_binding *binding; + SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, p_ctx->port_binding_table) { + if (!hmapx_find(ct_updated_datapaths, binding->datapath)) { + continue; + } + const struct sbrec_port_binding *peer = + get_binding_peer(p_ctx->sbrec_port_binding_by_name, binding); + ofctrl_remove_flows(flow_table, &binding->header_.uuid); + if (peer) { + ofctrl_remove_flows(flow_table, &peer->header_.uuid); + } + } +} diff --git a/controller/physical.h b/controller/physical.h index feab41df4..0a4c3bab8 100644 --- a/controller/physical.h +++ b/controller/physical.h @@ -56,12 +56,16 @@ struct physical_ctx { const struct simap *ct_zones; enum mf_field_id mff_ovn_geneve; struct shash *local_bindings; + struct hmapx *ct_updated_datapaths; }; void physical_register_ovs_idl(struct ovsdb_idl *); void physical_run(struct physical_ctx *, struct ovn_desired_flow_table *); void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *); +void physical_clear_dp_flows(struct physical_ctx *p_ctx, + struct hmapx *ct_updated_datapaths, + struct ovn_desired_flow_table *flow_table); void physical_handle_port_binding_changes(struct physical_ctx *, struct ovn_desired_flow_table *); void physical_handle_mc_group_changes(struct physical_ctx *,
In commit f9cab11d5fabe2ae321a3b4bad5972b61df958c0, a LOG_TO_PHY flow was changed so that it was no longer associated with a particular port binding. The logic there was that the particular flow contains data pertaining to the port binding's peer's datapath, so it didn't make sense to associate the flow with the port binding. This change was necessary in order for flows to be recalculated properly if the requested SNAT CT zone on a gateway router was changed. Since the datapath was changed but no port bindings were changed, that particular flow needed to be cleared so it could be recalculated with the new CT zones put in place. Unfortunately, that change broke some other behavior. Specifically, if a router was changed from a distributed router to a gateway router, then its port bindings and its port bindings' peers would be updated so that they were no longer type "patch" but instead type "l3gateway". They would attempt to remove all associated physical flows and then install the newly relevant ones. Since the LOG_TO_PHY flow was no longer associated with a port binding, that flow would remain. The result was that traffic could be sent to the gateway router on chassis where the gateway router was not pinned. This commit seeks to fix both behaviors. Now if CT zones are changed on a particular datapath, then all port bindings on that datapath, as well as all of those port bindings' peers will have their physical flows removed. When physical flows are recomputed, all of the appropriate flows will be added. Signed-off-by: Mark Michelson <mmichels@redhat.com> --- controller/ovn-controller.c | 31 ++++++++++++++++++-- controller/physical.c | 58 +++++++++++++++++++++++++++++-------- controller/physical.h | 4 +++ 3 files changed, 79 insertions(+), 14 deletions(-)