diff mbox series

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

Message ID 1578959263-113782-1-git-send-email-hzhou@ovn.org
State New
Headers show
Series [ovs-dev,ovn,v2] ofctrl.c: Update installed OVS flow cookie when lflow is changed. | expand

Commit Message

Han Zhou Jan. 13, 2020, 11:47 p.m. UTC
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@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(-)

Comments

Mark Michelson Jan. 20, 2020, 7:01 p.m. UTC | #1
Acked-by: Mark Michelson <mmichels@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@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
>
Han Zhou Jan. 20, 2020, 11:13 p.m. UTC | #2
On Mon, Jan 20, 2020 at 11:01 AM Mark Michelson <mmichels@redhat.com> wrote:
>
> Acked-by: Mark Michelson <mmichels@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@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
> >
>

Thanks Mark for the review, and thanks Ben for explaining the background of
cookie update as well.
I applied this to master.
diff mbox series

Patch

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