[ovs-dev] [PATCH ovn] ofctrl.c: Update installed OVS flow cookie when lflow is changed.
Han Zhou
hzhou at ovn.org
Sun Jan 12 22:48:10 UTC 2020
On Sun, Jan 12, 2020 at 2:10 PM Han Zhou <hzhou at 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 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,
> };
> + /* 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
More information about the dev
mailing list