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

Han Zhou hzhou at ovn.org
Mon Jan 13 23:47:59 UTC 2020


Hi Mark,

Thanks for your review.

On Mon, Jan 13, 2020 at 12:50 PM Mark Michelson <mmichels at redhat.com> wrote:
>
> On 1/12/20 5:10 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, which effectively modifies existing flow
> > if a match is found.
> >
> > Signed-off-by: Han Zhou <hzhou at ovn.org>
> > ---
> >   controller/ofctrl.c | 12 ++++++++++--
> >   tests/ovn.at        | 36 ++++++++++++++++++++++++++++++++++++
> >   2 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index 10edd84..6f2c530 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,
> > @@ -1184,8 +1186,14 @@ ofctrl_put(struct ovn_desired_flow_table
*flow_table,
> >                       .table_id = i->table_id,
> >                       .ofpacts = d->ofpacts,
> >                       .ofpacts_len = d->ofpacts_len,
> > -                    .command = OFPFC_MODIFY_STRICT,
> > +                    .command = OFPFC_ADD,
>
> I think the command only needs to be OFPFC_ADD if the cookie is being
> updated. Otherwise, we'll end up reseting packet counts, idle timeout,
> etc. on flows when it's not necessary.
>
Thanks for pointing this out. I didn't thought about the counters.
I sent out v2:
https://patchwork.ozlabs.org/patch/1222394/

> >                   };
> > +                /* Update cookie if it is changed. */
> > +                if (i->cookie != d->cookie) {
> > +                    fm.modify_cookie = true;
> > +                    fm.new_cookie = htonll(d->cookie);
> > +                    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