| Message ID | 1578867049-117521-1-git-send-email-hzhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn] ofctrl.c: Update installed OVS flow cookie when lflow is changed. | expand |
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
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 >
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 >
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 > > >
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.
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
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(-)