[ovs-dev] [PATCH ovn v2] ovn-controller: Fix port group I-P when they contain non-vif ports.
Han Zhou
hzhou at ovn.org
Wed Jun 30 23:03:25 UTC 2021
On Wed, Jun 30, 2021 at 7:01 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> It's valid that port_groups contain non-vif ports, they can actually
> contain any type of logical_switch_port.
>
> Also, there's no need to allocate a new sset containing the local ports'
> names every time the I-P engine processes a change. We were already
> maintaining a set of "local_lport_ids". These correspond to port
> bindings that are relevant locally (including non-vif ports). Extend
> it to include the locally relevant lport names too and rename the
> structure an its helper functions to related_lport*.
>
> Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163
> Reported-by: Antonio Ojea <aojea at redhat.com>
> Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow
explosion problem.")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
> v2:
> - Addressed Numan's and Han's comments:
> - add struct related_lports
> - add test case.
> ---
> controller/binding.c | 79 ++++++++++++++++++-------------------
> controller/binding.h | 31 ++++++++-------
> controller/lflow.c | 2 +-
> controller/lflow.h | 2 +-
> controller/ovn-controller.c | 48 +++++++++-------------
> tests/ovn.at | 44 +++++++++++++++++++++
> 6 files changed, 120 insertions(+), 86 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 7fde0fdbb..594babc98 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -531,38 +531,41 @@ remove_local_lports(const char *iface_id, struct
binding_ctx_out *b_ctx)
> }
> }
>
> -/* Add a port binding ID (of the form "dp-key"_"port-key") to the set of
local
> - * lport IDs. Also track if the set has changed.
> +/* Add a port binding to the set of locally relevant lports.
> + * Also track if the set has changed.
> */
> static void
> -update_local_lport_ids(const struct sbrec_port_binding *pb,
> - struct binding_ctx_out *b_ctx)
> +update_related_lport(const struct sbrec_port_binding *pb,
> + struct binding_ctx_out *b_ctx)
> {
> char buf[16];
> get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key,
> buf, sizeof(buf));
> - if (sset_add(b_ctx->local_lport_ids, buf) != NULL) {
> - b_ctx->local_lport_ids_changed = true;
> + if (sset_add(&b_ctx->related_lports->lport_ids, buf) != NULL) {
> + b_ctx->related_lports_changed = true;
>
> if (b_ctx->tracked_dp_bindings) {
> /* Add the 'pb' to the tracked_datapaths. */
> tracked_binding_datapath_lport_add(pb,
b_ctx->tracked_dp_bindings);
> }
> }
> + sset_add(&b_ctx->related_lports->lport_names, pb->logical_port);
> }
>
> -/* Remove a port binding id from the set of local lport IDs. Also track
if
> - * the set has changed.
> +/* Remove a port binding id from the set of locally relevant lports.
> + * Also track if the set has changed.
> */
> static void
> -remove_local_lport_ids(const struct sbrec_port_binding *pb,
> - struct binding_ctx_out *b_ctx)
> +remove_related_lport(const struct sbrec_port_binding *pb,
> + struct binding_ctx_out *b_ctx)
> {
> char buf[16];
> get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key,
> buf, sizeof(buf));
> - if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) {
> - b_ctx->local_lport_ids_changed = true;
> + sset_find_and_delete(&b_ctx->related_lports->lport_names,
> + pb->logical_port);
> + if (sset_find_and_delete(&b_ctx->related_lports->lport_ids, buf)) {
> + b_ctx->related_lports_changed = true;
>
> if (b_ctx->tracked_dp_bindings) {
> /* Add the 'pb' to the tracked_datapaths. */
> @@ -678,6 +681,20 @@ static struct binding_lport
*binding_lport_check_and_cleanup(
>
> static char *get_lport_type_str(enum en_lport_type lport_type);
>
> +void
> +related_lports_init(struct related_lports *rp)
> +{
> + sset_init(&rp->lport_names);
> + sset_init(&rp->lport_ids);
> +}
> +
> +void
> +related_lports_destroy(struct related_lports *rp)
> +{
> + sset_destroy(&rp->lport_names);
> + sset_destroy(&rp->lport_ids);
> +}
> +
> void
> local_binding_data_init(struct local_binding_data *lbinding_data)
> {
> @@ -1172,7 +1189,7 @@ release_binding_lport(const struct sbrec_chassis
*chassis_rec,
> struct binding_ctx_out *b_ctx_out)
> {
> if (is_binding_lport_this_chassis(b_lport, chassis_rec)) {
> - remove_local_lport_ids(b_lport->pb, b_ctx_out);
> + remove_related_lport(b_lport->pb, b_ctx_out);
> if (!release_lport(b_lport->pb, sb_readonly,
> b_ctx_out->tracked_dp_bindings,
> b_ctx_out->if_mgr)) {
> @@ -1214,7 +1231,7 @@ consider_vif_lport_(const struct sbrec_port_binding
*pb,
> pb->datapath, false,
> b_ctx_out->local_datapaths,
> b_ctx_out->tracked_dp_bindings);
> - update_local_lport_ids(pb, b_ctx_out);
> + update_related_lport(pb, b_ctx_out);
> update_local_lports(pb->logical_port, b_ctx_out);
> if (b_lport->lbinding->iface && qos_map &&
b_ctx_in->ovs_idl_txn) {
> get_qos_params(pb, qos_map);
> @@ -1405,7 +1422,7 @@ consider_virtual_lport(const struct
sbrec_port_binding *pb,
> * its entry from the local_lport_ids if present. This is required
> * when a virtual port moves from one chassis to other.*/
> if (!virtual_b_lport) {
> - remove_local_lport_ids(pb, b_ctx_out);
> + remove_related_lport(pb, b_ctx_out);
> }
>
> return true;
> @@ -1430,7 +1447,7 @@ consider_nonvif_lport_(const struct
sbrec_port_binding *pb,
> b_ctx_out->local_datapaths,
> b_ctx_out->tracked_dp_bindings);
>
> - update_local_lport_ids(pb, b_ctx_out);
> + update_related_lport(pb, b_ctx_out);
> return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL,
> !b_ctx_in->ovnsb_idl_txn, false,
> b_ctx_out->tracked_dp_bindings,
> @@ -1482,7 +1499,7 @@ consider_localnet_lport(const struct
sbrec_port_binding *pb,
> get_qos_params(pb, qos_map);
> }
>
> - update_local_lport_ids(pb, b_ctx_out);
> + update_related_lport(pb, b_ctx_out);
> }
>
> static bool
> @@ -1512,7 +1529,7 @@ consider_ha_lport(const struct sbrec_port_binding
*pb,
> pb->datapath, false,
> b_ctx_out->local_datapaths,
> b_ctx_out->tracked_dp_bindings);
> - update_local_lport_ids(pb, b_ctx_out);
> + update_related_lport(pb, b_ctx_out);
> }
>
> return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in,
b_ctx_out);
> @@ -1634,7 +1651,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct
binding_ctx_out *b_ctx_out)
> case LP_PATCH:
> case LP_LOCALPORT:
> case LP_VTEP:
> - update_local_lport_ids(pb, b_ctx_out);
> + update_related_lport(pb, b_ctx_out);
> break;
>
> case LP_VIF:
> @@ -1895,7 +1912,7 @@ remove_pb_from_local_datapath(const struct
sbrec_port_binding *pb,
> struct binding_ctx_out *b_ctx_out,
> struct local_datapath *ld)
> {
> - remove_local_lport_ids(pb, b_ctx_out);
> + remove_related_lport(pb, b_ctx_out);
> if (!strcmp(pb->type, "patch") ||
> !strcmp(pb->type, "l3gateway")) {
> remove_local_datapath_peer_port(pb, ld,
b_ctx_out->local_datapaths);
> @@ -2502,7 +2519,7 @@ delete_done:
> case LP_PATCH:
> case LP_LOCALPORT:
> case LP_VTEP:
> - update_local_lport_ids(pb, b_ctx_out);
> + update_related_lport(pb, b_ctx_out);
> if (lport_type == LP_PATCH) {
> if (!ld) {
> /* If 'ld' for this lport is not present, then check
if
> @@ -2926,23 +2943,3 @@ cleanup:
>
> return b_lport;
> }
> -
> -struct sset *
> -binding_collect_local_binding_lports(struct local_binding_data
*lbinding_data)
> -{
> - struct sset *lports = xzalloc(sizeof *lports);
> - sset_init(lports);
> - struct shash_node *shash_node;
> - SHASH_FOR_EACH (shash_node, &lbinding_data->lports) {
> - struct binding_lport *b_lport = shash_node->data;
> - sset_add(lports, b_lport->name);
> - }
> - return lports;
> -}
> -
> -void
> -binding_destroy_local_binding_lports(struct sset *lports)
> -{
> - sset_destroy(lports);
> - free(lports);
> -}
> diff --git a/controller/binding.h b/controller/binding.h
> index 8f3289476..a08011ae2 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -22,6 +22,7 @@
> #include "openvswitch/hmap.h"
> #include "openvswitch/uuid.h"
> #include "openvswitch/list.h"
> +#include "sset.h"
>
> struct hmap;
> struct ovsdb_idl;
> @@ -56,6 +57,19 @@ struct binding_ctx_in {
> const struct ovsrec_interface_table *iface_table;
> };
>
> +/* Locally relevant port bindings, e.g., VIFs that might be bound
locally,
> + * patch ports.
> + */
> +struct related_lports {
> + struct sset lport_names; /* Set of port names. */
> + struct sset lport_ids; /* Set of
<datapath-tunnel-key>_<port-tunnel-key>
> + * IDs for fast lookup.
> + */
> +};
> +
> +void related_lports_init(struct related_lports *);
> +void related_lports_destroy(struct related_lports *);
> +
> struct binding_ctx_out {
> struct hmap *local_datapaths;
> struct local_binding_data *lbinding_data;
> @@ -65,11 +79,9 @@ struct binding_ctx_out {
> /* Track if local_lports have been updated. */
> bool local_lports_changed;
>
> - /* sset of local lport ids in the format
> - * <datapath-tunnel-key>_<port-tunnel-key>. */
> - struct sset *local_lport_ids;
> - /* Track if local_lport_ids has been updated. */
> - bool local_lport_ids_changed;
> + /* Port bindings that are relevant to the local chassis. */
> + struct related_lports *related_lports;
> + bool related_lports_changed;
>
> /* Track if non-vif port bindings (e.g., patch, external) have been
> * added/deleted.
> @@ -133,13 +145,4 @@ bool binding_handle_port_binding_changes(struct
binding_ctx_in *,
> void binding_tracked_dp_destroy(struct hmap *tracked_datapaths);
>
> void binding_dump_local_bindings(struct local_binding_data *, struct ds
*);
> -
> -/* Generates a sset of lport names from local_binding_data.
> - * Note: the caller is responsible for destroying and freeing the
returned
> - * sset, by calling binding_detroy_local_binding_lports(). */
> -struct sset *binding_collect_local_binding_lports(struct
local_binding_data *);
> -
> -/* Destroy and free the lports sset returned by
> - * binding_collect_local_binding_lports(). */
> -void binding_destroy_local_binding_lports(struct sset *lports);
> #endif /* controller/binding.h */
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 34eca135a..abb01f0ce 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -625,7 +625,7 @@ add_matches_to_flow_table(const struct
sbrec_logical_flow *lflow,
> get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
> lflow_resource_add(l_ctx_out->lfrr,
REF_TYPE_PORTBINDING, buf,
> &lflow->header_.uuid);
> - if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
> + if (!sset_contains(l_ctx_in->related_lport_ids, buf)) {
> VLOG_DBG("lflow "UUID_FMT
> " port %s in match is not local, skip",
> UUID_ARGS(&lflow->header_.uuid),
> diff --git a/controller/lflow.h b/controller/lflow.h
> index e98edf81d..699f9c2d5 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -144,7 +144,7 @@ struct lflow_ctx_in {
> const struct shash *addr_sets;
> const struct shash *port_groups;
> const struct sset *active_tunnels;
> - const struct sset *local_lport_ids;
> + const struct sset *related_lport_ids;
> };
>
> struct lflow_ctx_out {
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index b6afb8fb9..3bb8b22eb 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1014,9 +1014,10 @@ struct ed_type_runtime_data {
> * local hypervisor, and localnet ports. */
> struct sset local_lports;
>
> - /* Contains the same ports as local_lports, but in the format:
> - * <datapath-tunnel-key>_<port-tunnel-key> */
> - struct sset local_lport_ids;
> + /* Port bindings that are relevant to the local chassis (VIFs bound
> + * localy, patch ports).
> + */
> + struct related_lports related_lports;
> struct sset active_tunnels;
>
> /* runtime data engine private data. */
> @@ -1109,7 +1110,7 @@ en_runtime_data_init(struct engine_node *node
OVS_UNUSED,
>
> hmap_init(&data->local_datapaths);
> sset_init(&data->local_lports);
> - sset_init(&data->local_lport_ids);
> + related_lports_init(&data->related_lports);
> sset_init(&data->active_tunnels);
> sset_init(&data->egress_ifaces);
> smap_init(&data->local_iface_ids);
> @@ -1127,7 +1128,7 @@ en_runtime_data_cleanup(void *data)
> struct ed_type_runtime_data *rt_data = data;
>
> sset_destroy(&rt_data->local_lports);
> - sset_destroy(&rt_data->local_lport_ids);
> + related_lports_destroy(&rt_data->related_lports);
> sset_destroy(&rt_data->active_tunnels);
> sset_destroy(&rt_data->egress_ifaces);
> smap_destroy(&rt_data->local_iface_ids);
> @@ -1219,8 +1220,8 @@ init_binding_ctx(struct engine_node *node,
> b_ctx_out->local_datapaths = &rt_data->local_datapaths;
> b_ctx_out->local_lports = &rt_data->local_lports;
> b_ctx_out->local_lports_changed = false;
> - b_ctx_out->local_lport_ids = &rt_data->local_lport_ids;
> - b_ctx_out->local_lport_ids_changed = false;
> + b_ctx_out->related_lports = &rt_data->related_lports;
> + b_ctx_out->related_lports_changed = false;
> b_ctx_out->non_vif_ports_changed = false;
> b_ctx_out->egress_ifaces = &rt_data->egress_ifaces;
> b_ctx_out->lbinding_data = &rt_data->lbinding_data;
> @@ -1235,7 +1236,6 @@ en_runtime_data_run(struct engine_node *node, void
*data)
> struct ed_type_runtime_data *rt_data = data;
> struct hmap *local_datapaths = &rt_data->local_datapaths;
> struct sset *local_lports = &rt_data->local_lports;
> - struct sset *local_lport_ids = &rt_data->local_lport_ids;
> struct sset *active_tunnels = &rt_data->active_tunnels;
>
> static bool first_run = true;
> @@ -1252,12 +1252,12 @@ en_runtime_data_run(struct engine_node *node,
void *data)
> hmap_clear(local_datapaths);
> local_binding_data_destroy(&rt_data->lbinding_data);
> sset_destroy(local_lports);
> - sset_destroy(local_lport_ids);
> + related_lports_destroy(&rt_data->related_lports);
> sset_destroy(active_tunnels);
> sset_destroy(&rt_data->egress_ifaces);
> smap_destroy(&rt_data->local_iface_ids);
> sset_init(local_lports);
> - sset_init(local_lport_ids);
> + related_lports_init(&rt_data->related_lports);
> sset_init(active_tunnels);
> sset_init(&rt_data->egress_ifaces);
> smap_init(&rt_data->local_iface_ids);
> @@ -1327,7 +1327,7 @@ runtime_data_sb_port_binding_handler(struct
engine_node *node, void *data)
> }
>
> rt_data->local_lports_changed = b_ctx_out.local_lports_changed;
> - if (b_ctx_out.local_lport_ids_changed ||
> + if (b_ctx_out.related_lports_changed ||
> b_ctx_out.non_vif_ports_changed ||
> b_ctx_out.local_lports_changed ||
> !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) {
> @@ -1638,11 +1638,8 @@ en_port_groups_run(struct engine_node *node, void
*data)
> struct ed_type_runtime_data *rt_data =
> engine_get_input_data("runtime_data", node);
>
> - struct sset *local_b_lports = binding_collect_local_binding_lports(
> - &rt_data->lbinding_data);
> - port_groups_init(pg_table, local_b_lports, &pg->port_group_ssets,
> - &pg->port_groups_cs_local);
> - binding_destroy_local_binding_lports(local_b_lports);
> + port_groups_init(pg_table, &rt_data->related_lports.lport_names,
> + &pg->port_group_ssets, &pg->port_groups_cs_local);
>
> engine_set_node_state(node, EN_UPDATED);
> }
> @@ -1659,12 +1656,9 @@ port_groups_sb_port_group_handler(struct
engine_node *node, void *data)
> struct ed_type_runtime_data *rt_data =
> engine_get_input_data("runtime_data", node);
>
> - struct sset *local_b_lports = binding_collect_local_binding_lports(
> - &rt_data->lbinding_data);
> - port_groups_update(pg_table, local_b_lports, &pg->port_group_ssets,
> - &pg->port_groups_cs_local, &pg->new, &pg->deleted,
> - &pg->updated);
> - binding_destroy_local_binding_lports(local_b_lports);
> + port_groups_update(pg_table, &rt_data->related_lports.lport_names,
> + &pg->port_group_ssets, &pg->port_groups_cs_local,
> + &pg->new, &pg->deleted, &pg->updated);
>
> if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) ||
> !sset_is_empty(&pg->updated)) {
> @@ -1697,9 +1691,6 @@ port_groups_runtime_data_handler(struct engine_node
*node, void *data)
> goto out;
> }
>
> - struct sset *local_b_lports = binding_collect_local_binding_lports(
> - &rt_data->lbinding_data);
> -
> const struct sbrec_port_group *pg_sb;
> SBREC_PORT_GROUP_TABLE_FOR_EACH (pg_sb, pg_table) {
> struct sset *pg_lports = shash_find_data(&pg->port_group_ssets,
> @@ -1726,13 +1717,12 @@ port_groups_runtime_data_handler(struct
engine_node *node, void *data)
> if (need_update) {
> expr_const_sets_add_strings(&pg->port_groups_cs_local,
pg_sb->name,
> (const char *const *)
pg_sb->ports,
> - pg_sb->n_ports, local_b_lports);
> + pg_sb->n_ports,
> +
&rt_data->related_lports.lport_names);
> sset_add(&pg->updated, pg_sb->name);
> }
> }
>
> - binding_destroy_local_binding_lports(local_b_lports);
> -
> out:
> if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) ||
> !sset_is_empty(&pg->updated)) {
> @@ -2042,7 +2032,7 @@ init_lflow_ctx(struct engine_node *node,
> l_ctx_in->addr_sets = addr_sets;
> l_ctx_in->port_groups = port_groups;
> l_ctx_in->active_tunnels = &rt_data->active_tunnels;
> - l_ctx_in->local_lport_ids = &rt_data->local_lport_ids;
> + l_ctx_in->related_lport_ids = &rt_data->related_lports.lport_ids;
>
> l_ctx_out->flow_table = &fo->flow_table;
> l_ctx_out->group_table = &fo->group_table;
> diff --git a/tests/ovn.at b/tests/ovn.at
> index db1a0a35c..7a718d4b6 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -26805,6 +26805,50 @@ OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
>
> +# Tests that ACLs referencing port groups that include ports connected to
> +# logical routers are correctly applied.
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn -- ACL with Port Group including router ports])
> +ovn_start
> +net_add n1
> +
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +check ovn-nbctl \
> + -- lr-add lr \
> + -- ls-add ls \
> + -- lrp-add lr lrp_ls 00:00:00:00:00:01 42.42.42.1/24 \
> + -- lsp-add ls ls_lr \
> + -- lsp-set-addresses ls_lr router \
> + -- lsp-set-type ls_lr router \
> + -- lsp-set-options ls_lr router-port=lr_ls \
> + -- lsp-add ls vm1
> +
> +check ovn-nbctl pg-add pg ls_lr \
> + -- acl-add pg from-lport 1 'inport == @pg && ip4.dst == 42.42.42.42'
drop
> +
> +check ovs-vsctl add-port br-int vm1 \
> + -- set interface vm1 external_ids:iface-id=vm1
> +
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=ls)
> +rtr_port_key=$(fetch_column Port_Binding tunnel_key logical_port=ls_lr)
> +
> +# Check that ovn-controller adds a flow to drop packets with dest IP
> +# 42.42.42.42 coming from the router port.
> +AT_CHECK([ovs-ofctl dump-flows br-int table=17 | grep
"reg14=0x${rtr_port_key},metadata=0x${dp_key},nw_dst=42.42.42.42
actions=drop" -c], [0], [dnl
> +1
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([ovn -- Static route with discard nexthop])
> ovn_start
> --
> 2.27.0
>
Thanks Dumitru!
Acked-by: Han Zhou <hzhou at ovn.org>
Not sure if Numan would like to take a second look as well, so let's wait
for one or two days before merging.
More information about the dev
mailing list