diff mbox

[ovs-dev,v3] ovn-controller: Fix duplicated flow add attempts in table 32.

Message ID 1482352336-14288-1-git-send-email-zhouhan@gmail.com
State Accepted
Headers show

Commit Message

Han Zhou Dec. 21, 2016, 8:32 p.m. UTC 
In commit 475f0a2c it introduced a priority 150 flow for filtering
the sending of traffic received from vxlan tunnels back out tunnels.
However, it added the flow for every remote port processing, which
results in continuous logs about duplicated flows. We only need to
install this flow once per physical_run() loop iteration.

Signed-off-by: Han Zhou <zhouhan@gmail.com>
Acked-by: Darrell Ball <dball@vmware.com>
---

Notes:
    v1 -> v2: update commit message according to Darrell's comments.
    v2 -> v3: update test case.

 ovn/controller/physical.c | 47 ++++++++++++++++++++++++-----------------------
 tests/ovn.at              |  5 +++++
 2 files changed, 29 insertions(+), 23 deletions(-)

Comments

Ben Pfaff Dec. 21, 2016, 9:36 p.m. UTC | #1 
On Wed, Dec 21, 2016 at 12:32:16PM -0800, Han Zhou wrote:
> In commit 475f0a2c it introduced a priority 150 flow for filtering
> the sending of traffic received from vxlan tunnels back out tunnels.
> However, it added the flow for every remote port processing, which
> results in continuous logs about duplicated flows. We only need to
> install this flow once per physical_run() loop iteration.
> 
> Signed-off-by: Han Zhou <zhouhan@gmail.com>
> Acked-by: Darrell Ball <dball@vmware.com>
> ---
> 
> Notes:
>     v1 -> v2: update commit message according to Darrell's comments.
>     v2 -> v3: update test case.

Thanks, applied to master.

(I reformatted a couple of comments.)
Darrell Ball Dec. 21, 2016, 9:45 p.m. UTC | #2 
On 12/21/16, 12:32 PM, "ovs-dev-bounces@openvswitch.org on behalf of Han Zhou" <ovs-dev-bounces@openvswitch.org on behalf of zhouhan@gmail.com> wrote:

    In commit 475f0a2c it introduced a priority 150 flow for filtering
    the sending of traffic received from vxlan tunnels back out tunnels.
    However, it added the flow for every remote port processing, which
    results in continuous logs about duplicated flows. We only need to
    install this flow once per physical_run() loop iteration.

I thought we covered this part in the last version.

s/However, it added the flow for every remote port processing, which
    results in continuous logs about duplicated flows. We only need to
    install this flow once per physical_run() loop iteration.
/
    However, it attempted to add the flow for every remote port processing,
    which results in continuous logs about duplicated flow add attempts. We
    only need to attempt to install this flow once per physical_run() loop iteration.
