[ovs-dev] [PATCH ovn] controller: binding: Ignore changes to OVS interfaces which doesn't belong to int bridge.

Numan Siddique numans at ovn.org
Thu Sep 24 01:48:42 UTC 2020


On Thu, Sep 24, 2020 at 6:19 AM Han Zhou <hzhou at ovn.org> wrote:

> On Wed, Sep 23, 2020 at 5:40 PM Numan Siddique <numans at ovn.org> wrote:
>
> >
> >
> > On Thu, Sep 24, 2020 at 5:55 AM Han Zhou <hzhou at ovn.org> wrote:
> >
> >> On Wed, Sep 23, 2020 at 10:10 AM <numans at ovn.org> wrote:
> >> >
> >> > From: Numan Siddique <numans at ovn.org>
> >> >
> >> > Consider the below commands.
> >> >
> >> > ovn-nbctl ls-add sw0
> >> > ovn-nbctl lsp-add sw0 sw0-p1
> >> >
> >> > ovs-vsctl add-br br-temp
> >> > ovs-vsctl add port br-temp t1 -- set interface t1
> >> external_ids:iface-id=sw0-p1
> >> >
> >> > When ovn-controller handles the OVS db changes incrementally, it binds
> >> the lport
> >> > sw0-p1, which is wrong as t1 is not in the integration bridge -
> br-int.
> >> >
> >> > If a recompute is triggered, ovn-controller releases the lport sw0-p1.
> >> >
> >> > This patch fixes this issue.
> >> >
> >> > Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS
> >> interface in runtime_data.")
> >> > CC: Han Zhou <hzhou at ovn.org>
> >> > Signed-off-by: Numan Siddique <numans at ovn.org>
> >> > ---
> >> >  controller/binding.c | 21 +++++++++++++++++++++
> >> >  tests/ovn.at         | 43
> +++++++++++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 64 insertions(+)
> >> >
> >> > diff --git a/controller/binding.c b/controller/binding.c
> >> > index 99e9fae301..bf4aa70c5d 100644
> >> > --- a/controller/binding.c
> >> > +++ b/controller/binding.c
> >> > @@ -1799,6 +1799,21 @@ is_iface_vif(const struct ovsrec_interface
> >> *iface_rec)
> >> >      return true;
> >> >  }
> >> >
> >> > +static bool
> >> > +is_iface_in_int_bridge(const struct ovsrec_interface *iface,
> >> > +                       const struct ovsrec_bridge *br_int)
> >> > +{
> >> > +    for (size_t i = 0; i < br_int->n_ports; i++) {
> >> > +        const struct ovsrec_port *p = br_int->ports[i];
> >> > +        for (size_t j = 0; j < p->n_interfaces; j++) {
> >> > +            if (!strcmp(iface->name, p->interfaces[j]->name)) {
> >> > +                return true;
> >> > +            }
> >> > +        }
> >> > +    }
> >> > +    return false;
> >> > +}
> >> > +
> >>
> >> Hi Numan, would it be better to build a hmap first so that the check is
> >> faster (O(n) instead of O(n^2))? There can be a big number of ports in a
> >> large scale environment considering that all tunnel ports are on the
> >> br-int
> >> bridge. Or, we can create an OVSDB IDL index for the interface->name
> >> column, which would be O(nlogn), but the engine node interface would
> need
> >> to be updated with the new index added.
> >>
> >>
> > Hi Han,
> >
> > Thanks for the review. I agree with you. But I have a couple of questions
> > on how to do it. An OVS port
> > can have multiple OVS interfaces. But generally I have seen only one
> > interface associated with an OVS port.
> > If we assume that, the complexity of the function
> is_iface_in_int_bridge()
> > would be O(n) right ?
> >
> > In order to improve this, we need to find the OVS port in the br-int
> using
> > the OVS interface name.
> > Given that an OVS port can have multiple interfaces, can we assume that
> we
> > can find the OVS port
> > in the br-int using the OVS interface name ?
> >
> > If you have any top of the head ideas on how to maintain the hmap please
> > let me know. No worries
> > otherwise. I will think about it.
> >
> > Thanks
> > Numan
> >
>
> Sorry I think I was fooled by the nested loop without looking at it more
> closely. Yes I agree with you in OVN use case it is 1-1 mapping between
> ports and interfaces.
> Using index would change this from O(n) to O(log(n)). But I am ok if you
> think that's not quite necessary here. So:
>
> Acked-by: Han Zhou <hzhou at ovn.org>
>


