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

Message ID 1578867049-117521-1-git-send-email-hzhou@ovn.org
State New
Headers show
Series
  • [ovs-dev,ovn] ofctrl.c: Update installed OVS flow cookie when lflow is changed.
Related show

Commit Message

Han Zhou Jan. 12, 2020, 10:10 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, which effectively modifies existing flow
if a match is found.

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 controller/ofctrl.c | 12 ++++++++++--
 tests/ovn.at        | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

Comments

Han Zhou Jan. 12, 2020, 10:48 p.m. UTC | #1
On Sun, Jan 12, 2020 at 2:10 PM Han Zhou <hzhou@ovn.org> 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@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,
>                  };
> +                /* 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
> --
> 2.1.0
>

CC Ben

Hi Ben,

I need your help to confirm that it is ok to replace OFPFC_MODIFY_STRICT
with OFPFC_ADD for flow update, so that flow cookies can be updated. It
seems there is no other way to update cookie after OF1.1.
Although I see this in Documentation/topics/design.rst, which mentioned
that NXM supports updating cookie with OFPFC_MODIFY_STRICT when cookie is
not 'UNIT64_MAX'.
---------------
The following table shows the handling of different protocols when receiving
``OFPFC_MODIFY`` and ``OFPFC_MODIFY_STRICT`` messages.  A mask of 0
indicates
either an explicit mask of zero or an implicit one by not specifying the
``NXM_NX_COOKIE(_W)`` field.

==============  ======  ======  =============  =============
                Match   Update   Add on miss    Add on miss
                cookie  cookie     mask!=0        mask==0
==============  ======  ======  =============  =============
OpenFlow 1.0      no     yes    (add on miss)  (add on miss)
OpenFlow 1.1     yes      no         no             yes
OpenFlow 1.2     yes      no         no             no
NXM              yes     yes\*       no             yes
==============  ======  ======  =============  =============

\* Updates the flow's cookie unless the ``cookie`` field is ``UINT64_MAX``.
---------------
However, it didn't work when I try using OFPFC_MODIFY_STRICT, even if I set
fm.cookie = <new cookie> and fm.cookie_mask = UINT64_MAX. So I had to
change it to OFPFC_ADD to make it work.

I am not familiar with OF standard and its implementation in OVS, but the
change worked well with my tests. Could you help explain what's the side
effect of using OFPFC_ADD instead of OFPFC_MODIFY_STRICT?

Thanks,
Han
Mark Michelson Jan. 13, 2020, 8:45 p.m. UTC | #2
On 1/12/20 5:48 PM, Han Zhou wrote:
> On Sun, Jan 12, 2020 at 2:10 PM Han Zhou <hzhou@ovn.org> 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@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,
>>                   };
>> +                /* 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
>> --
>> 2.1.0
>>
> 
> CC Ben
> 
> Hi Ben,
> 
> I need your help to confirm that it is ok to replace OFPFC_MODIFY_STRICT
> with OFPFC_ADD for flow update, so that flow cookies can be updated. It
> seems there is no other way to update cookie after OF1.1.
> Although I see this in Documentation/topics/design.rst, which mentioned
> that NXM supports updating cookie with OFPFC_MODIFY_STRICT when cookie is
> not 'UNIT64_MAX'.
> ---------------
> The following table shows the handling of different protocols when receiving
> ``OFPFC_MODIFY`` and ``OFPFC_MODIFY_STRICT`` messages.  A mask of 0
> indicates
> either an explicit mask of zero or an implicit one by not specifying the
> ``NXM_NX_COOKIE(_W)`` field.
> 
> ==============  ======  ======  =============  =============
>                  Match   Update   Add on miss    Add on miss
>                  cookie  cookie     mask!=0        mask==0
> ==============  ======  ======  =============  =============
> OpenFlow 1.0      no     yes    (add on miss)  (add on miss)
> OpenFlow 1.1     yes      no         no             yes
> OpenFlow 1.2     yes      no         no             no
> NXM              yes     yes\*       no             yes
> ==============  ======  ======  =============  =============
> 
> \* Updates the flow's cookie unless the ``cookie`` field is ``UINT64_MAX``.
> ---------------
> However, it didn't work when I try using OFPFC_MODIFY_STRICT, even if I set
> fm.cookie = <new cookie> and fm.cookie_mask = UINT64_MAX. So I had to
> change it to OFPFC_ADD to make it work.

Hi Han, I decided to look into this, and according to the OpenFlow 
protocol version 1.1.0 up through 1.5.0, the following language exists 
in some section:

"For modify requests (OFPFC_MODIFY or OFPFC_MODIFY_STRICT), if a 
matching entry exists in the table, the instructions field of this entry 
is updated with the value from the request, whereas its
cookie, idle_timeout, hard_timeout, flags, counters and duration fields 
are left unchanged."

In OpenFlow 1.1.0, the section ends with:

"For modify requests, if no flow currently residing in the requested 
table matches the request, and if the cookie_mask field contains 0, the 
modify acts like an add, and the new flow entry must be inserted with 
zeroed counters"

However, in OpenFlow 1.2.0 onwards, the section ends with:

"For modify requests, if no flow entry currently residing in the 
requested table matches the request, no error is recorded, and no flow 
table modification occurs."

So it appears that with 1.1.0, modification with a zeroed cookie mask 
resulted in the equivalent OFPFC_ADD. In 1.2.0 onwards this is not the case.

> 
> I am not familiar with OF standard and its implementation in OVS, but the
> change worked well with my tests. Could you help explain what's the side
> effect of using OFPFC_ADD instead of OFPFC_MODIFY_STRICT?

 From an OpenFlow point of view, this will reset statistics on the flow 
(idle time, packet count, etc.) but otherwise has no negative effects. 
 From an OVS point of view, I'm not sure how it is different from flow 
modification. I would suspect that both might result in invalidation of 
cached megaflows though.

> 
> Thanks,
> Han
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Mark Michelson Jan. 13, 2020, 8:50 p.m. UTC | #3
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@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.

>                   };
> +                /* 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
>
Han Zhou Jan. 13, 2020, 11:47 p.m. UTC | #4
Hi Mark,

Thanks for your review.

On Mon, Jan 13, 2020 at 12:50 PM Mark Michelson <mmichels@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@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
> >
>
Ben Pfaff Jan. 16, 2020, 7:33 p.m. UTC | #5
On Sun, Jan 12, 2020 at 02:48:10PM -0800, Han Zhou wrote:
> On Sun, Jan 12, 2020 at 2:10 PM Han Zhou <hzhou@ovn.org> 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@ovn.org>
>
> Hi Ben,
> 
> I need your help to confirm that it is ok to replace OFPFC_MODIFY_STRICT
> with OFPFC_ADD for flow update, so that flow cookies can be updated. It
> seems there is no other way to update cookie after OF1.1.

I think you are right.  The basic issue is that the OF1.1 flow_mod wire
format (which is also used unmodified by all later versions of OpenFlow)
has only one field for a flow cookie.  This means that this field can be
used for matching on a flow cookie or updating a flow cookie, but not
(reasonably) used for both purposes.  "modify_strict" matches on cookie,
so it doesn't update it; "add" sets a cookie, so it doesn't match on it.

> Although I see this in Documentation/topics/design.rst, which mentioned
> that NXM supports updating cookie with OFPFC_MODIFY_STRICT when cookie is
> not 'UNIT64_MAX'.

The NXM wire format does allow for this.  It is just as borked as the
other protocols in terms of the number of cookie fields, that is, just
one.  (That's because the same people designed it as the plain OpenFlow
wire format, with the same lack of insight.)  But later one it was
extended to allow for a special NXM "field" that matches on the cookie.
If this is used, then we magically have two fields instead of one and
both purposes can be satisfied.

> ---------------
> The following table shows the handling of different protocols when receiving
> ``OFPFC_MODIFY`` and ``OFPFC_MODIFY_STRICT`` messages.  A mask of 0
> indicates
> either an explicit mask of zero or an implicit one by not specifying the
> ``NXM_NX_COOKIE(_W)`` field.
> 
> ==============  ======  ======  =============  =============
>                 Match   Update   Add on miss    Add on miss
>                 cookie  cookie     mask!=0        mask==0
> ==============  ======  ======  =============  =============
> OpenFlow 1.0      no     yes    (add on miss)  (add on miss)
> OpenFlow 1.1     yes      no         no             yes
> OpenFlow 1.2     yes      no         no             no
> NXM              yes     yes\*       no             yes
> ==============  ======  ======  =============  =============
> 
> \* Updates the flow's cookie unless the ``cookie`` field is ``UINT64_MAX``.
> ---------------
> However, it didn't work when I try using OFPFC_MODIFY_STRICT, even if I set
> fm.cookie = <new cookie> and fm.cookie_mask = UINT64_MAX. So I had to
> change it to OFPFC_ADD to make it work.

Probably, that's because OVS doesn't really fully support the Nicira
extension flow mod in OpenFlow versions later than 1.0.  I guess that
you were modifying ovn-controller, which uses OpenFlow 1.3.  OVS doesn't
ever generate Nicira extension flow mods in OF1.3.  

In turn, that's because I didn't realize until this very moment that
there were NXM features that are not in OF1.3.  Honestly, I'd rather not
use Nicira extension flow mods outside of OF1.0.  They were supposed to
be a nasty kluge because OF1.0 was inadequate and not evolving quickly
enough.  OF1.3 is pretty good and I don't want to go back to a place
where OVS is a huge pile of extensions on top of the official protocols.

> I am not familiar with OF standard and its implementation in OVS, but the
> change worked well with my tests. Could you help explain what's the side
> effect of using OFPFC_ADD instead of OFPFC_MODIFY_STRICT?

I think it's pretty much what's listed in the table.  The reset of the
counters is the biggest thing.

Patch
diff mbox series

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,
                 };
+                /* 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