/
    
    Signed-off-by: Han Zhou <zhouhan@gmail.com>
    Acked-by: Darrell Ball <dball@vmware.com>
    ---
    
    Notes:
        v1 -> v2: update commit message according to Darrell's comments.
        v2 -> v3: update test case.
    
     ovn/controller/physical.c | 47 ++++++++++++++++++++++++-----------------------
     tests/ovn.at              |  5 +++++
     2 files changed, 29 insertions(+), 23 deletions(-)
    
    diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
    index 48adb78..3b653dd 100644
    --- a/ovn/controller/physical.c
    +++ b/ovn/controller/physical.c
    @@ -465,33 +465,16 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
         } else {
             /* Remote port connected by tunnel */
     
    -        /* Table 32, priority 150 and 100.
    +        /* Table 32, priority 100.
              * ===============================
              *
    -         * Priority 150 is for packets received from a VXLAN tunnel
    -         * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to
    -         * lack of needed metadata in VXLAN, explicitly skip sending
    -         * back out any tunnels and resubmit to table 33 for local
    -         * delivery.
    -         *
    -         * Priority 100 is for all other traffic which need to be sent
    -         * to a remote hypervisor.  Each flow matches an output port
    -         * that includes a logical port on a remote hypervisor, and
    -         * tunnels the packet to that hypervisor.
    +         * Priority 100 is for traffic that needs to be sent to a remote
    +         * hypervisor.  Each flow matches an output port that includes a
    +         * logical port on a remote hypervisor, and tunnels the packet to
    +         * that hypervisor.
              */
             match_init_catchall(&match);
             ofpbuf_clear(ofpacts_p);
    -        match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
    -                             MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN);
    -
    -        /* Resubmit to table 33. */
    -        put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p);
    -        ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match,
    -                        ofpacts_p);
    -
    -
    -        match_init_catchall(&match);
    -        ofpbuf_clear(ofpacts_p);
     
             /* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */
             match_set_metadata(&match, htonll(dp_key));
    @@ -870,12 +853,30 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
             }
         }
     
    +    /* Table 32, priority 150.
    +     * ===============================
    +     *
    +     * Priority 150 is for packets received from a VXLAN tunnel
    +     * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to
    +     * lack of needed metadata in VXLAN, explicitly skip sending
    +     * back out any tunnels and resubmit to table 33 for local
    +     * delivery.
    +     */
    +    struct match match;
    +    match_init_catchall(&match);
    +    ofpbuf_clear(&ofpacts);
    +    match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
    +                         MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN);
    +
    +    /* Resubmit to table 33. */
    +    put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
    +    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, &ofpacts);
    +
         /* Table 32, Priority 0.
          * =======================
          *
          * Resubmit packets that are not directed at tunnels or part of a
          * multicast group to the local output table. */
    -    struct match match;
         match_init_catchall(&match);
         ofpbuf_clear(&ofpacts);
         put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
    diff --git a/tests/ovn.at b/tests/ovn.at
    index 628d3c8..b852665 100644
    --- a/tests/ovn.at
    +++ b/tests/ovn.at
    @@ -1058,6 +1058,11 @@ ovn_populate_arp
     # XXX This should be more systematic.
     sleep 1
     
    +# Make sure there is no attempt to adding duplicated flows by ovn-controller
    +AT_FAIL_IF([test -n "`grep duplicate hv1/ovn-controller.log`"])
    +AT_FAIL_IF([test -n "`grep duplicate hv2/ovn-controller.log`"])
    +AT_FAIL_IF([test -n "`grep duplicate hv3/ovn-controller.log`"])

