[ovs-dev] [PATCH ovn v2] controller: binding: Ignore changes to OVS interfaces which doesn't belong to int bridge.
Numan Siddique
numans at ovn.org
Thu Sep 24 05:45:02 UTC 2020
On Thu, Sep 24, 2020 at 9:24 AM Han Zhou <hzhou at ovn.org> wrote:
> 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 Han. I applied this patch to master and backported to branch-20.09
and branch-20.06.
Numan
> >
> > 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
> >>
> >>
> >>
> >>
> >>
> >>
> >
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list