Thanks for the Ack. I think it's not quite necessary here.  I submitted v2
by moving the
call to is_iface_in_int_bridge() only if external_ids:iface-id is set and
ofport > 0.

Request to take a look -
https://patchwork.ozlabs.org/project/ovn/patch/20200924014435.1503008-1-numans@ovn.org/
and see if it is fine. I have added your Acked-by too.

Thanks
Numan


>
> > >  /* Returns true if the ovs interface changes were handled
> successfully,
> >> >   * false otherwise.
> >> >   */
> >> > @@ -1904,6 +1919,12 @@ binding_handle_ovs_interface_changes(struct
> >> binding_ctx_in *b_ctx_in,
> >> >              continue;
> >> >          }
> >> >
> >> > +        /* If the changed interface doesn't belong to the integration
> >> bridge,
> >> > +         * ignore it. */
> >> > +        if (!is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) {
> >> > +            continue;
> >> > +        }
> >> > +
> >> >          const char *iface_id = smap_get(&iface_rec->external_ids,
> >> "iface-id");
> >> >          int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport :
> 0;
> >> >          if (iface_id && ofport > 0) {
> >> > diff --git a/tests/ovn.at b/tests/ovn.at
> >> > index 59e59f43f8..b6c8622ba4 100644
> >> > --- a/tests/ovn.at
> >> > +++ b/tests/ovn.at
> >> > @@ -22256,3 +22256,46 @@ grep conjunction | wc -l`])
> >> >
> >> >  OVN_CLEANUP([hv1])
> >> >  AT_CLEANUP
> >> > +
> >> > +AT_SETUP([ovn -- I-P of OVS interface changes which belong to non
> >> integration bridge])
> >> > +ovn_start
> >> > +
> >> > +net_add n1
> >> > +sim_add hv1
> >> > +as hv1
> >> > +ovs-vsctl add-br br-phys
> >> > +ovn_attach n1 br-phys 192.168.0.10
> >> > +
> >> > +ovn-nbctl ls-add sw0
> >> > +ovn-nbctl lsp-add sw0 sw0-p1
> >> > +ovn-nbctl lsp-add sw0 sw0-p2
> >> > +
> >> > +as hv1 ovs-vsctl \
> >> > +    -- add-port br-int vif1 \
> >> > +    -- set Interface vif1 external_ids:iface-id=sw0-p1
> >> > +
> >> > +# Wait for port to be bound.
> >> > +OVS_WAIT_UNTIL([test $(ovn-sbctl --columns _uuid --bare list chassis
> >> hv1
> >> | wc -l) -eq 1])
> >> > +ch=$(ovn-sbctl --columns _uuid --bare list chassis hv1)
> >> > +OVS_WAIT_UNTIL([test $(ovn-sbctl --columns chassis --bare list
> >> port_binding sw0-p1 | grep $ch -c) -eq 1])
> >> > +
> >> > +as hv1 ovs-vsctl add-br br-temp
> >> > +as hv1 ovs-vsctl \
> >> > +    -- add-port br-temp t1 \
> >> > +    -- set Interface t1 external_ids:iface-id=sw0-p2
> >> > +
> >> > +ovn-nbctl --wait=hv sync
> >> > +
> >> > +# hv1 ovn-controller should not bind sw0-p2.
> >> > +AT_CHECK([test $(ovn-sbctl --columns chassis --bare list port_binding
> >> sw0-p2 | grep $ch -c) -eq 0])
> >> > +
> >> > +# Trigger recompute and sw0-p2 should not be claimed.
> >> > +as hv1 ovn-appctl -t ovn-controller recompute
> >> > +ovn-nbctl --wait=hv sync
> >> > +
> >> > +AT_CHECK([test $(ovn-sbctl --columns chassis --bare list port_binding
> >> sw0-p2 | grep $ch -c) -eq 0])
> >> > +
> >> > +ovn-sbctl --columns chassis --bare list port_binding sw0-p2
> >> > +
> >> > +OVN_CLEANUP([hv1])
> >> > +AT_CLEANUP
> >> > --
> >> > 2.26.2
> >> >
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list