This will catch other kinds of duplicate attempts as well, see below.
It may be ok, but also may be spurious logs.

        if (lport_lookup_by_name(lports, pb->logical_port)) {
            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
            VLOG_WARN_RL(&rl, "duplicate logical port name '%s'",
                         pb->logical_port);
            continue;
        }
        if (lport_lookup_by_key(lports, pb->datapath->tunnel_key,
                                pb->tunnel_key)) {
            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
            VLOG_WARN_RL(&rl, "duplicate logical port %"PRId64" in logical "
                         "datapath %"PRId64,
                         pb->tunnel_key, pb->datapath->tunnel_key);
            continue;
        }

    const struct sbrec_multicast_group *mg;
    SBREC_MULTICAST_GROUP_FOR_EACH (mg, ovnsb_idl) {
        const struct uuid *dp_uuid = &mg->datapath->header_.uuid;
        if (mcgroup_lookup_by_dp_name(mcgroups, mg->datapath, mg->name)) {
            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
            VLOG_WARN_RL(&rl, "datapath "UUID_FMT" contains duplicate "
                         "multicast group '%s'", UUID_ARGS(dp_uuid), mg->name);
            continue;


    +
     # Given the name of a logical port, prints the name of the hypervisor
     # on which it is located.
     vif_to_hv() {
    -- 
    2.1.0
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DgICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=T1PWK66qyQTC28gmZXt3PEjl-nSsstAfWMChPjgE2FE&s=SSrg-AJpWSMJ_8PCC2RQe_0JJLFlRKpfctY2PuKt_go&e=
Han Zhou Dec. 21, 2016, 9:50 p.m. UTC | #3 
oops, I updated the head line in v3, but forgot to update this message. I
am very sorry about that.

On Wed, Dec 21, 2016 at 1:45 PM, Darrell Ball <dball@vmware.com> wrote:

>
>
> On 12/21/16, 12:32 PM, "ovs-dev-bounces@openvswitch.org on behalf of Han
> Zhou" <ovs-dev-bounces@openvswitch.org on behalf of zhouhan@gmail.com>
> wrote:
>
>     In commit 475f0a2c it introduced a priority 150 flow for filtering
>     the sending of traffic received from vxlan tunnels back out tunnels.
>     However, it added the flow for every remote port processing, which
>     results in continuous logs about duplicated flows. We only need to
>     install this flow once per physical_run() loop iteration.
>
> I thought we covered this part in the last version.
>
> s/However, it added the flow for every remote port processing, which
>     results in continuous logs about duplicated flows. We only need to
>     install this flow once per physical_run() loop iteration.
> /
>     However, it attempted to add the flow for every remote port processing,
>     which results in continuous logs about duplicated flow add attempts. We
>     only need to attempt to install this flow once per physical_run() loop
> iteration.
> /
>
>     Signed-off-by: Han Zhou <zhouhan@gmail.com>
>     Acked-by: Darrell Ball <dball@vmware.com>
>     ---
>
>     Notes:
>         v1 -> v2: update commit message according to Darrell's comments.
>         v2 -> v3: update test case.
>
>      ovn/controller/physical.c | 47 ++++++++++++++++++++++++------
> -----------------
>      tests/ovn.at              |  5 +++++
>      2 files changed, 29 insertions(+), 23 deletions(-)
>
>     diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
>     index 48adb78..3b653dd 100644
>     --- a/ovn/controller/physical.c
>     +++ b/ovn/controller/physical.c
>     @@ -465,33 +465,16 @@ consider_port_binding(enum mf_field_id
> mff_ovn_geneve,
>          } else {
>              /* Remote port connected by tunnel */
>
>     -        /* Table 32, priority 150 and 100.
>     +        /* Table 32, priority 100.
>               * ===============================
>               *
>     -         * Priority 150 is for packets received from a VXLAN tunnel
>     -         * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due
> to
>     -         * lack of needed metadata in VXLAN, explicitly skip sending
>     -         * back out any tunnels and resubmit to table 33 for local
>     -         * delivery.
>     -         *
>     -         * Priority 100 is for all other traffic which need to be sent
>     -         * to a remote hypervisor.  Each flow matches an output port
>     -         * that includes a logical port on a remote hypervisor, and
>     -         * tunnels the packet to that hypervisor.
>     +         * Priority 100 is for traffic that needs to be sent to a
> remote
>     +         * hypervisor.  Each flow matches an output port that
> includes a
>     +         * logical port on a remote hypervisor, and tunnels the
> packet to
>     +         * that hypervisor.
>               */
>              match_init_catchall(&match);
>              ofpbuf_clear(ofpacts_p);
>     -        match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
>     -                             MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN);
>     -
>     -        /* Resubmit to table 33. */
>     -        put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p);
>     -        ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150,
> &match,
>     -                        ofpacts_p);
>     -
>     -
>     -        match_init_catchall(&match);
>     -        ofpbuf_clear(ofpacts_p);
>
>              /* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */
>              match_set_metadata(&match, htonll(dp_key));
>     @@ -870,12 +853,30 @@ physical_run(struct controller_ctx *ctx, enum
> mf_field_id mff_ovn_geneve,
>              }
>          }
>
>     +    /* Table 32, priority 150.
>     +     * ===============================
>     +     *
>     +     * Priority 150 is for packets received from a VXLAN tunnel
>     +     * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to
>     +     * lack of needed metadata in VXLAN, explicitly skip sending
>     +     * back out any tunnels and resubmit to table 33 for local
>     +     * delivery.
>     +     */
>     +    struct match match;
>     +    match_init_catchall(&match);
>     +    ofpbuf_clear(&ofpacts);
>     +    match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
>     +                         MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN);
>     +
>     +    /* Resubmit to table 33. */
>     +    put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
>     +    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match,
> &ofpacts);
>     +
>          /* Table 32, Priority 0.
>           * =======================
>           *
>           * Resubmit packets that are not directed at tunnels or part of a
>           * multicast group to the local output table. */
>     -    struct match match;
>          match_init_catchall(&match);
>          ofpbuf_clear(&ofpacts);
>          put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
>     diff --git a/tests/ovn.at b/tests/ovn.at
>     index 628d3c8..b852665 100644
>     --- a/tests/ovn.at
>     +++ b/tests/ovn.at
>     @@ -1058,6 +1058,11 @@ ovn_populate_arp
>      # XXX This should be more systematic.
>      sleep 1
>
>     +# Make sure there is no attempt to adding duplicated flows by
> ovn-controller
>     +AT_FAIL_IF([test -n "`grep duplicate hv1/ovn-controller.log`"])
>     +AT_FAIL_IF([test -n "`grep duplicate hv2/ovn-controller.log`"])
>     +AT_FAIL_IF([test -n "`grep duplicate hv3/ovn-controller.log`"])
>
> This will catch other kinds of duplicate attempts as well, see below.
> It may be ok, but also may be spurious logs.
>
>         if (lport_lookup_by_name(lports, pb->logical_port)) {
>             static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>             VLOG_WARN_RL(&rl, "duplicate logical port name '%s'",
>                          pb->logical_port);
>             continue;
>         }
>         if (lport_lookup_by_key(lports, pb->datapath->tunnel_key,
>                                 pb->tunnel_key)) {
>             static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>             VLOG_WARN_RL(&rl, "duplicate logical port %"PRId64" in logical
> "
>                          "datapath %"PRId64,
>                          pb->tunnel_key, pb->datapath->tunnel_key);
>             continue;
>         }
>
>     const struct sbrec_multicast_group *mg;
>     SBREC_MULTICAST_GROUP_FOR_EACH (mg, ovnsb_idl) {
>         const struct uuid *dp_uuid = &mg->datapath->header_.uuid;
>         if (mcgroup_lookup_by_dp_name(mcgroups, mg->datapath, mg->name)) {
>             static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>             VLOG_WARN_RL(&rl, "datapath "UUID_FMT" contains duplicate "
>                          "multicast group '%s'", UUID_ARGS(dp_uuid),
> mg->name);
>             continue;
>
>
>     +
>      # Given the name of a logical port, prints the name of the hypervisor
>      # on which it is located.
>      vif_to_hv() {
>     --
>     2.1.0
>
>     _______________________________________________
>     dev mailing list
>     dev@openvswitch.org
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.
> openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DgICAg&c=
> uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=
> T1PWK66qyQTC28gmZXt3PEjl-nSsstAfWMChPjgE2FE&s=SSrg-AJpWSMJ_8PCC2RQe_
> 0JJLFlRKpfctY2PuKt_go&e=
>
>
>
Darrell Ball Dec. 21, 2016, 9:52 p.m. UTC | #4 
There is another comment about the test below

From: Han Zhou <zhouhan@gmail.com>
Date: Wednesday, December 21, 2016 at 1:50 PM
To: Darrell Ball <dball@vmware.com>
Cc: "dev@openvswitch.org" <dev@openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v3] ovn-controller: Fix duplicated flow add attempts in table 32.

oops, I updated the head line in v3, but forgot to update this message. I am very sorry about that.

On Wed, Dec 21, 2016 at 1:45 PM, Darrell Ball <dball@vmware.com<mailto:dball@vmware.com>> wrote:


On 12/21/16, 12:32 PM, "ovs-dev-bounces@openvswitch.org<mailto:ovs-dev-bounces@openvswitch.org> on behalf of Han Zhou" <ovs-dev-bounces@openvswitch.org<mailto:ovs-dev-bounces@openvswitch.org> on behalf of zhouhan@gmail.com<mailto:zhouhan@gmail.com>> wrote:

    In commit 475f0a2c it introduced a priority 150 flow for filtering
    the sending of traffic received from vxlan tunnels back out tunnels.
    However, it added the flow for every remote port processing, which
    results in continuous logs about duplicated flows. We only need to
    install this flow once per physical_run() loop iteration.

I thought we covered this part in the last version.

s/However, it added the flow for every remote port processing, which
    results in continuous logs about duplicated flows. We only need to
    install this flow once per physical_run() loop iteration.
/
    However, it attempted to add the flow for every remote port processing,
    which results in continuous logs about duplicated flow add attempts. We
    only need to attempt to install this flow once per physical_run() loop iteration.
/

    Signed-off-by: Han Zhou <zhouhan@gmail.com<mailto:zhouhan@gmail.com>>
    Acked-by: Darrell Ball <dball@vmware.com<mailto:dball@vmware.com>>
    ---

    Notes:
        v1 -> v2: update commit message according to Darrell's comments.
        v2 -> v3: update test case.

     ovn/controller/physical.c | 47 ++++++++++++++++++++++++-----------------------
     tests/ovn.at<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DgMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=NRneCPMLzQ_PwYoGlU0-mmXbVPveJdUk2dcnWMS-6J4&s=-INO7BfZNDNgwPg-BMxNNjgtIbRWunu3sfm6BLQ7094&e=>              |  5 +++++
     2 files changed, 29 insertions(+), 23 deletions(-)

    diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
    index 48adb78..3b653dd 100644
    --- a/ovn/controller/physical.c
    +++ b/ovn/controller/physical.c
    @@ -465,33 +465,16 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
         } else {
             /* Remote port connected by tunnel */

    -        /* Table 32, priority 150 and 100.
    +        /* Table 32, priority 100.
              * ===============================
              *
    -         * Priority 150 is for packets received from a VXLAN tunnel
    -         * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to
    -         * lack of needed metadata in VXLAN, explicitly skip sending
    -         * back out any tunnels and resubmit to table 33 for local
    -         * delivery.
    -         *
    -         * Priority 100 is for all other traffic which need to be sent
    -         * to a remote hypervisor.  Each flow matches an output port
    -         * that includes a logical port on a remote hypervisor, and
    -         * tunnels the packet to that hypervisor.
    +         * Priority 100 is for traffic that needs to be sent to a remote
    +         * hypervisor.  Each flow matches an output port that includes a
    +         * logical port on a remote hypervisor, and tunnels the packet to
    +         * that hypervisor.
              */
             match_init_catchall(&match);
             ofpbuf_clear(ofpacts_p);
    -        match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
    -                             MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN);
    -
    -        /* Resubmit to table 33. */
    -        put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p);
    -        ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match,
    -                        ofpacts_p);
    -
    -
    -        match_init_catchall(&match);
    -        ofpbuf_clear(ofpacts_p);

             /* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */
             match_set_metadata(&match, htonll(dp_key));
    @@ -870,12 +853,30 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
             }
         }

    +    /* Table 32, priority 150.
    +     * ===============================
    +     *
    +     * Priority 150 is for packets received from a VXLAN tunnel
    +     * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to
    +     * lack of needed metadata in VXLAN, explicitly skip sending
    +     * back out any tunnels and resubmit to table 33 for local
    +     * delivery.
    +     */
    +    struct match match;
    +    match_init_catchall(&match);
    +    ofpbuf_clear(&ofpacts);
    +    match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
    +                         MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN);
    +
    +    /* Resubmit to table 33. */
    +    put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
    +    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, &ofpacts);
    +
         /* Table 32, Priority 0.
          * =======================
          *
          * Resubmit packets that are not directed at tunnels or part of a
          * multicast group to the local output table. */
    -    struct match match;
         match_init_catchall(&match);
         ofpbuf_clear(&ofpacts);
         put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
    diff --git a/tests/ovn.at<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DgMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=NRneCPMLzQ_PwYoGlU0-mmXbVPveJdUk2dcnWMS-6J4&s=-INO7BfZNDNgwPg-BMxNNjgtIbRWunu3sfm6BLQ7094&e=> b/tests/ovn.at<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DgMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=NRneCPMLzQ_PwYoGlU0-mmXbVPveJdUk2dcnWMS-6J4&s=-INO7BfZNDNgwPg-BMxNNjgtIbRWunu3sfm6BLQ7094&e=>
    index 628d3c8..b852665 100644
    --- a/tests/ovn.at<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DgMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=NRneCPMLzQ_PwYoGlU0-mmXbVPveJdUk2dcnWMS-6J4&s=-INO7BfZNDNgwPg-BMxNNjgtIbRWunu3sfm6BLQ7094&e=>
    +++ b/tests/ovn.at<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DgMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=NRneCPMLzQ_PwYoGlU0-mmXbVPveJdUk2dcnWMS-6J4&s=-INO7BfZNDNgwPg-BMxNNjgtIbRWunu3sfm6BLQ7094&e=>
    @@ -1058,6 +1058,11 @@ ovn_populate_arp
     # XXX This should be more systematic.
     sleep 1

    +# Make sure there is no attempt to adding duplicated flows by ovn-controller
    +AT_FAIL_IF([test -n "`grep duplicate hv1/ovn-controller.log`"])
    +AT_FAIL_IF([test -n "`grep duplicate hv2/ovn-controller.log`"])
    +AT_FAIL_IF([test -n "`grep duplicate hv3/ovn-controller.log`"])
