[ovs-dev] [PATCH v2 ovn] Save the addr set and port groups in lflow expr cache
Mark Michelson
mmichels at redhat.com
Tue Feb 18 21:30:23 UTC 2020
Hi, Numan. Would it be possible to add a test case that exercises the fix?
On 2/18/20 2:53 PM, numans at ovn.org wrote:
> From: Numan Siddique <numans at ovn.org>
>
> After the patch [1], which added caching of lflow expr, the lflow_resource_ref
> is not rebuilt properly when lflow_run() is called. If a lflow is already cached
> in lflow expr cache, then the lflow_resource_ref is not updated.
> But flow_output_run() clears the lflow_resource_ref cache before calling lflow_run().
>
> This results in incorrect OF flows present in the switch even if the
> address set/port group references have changed.
>
> This patch fixes this issue by saving the addr set and port group sets in
> the lfow expr cache and updating the lflow_resource_ref.
>
> [1] - 8795bec737b9("ovn-controller: Cache logical flow expr tree for each lflow.")
> Fixes: 8795bec737b9("ovn-controller: Cache logical flow expr tree for each lflow.")
>
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
> controller/lflow.c | 63 ++++++++++++++++++++++++++++++++++------------
> tests/ovn.at | 4 +++
> 2 files changed, 51 insertions(+), 16 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 79d89131a..110809df1 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -268,16 +268,21 @@ struct lflow_expr {
> struct hmap_node node;
> struct uuid lflow_uuid; /* key */
> struct expr *expr;
> + struct sset addr_sets_ref;
> + struct sset port_groups_ref;
> };
>
> static void
> lflow_expr_add(struct hmap *lflow_expr_cache,
> const struct sbrec_logical_flow *lflow,
> - struct expr *lflow_expr)
> + struct expr *lflow_expr, struct sset *addr_sets_ref,
> + struct sset *port_groups_ref)
> {
> struct lflow_expr *le = xmalloc(sizeof *le);
> le->lflow_uuid = lflow->header_.uuid;
> le->expr = lflow_expr;
> + sset_clone(&le->addr_sets_ref, addr_sets_ref);
> + sset_clone(&le->port_groups_ref, port_groups_ref);
> hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid));
> }
>
> @@ -301,6 +306,8 @@ lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
> {
> hmap_remove(lflow_expr_cache, &le->node);
> expr_destroy(le->expr);
> + sset_destroy(&le->addr_sets_ref);
> + sset_destroy(&le->port_groups_ref);
> free(le);
> }
>
> @@ -310,6 +317,8 @@ lflow_expr_destroy(struct hmap *lflow_expr_cache)
> struct lflow_expr *le;
> HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
> expr_destroy(le->expr);
> + sset_destroy(&le->addr_sets_ref);
> + sset_destroy(&le->port_groups_ref);
> free(le);
> }
>
> @@ -548,6 +557,25 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
> return true;
> }
>
> +static void
> +lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
> + struct sset *addr_sets_ref,
> + struct sset *port_groups_ref,
> + struct lflow_resource_ref *lfrr)
> +{
> + const char *addr_set_name;
> + SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
> + lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
> + &lflow->header_.uuid);
> + }
> +
> + const char *port_group_name;
> + SSET_FOR_EACH (port_group_name, port_groups_ref) {
> + lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
> + &lflow->header_.uuid);
> + }
> +}
> +
> static bool
> consider_logical_flow(const struct sbrec_logical_flow *lflow,
> struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> @@ -615,33 +643,27 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
> struct hmap matches;
> struct expr *expr = NULL;
>
> - struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> - struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
> if (le) {
> if (delete_expr_from_cache) {
> lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> + le = NULL;
> } else {
> expr = le->expr;
> }
> }
>
> if (!expr) {
> + struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> + struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> +
> expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets,
> l_ctx_in->port_groups, &addr_sets_ref,
> &port_groups_ref, &error);
> - const char *addr_set_name;
> - SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
> - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET,
> - addr_set_name, &lflow->header_.uuid);
> - }
> - const char *port_group_name;
> - SSET_FOR_EACH (port_group_name, &port_groups_ref) {
> - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
> - port_group_name, &lflow->header_.uuid);
> - }
> - sset_destroy(&addr_sets_ref);
> - sset_destroy(&port_groups_ref);
> + /* Add the address set and port groups (if any) to the lflow
> + * references. */
> + lflow_update_resource_refs(lflow, &addr_sets_ref, &port_groups_ref,
> + l_ctx_out->lfrr);
>
> if (!error) {
> if (prereqs) {
> @@ -658,15 +680,24 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
> free(error);
> ovnacts_free(ovnacts.data, ovnacts.size);
> ofpbuf_uninit(&ovnacts);
> + sset_destroy(&addr_sets_ref);
> + sset_destroy(&port_groups_ref);
> return true;
> }
>
> expr = expr_simplify(expr);
> expr = expr_normalize(expr);
>
> - lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr);
> + lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
> + &addr_sets_ref, &port_groups_ref);
> + sset_destroy(&addr_sets_ref);
> + sset_destroy(&port_groups_ref);
> } else {
> expr_destroy(prereqs);
> + /* Add the cached address set and port groups (if any) to the lflow
> + * references. */
> + lflow_update_resource_refs(lflow, &le->addr_sets_ref,
> + &le->port_groups_ref, l_ctx_out->lfrr);
> }
>
> struct condition_aux cond_aux = {
> diff --git a/tests/ovn.at b/tests/ovn.at
> index ea6760e1f..254645a3a 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13778,6 +13778,10 @@ for i in 1 2 3; do
> n_flows_after=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc -l`
> AT_CHECK([test $(expr $n_flows_before \* 2) = $n_flows_after], [0], [ignore])
>
> + # Trigger full recompute. Creating a chassis would trigger full recompute.
> + ovn-sbctl chassis-add tst geneve 127.0.0.4
> + ovn-sbctl chassis-del tst
> +
> # Remove an ACL
> ovn-nbctl --wait=hv acl-del ls1 to-lport 200 \
> 'outport=="lp2" && ip4 && ip4.src == $as1'
>
More information about the dev
mailing list