[ovs-dev] [PATCH ovn] ovn-controller: Fix the missing flows with monitor-all set to True

Numan Siddique numans at ovn.org
Thu Jul 16 05:17:21 UTC 2020


On Thu, Jul 16, 2020 at 1:34 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 7/15/20 7:56 PM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > When the config ovn-monitor-all is set to True, then ovn-controller is not
> > programming the flows completely when it binds the logical port of
> > a logical switch until a full recompute is triggered.
> >
> > This issue was introduced by the commit in Fixes - which added incremental procesing
> > for runtime data changes.
> >
> > This patch fixes this issue.
> >
> > Fixes: 3d096f833c4a("ovn-controller: Handle runtime data changes in flow output engine")
> >
> > Reported-by: Dumitru Ceara <dceara at redhat.com>
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> >  controller/binding.c |  17 ++++++--
> >  tests/ovn.at         | 101 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 115 insertions(+), 3 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index b2c388146d..8f6d68006e 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -103,9 +103,20 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> >      ld->localnet_port = NULL;
> >      ld->has_local_l3gateway = has_local_l3gateway;
> >
> > -    if (tracked_datapaths &&
> > -        !tracked_binding_datapath_find(tracked_datapaths, datapath)) {
> > -        tracked_binding_datapath_create(datapath, true, tracked_datapaths);
> > +    if (tracked_datapaths) {
> > +        struct tracked_binding_datapath *tdp =
> > +            tracked_binding_datapath_find(tracked_datapaths, datapath);
> > +        if (!tdp) {
> > +            tracked_binding_datapath_create(datapath, true, tracked_datapaths);
> > +        } else {
> > +            /* Its possible that there is already an entry in tracked datapaths
>
> Nit: s/Its/It's.
>
> > +             * for this 'datapath'. tracked_binding_datapath_lport_add() may
> > +             * have created it. Since the 'datapath' is added to the
> > +             * local datapaths, set the tdp->is_new to true so that the flows
>
> Nit s/set the/set
>
> Otherwise:
> Acked-by: Dumitru Ceara <dceara at redhat.com>

Thanks Dumitru.
I corrected the typo and applied to master and branch-20.06

Thanks
Numan

>
> Thanks,
> Dumitru
>
> > +             * for this datapath are programmed properly.
> > +             * */
> > +            tdp->is_new = true;
> > +        }
> >      }
> >
> >      if (depth >= 100) {
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 24d93bc245..ba1a534e92 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -20601,3 +20601,104 @@ OVS_WAIT_UNTIL([
> >
> >  OVN_CLEANUP([hv1], [hv2])
> >  AT_CLEANUP
> > +
> > +AT_SETUP([ovn -- controller I-P handling with monitoring disabled])
> > +AT_KEYWORDS([lb])
> > +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
> > +
> > +
> > +sim_add hv2
> > +as hv2
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.2
> > +
> > +ovn-nbctl ls-add sw0
> > +
> > +ovn-nbctl lsp-add sw0 sw0-p1
> > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> > +
> > +ovn-nbctl lsp-add sw0 sw0-p2
> > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> > +
> > +as hv1
> > +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> > +    set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
> > +    options:tx_pcap=hv1/vif1-tx.pcap \
> > +    options:rxq_pcap=hv1/vif1-rx.pcap \
> > +    ofport-request=1
> > +
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup])
> > +
> > +# Get the number of OF flows in hv1 and hv2
> > +hv1_offlows=$(as hv1 ovs-ofctl dump-flows br-int | wc -l)
> > +echo "hv1 flows : $hv1_offlows"
> > +AT_CHECK([test $hv1_offlows -gt 0])
> > +
> > +as hv2
> > +ovs-vsctl -- add-port br-int hv2-vif1 -- \
> > +    set interface hv2-vif1 external-ids:iface-id=sw0-p2 \
> > +    options:tx_pcap=hv2/vif1-tx.pcap \
> > +    options:rxq_pcap=hv2/vif1-rx.pcap \
> > +    ofport-request=1
> > +
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup])
> > +
> > +hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l)
> > +echo "hv2 flows : $hv2_offlows"
> > +AT_CHECK([test $hv2_offlows -gt 0])
> > +
> > +ovn-nbctl ls-del sw0
> > +as hv1 ovs-vsctl del-port hv1-vif1
> > +as hv2 ovs-vsctl del-port hv2-vif1
> > +
> > +as hv1 ovs-vsctl set open . external_ids:ovn-monitor-all=true
> > +as hv2 ovs-vsctl set open . external_ids:ovn-monitor-all=true
> > +
> > +ovn-nbctl ls-add sw0
> > +
> > +ovn-nbctl lsp-add sw0 sw0-p1
> > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> > +
> > +ovn-nbctl lsp-add sw0 sw0-p2
> > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> > +
> > +as hv1
> > +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> > +    set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
> > +    options:tx_pcap=hv1/vif1-tx.pcap \
> > +    options:rxq_pcap=hv1/vif1-rx.pcap \
> > +    ofport-request=1
> > +
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup])
> > +
> > +# Get the number of OF flows in hv1 and hv2
> > +hv1_offlows_mon=$(as hv1 ovs-ofctl dump-flows br-int | wc -l)
> > +echo "hv1 flows after monitor-all=true : $hv1_offlows"
> > +AT_CHECK([test "$hv1_offlows" = "$hv1_offlows_mon"])
> > +
> > +as hv2
> > +ovs-vsctl -- add-port br-int hv2-vif1 -- \
> > +    set interface hv2-vif1 external-ids:iface-id=sw0-p2 \
> > +    options:tx_pcap=hv2/vif1-tx.pcap \
> > +    options:rxq_pcap=hv2/vif1-rx.pcap \
> > +    ofport-request=1
> > +
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup])
> > +
> > +hv2_offlows_mon=$(as hv2 ovs-ofctl dump-flows br-int | wc -l)
> > +echo "hv2 flows after monitor-all=true : $hv2_offlows"
> > +AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"])
> > +
> > +OVN_CLEANUP([hv1], [hv2])
> > +AT_CLEANUP
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list