This will catch other kinds of duplicate attempts as well, see below.
It may be ok, but also may be spurious logs.

        if (lport_lookup_by_name(lports, pb->logical_port)) {
            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
            VLOG_WARN_RL(&rl, "duplicate logical port name '%s'",
                         pb->logical_port);
            continue;
        }
        if (lport_lookup_by_key(lports, pb->datapath->tunnel_key,
                                pb->tunnel_key)) {
            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
            VLOG_WARN_RL(&rl, "duplicate logical port %"PRId64" in logical "
                         "datapath %"PRId64,
                         pb->tunnel_key, pb->datapath->tunnel_key);
            continue;
        }

    const struct sbrec_multicast_group *mg;
    SBREC_MULTICAST_GROUP_FOR_EACH (mg, ovnsb_idl) {
        const struct uuid *dp_uuid = &mg->datapath->header_.uuid;
        if (mcgroup_lookup_by_dp_name(mcgroups, mg->datapath, mg->name)) {
            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
            VLOG_WARN_RL(&rl, "datapath "UUID_FMT" contains duplicate "
                         "multicast group '%s'", UUID_ARGS(dp_uuid), mg->name);
            continue;


    +
     # Given the name of a logical port, prints the name of the hypervisor
     # on which it is located.
     vif_to_hv() {
    --
    2.1.0

    _______________________________________________
    dev mailing list
    dev@openvswitch.org<mailto:dev@openvswitch.org>
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DgICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=T1PWK66qyQTC28gmZXt3PEjl-nSsstAfWMChPjgE2FE&s=SSrg-AJpWSMJ_8PCC2RQe_0JJLFlRKpfctY2PuKt_go&e=
diff mbox

Patch

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 48adb78..3b653dd 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -465,33 +465,16 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
     } else {
         /* Remote port connected by tunnel */
 
-        /* Table 32, priority 150 and 100.
+        /* Table 32, priority 100.
          * ===============================
          *
-         * Priority 150 is for packets received from a VXLAN tunnel
-         * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to
-         * lack of needed metadata in VXLAN, explicitly skip sending
-         * back out any tunnels and resubmit to table 33 for local
-         * delivery.
-         *
-         * Priority 100 is for all other traffic which need to be sent
-         * to a remote hypervisor.  Each flow matches an output port
-         * that includes a logical port on a remote hypervisor, and
-         * tunnels the packet to that hypervisor.
+         * Priority 100 is for traffic that needs to be sent to a remote
+         * hypervisor.  Each flow matches an output port that includes a
+         * logical port on a remote hypervisor, and tunnels the packet to
+         * that hypervisor.
          */
         match_init_catchall(&match);
         ofpbuf_clear(ofpacts_p);
-        match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
-                             MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN);
-
-        /* Resubmit to table 33. */
-        put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p);
-        ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match,
-                        ofpacts_p);
-
-
-        match_init_catchall(&match);
-        ofpbuf_clear(ofpacts_p);
 
         /* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */
         match_set_metadata(&match, htonll(dp_key));
