[ovs-dev] [PATCH ovn v2] ofctrl.c: Update installed OVS flow cookie when lflow is changed.

Mark Michelson mmichels at redhat.com
Mon Jan 20 19:01:26 UTC 2020


Acked-by: Mark Michelson <mmichels at redhat.com>

On 1/13/20 6:47 PM, Han Zhou wrote:
> When an old lflow is replaced by a new lflow, if the OVS flows
> translated by the old and new lflows have same match, ofctrl will
> update existing OVS flow instead of deleting and adding a new one.
> 
> However, when updating the existing flows, the cookie was not updated
> by current implementation, which results in discrepency between lflows
> and OVS flows, making debugging difficult and confuses tools such as
> ovn-trace. This patch fixes it.
> 
> Note: since command OFPFC_MODIFY_STRICT in FLOW_MOD message doesn't
> support updating flow cookie after OpenFlow 1.1, this patch changes
> to use OFPFC_ADD command whenever cookie needs to be updated, which
> effectively modifies existing flow if a match is found.
> 
> Signed-off-by: Han Zhou <hzhou at ovn.org>
> ---
> v1->v2: According to Mark Michelson's comment, use OFPFC_MODIFY_STRICT
>          when cookie doesn't need to be updated.
> 
>   controller/ofctrl.c | 12 +++++++++++-
>   tests/ovn.at        | 36 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 10edd84..36e39ba 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -828,6 +828,7 @@ ovn_flow_to_string(const struct ovn_flow *f)
>   {
>       struct ds s = DS_EMPTY_INITIALIZER;
>       ds_put_format(&s, "sb_uuid="UUID_FMT", ", UUID_ARGS(&f->sb_uuid));
> +    ds_put_format(&s, "cookie=%"PRIx64", ", f->cookie);
>       ds_put_format(&s, "table_id=%"PRIu8", ", f->table_id);
>       ds_put_format(&s, "priority=%"PRIu16", ", f->priority);
>       minimatch_format(&f->match, NULL, NULL, &s, OFP_DEFAULT_PRIORITY);
> @@ -1176,7 +1177,8 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table,
>                   i->sb_uuid = d->sb_uuid;
>               }
>               if (!ofpacts_equal(i->ofpacts, i->ofpacts_len,
> -                               d->ofpacts, d->ofpacts_len)) {
> +                               d->ofpacts, d->ofpacts_len) ||
> +                i->cookie != d->cookie) {
>                   /* Update actions in installed flow. */
>                   struct ofputil_flow_mod fm = {
>                       .match = i->match,
> @@ -1186,6 +1188,14 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table,
>                       .ofpacts_len = d->ofpacts_len,
>                       .command = OFPFC_MODIFY_STRICT,
>                   };
> +                /* Update cookie if it is changed. */
> +                if (i->cookie != d->cookie) {
> +                    fm.modify_cookie = true;
> +                    fm.new_cookie = htonll(d->cookie);
> +                    /* Use OFPFC_ADD so that cookie can be updated. */
> +                    fm.command = OFPFC_ADD,
> +                    i->cookie = d->cookie;
> +                }
>                   add_flow_mod(&fm, &msgs);
>                   ovn_flow_log(i, "updating installed");
>   
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 411b768..adb677c 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -17338,3 +17338,39 @@ OVS_WAIT_UNTIL([
>   
>   OVN_CLEANUP([hv1])
>   AT_CLEANUP
> +
> +AT_SETUP([ovn -- trace when flow cookie updated])
> +AT_KEYWORDS([cookie])
> +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
> +ovs-vsctl add-port br-int vif1 -- \
> +    set interface vif1 external-ids:iface-id=lp1 ofport-request=1
> +
> +ovn-nbctl ls-add lsw0
> +ovn-nbctl lsp-add lsw0 lp1
> +ovn-nbctl lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.1"
> +ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1234' drop
> +
> +ovn-nbctl --wait=hv --timeout=3 sync
> +
> +# Trace with --ovs should see ovs flow related to the ACL
> +AT_CHECK([ovn-trace --ovs lsw0 'inport == "lp1" && eth.type == 0x1234' | grep "dl_type=0x1234" | grep "cookie"], [0], [ignore])
> +
> +# Replace the ACL with same match but different action.
> +ovn-nbctl acl-del lsw0 -- \
> +    acl-add lsw0 from-lport 1000 'eth.type == 0x1234' allow
> +
> +ovn-nbctl --wait=hv --timeout=3 sync
> +
> +# Trace with --ovs should still see the ovs flow related to the ACL, which
> +# means the OVS flow is updated with new cookie corresponding to the new lflow.
> +AT_CHECK([ovn-trace --ovs lsw0 'inport == "lp1" && eth.type == 0x1234' | grep "dl_type=0x1234 actions="], [0], [ignore])
> +
> +OVN_CLEANUP([hv1])
> +
> +AT_CLEANUP
> 



More information about the dev mailing list