[ovs-dev] [PATCH ovn] ofctrl.c: Update installed OVS flow cookie when lflow is changed.
Mark Michelson
mmichels at redhat.com
Mon Jan 13 20:50:39 UTC 2020
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.
> };
> + /* 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