@@ -870,12 +853,30 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
         }
     }
 
+    /* Table 32, priority 150.
+     * ===============================
+     *
+     * Priority 150 is for packets received from a VXLAN tunnel
+     * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to
+     * lack of needed metadata in VXLAN, explicitly skip sending
+     * back out any tunnels and resubmit to table 33 for local
+     * delivery.
+     */
+    struct match match;
+    match_init_catchall(&match);
+    ofpbuf_clear(&ofpacts);
+    match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
+                         MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN);
+
+    /* Resubmit to table 33. */
+    put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
+    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, &ofpacts);
+
     /* Table 32, Priority 0.
      * =======================
      *
      * Resubmit packets that are not directed at tunnels or part of a
      * multicast group to the local output table. */
-    struct match match;
     match_init_catchall(&match);
     ofpbuf_clear(&ofpacts);
     put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
diff --git a/tests/ovn.at b/tests/ovn.at
index 628d3c8..b852665 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1058,6 +1058,11 @@  ovn_populate_arp
 # XXX This should be more systematic.
 sleep 1
 
+# Make sure there is no attempt to adding duplicated flows by ovn-controller
+AT_FAIL_IF([test -n "`grep duplicate hv1/ovn-controller.log`"])
+AT_FAIL_IF([test -n "`grep duplicate hv2/ovn-controller.log`"])
+AT_FAIL_IF([test -n "`grep duplicate hv3/ovn-controller.log`"])
+
 # Given the name of a logical port, prints the name of the hypervisor
 # on which it is located.
 vif_to_hv() {