Skip to content

Commit

Permalink
Clear port binding flows when datapath CT zone changes.
Browse files Browse the repository at this point in the history 
In commit f9cab11, 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.

Fixes: f9cab11("Allow explicit setting of the SNAT zone on a gateway router.")
Signed-off-by: Mark Michelson <[email protected]>
Signed-off-by: Numan Siddique <[email protected]>
  • Loading branch information
@putnopvut @numansiddique
putnopvut authored and numansiddique committed Nov 24, 2020
1 parent 880dca9 commit 53f60c7
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 14 deletions.
32 changes: 30 additions & 2 deletions controller/ovn-controller.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
#include "timer.h"
#include "stopwatch.h"
#include "lib/inc-proc-eng.h"
#include "hmapx.h"

VLOG_DEFINE_THIS_MODULE(main);

Expand Down Expand Up @@ -551,7 +552,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;
Expand All @@ -565,6 +566,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
Expand All @@ -573,6 +575,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) {
Expand Down Expand Up @@ -638,6 +642,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
Expand Down Expand Up @@ -666,10 +675,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
Expand Down Expand Up @@ -1135,6 +1151,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
Expand Down Expand Up @@ -1226,6 +1245,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;
}

Expand All @@ -1248,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);
}

static void
Expand Down Expand Up @@ -1385,6 +1407,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;
Expand Down Expand Up @@ -1523,9 +1546,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);
}
Expand Down Expand Up @@ -1735,6 +1760,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,
Expand Down Expand Up @@ -2167,6 +2193,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;
}
Expand Down
58 changes: 46 additions & 12 deletions controller/physical.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "sset.h"
#include "util.h"
#include "vswitch-idl.h"
#include "hmapx.h"

VLOG_DEFINE_THIS_MODULE(physical);

Expand Down Expand Up @@ -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,
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
}
}
}
4 changes: 4 additions & 0 deletions controller/physical.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 *,
Expand Down

0 comments on commit 53f60c7

Please sign in to comment.