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

[Han Zhou]

On Wed, Sep 23, 2020 at 8:18 PM Numan Siddique <numans at ovn.org> wrote:

>
>
> On Thu, Sep 24, 2020 at 8:12 AM Han Zhou <hzhou at ovn.org> wrote:
>
>> On Wed, Sep 23, 2020 at 6:44 PM <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>
>>
>>
>>
>>
>>
>> According to the
>>
>>
>> Documentation/internals/contributing/submitting-patches.rst, the CC tag
>>
>>
>> should be removed if there are other tags (such as Acked-by) for the same
>>
>>
>> person.
>>
>>
>>
>>
> Ack. I will remove it.
>
>
>
>>
>>
>> > Acked-by: Han Zhou <hzhou at ovn.org>
>>
>>
>> > Signed-off-by: Numan Siddique <numans at ovn.org>
>>
>>
>> > ---
>>
>>
>> > v1 -> v2
>>
>>
>> > ------
>>
>>
>> >   * Moved the code to call is_iface_in_int_bridge() only if the iface
>>
>>
>> >     has iface_id set and ofport > 0.
>>
>>
>> >
>>
>>
>> >  controller/binding.c | 18 +++++++++++++++++-
>>
>>
>> >  tests/ovn.at         | 43 +++++++++++++++++++++++++++++++++++++++++++
>>
>>
>> >  2 files changed, 60 insertions(+), 1 deletion(-)
>>
>>
>> >
>>
>>
>> > diff --git a/controller/binding.c b/controller/binding.c
>>
>>
>> > index 99e9fae301..36fd350091 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;
>>
>>
>> > +}
>>
>>
>> > +
>>
>>
>> >  /* Returns true if the ovs interface changes were handled successfully,
>>
>>
>> >   * false otherwise.
>>
>>
>> >   */
>>
>>
>> > @@ -1906,7 +1921,8 @@ binding_handle_ovs_interface_changes(struct
>>
>>
>> binding_ctx_in *b_ctx_in,
>>
>>
>> >
>>
>>
>> >          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) {
>>
>>
>> > +        if (iface_id && ofport > 0 &&
>>
>>
>>
>>
>>
>> I think we should check  is_iface_in_int_bridge() before any processing,
>>
>
> You mean you prefer version 1 of this patch for the 2nd TRACKED loop ?
>
> But we would unnecessarily run the for loop in is_iface_in_int_bridge()
> right ?
>

Agree.


If an OVS interface gets created/updated without any external_ids:iface-id
> set, we don't
> need to consider claiming the interface right ? What do you think ?
>
>
>
>
>>
>> and we should do this check in both of the
>>
>>
>> OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED loops (there are two places) in
>>
>>
>> this function.
>>
>
>>
>> If an interface originally had the ifact_id set (bound to a lsp), and now
>>
>>
>> it is changed without the ifact_id (meaning we should probably unbind it).
>>
>>
>> However, if this happens to an interface that doesn't belong to br-int, we
>>
>>
>> shouldn't care.
>>
>
> I think there is no need for the first loop  which takes care of releasing
> the iface.
> Suppose if we have ovs port p1 with external_ids:iface-id set to sw0-p1
> and ovn-controller
> has already claimed the logical port.
>
> Now when we delete the port as (ovs-vsctl del-port p1), then the
> function  is_iface_in_int_bridge()
> would return false, since the port no longer is there in the br-int and we
> will not release the binding
> for the logical port - sw0-p1.
>

Yes you are right.

>
> Suppose a port 'p2' is on br-temp and the external_ids:iface-id was set
> earlier and it changes without the iface-id (for
> the case you mentioned above) then even if we
> call consider_iface_release() it will be a no-op because there will not be
> any 'struct lbinding' for this interface as we would not have claimed
> earlier.
>

Agree. Please ignore my comments and keep my Ack :)

>
> Thanks
> Numan
>
>
>>
>> Thanks,
>>
>>
>> Han
>>
>>
>>
>>
>>
>> > +                is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) {
>>
>>
>> >              handled = consider_iface_claim(iface_rec, iface_id,
>> b_ctx_in,
>>
>>
>> >                                             b_ctx_out, qos_map_ptr);
>>
>>
>> >              if (!handled) {
>>
>>
>> > 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
>>
>>
>>
>>
>>
>>
>
>