On Fri, Nov 20, 2020 at 5:44 AM Mark Michelson <[email protected]> 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 <[email protected]>
> ---
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
