| Message ID | 20200924014435.1503008-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] controller: binding: Ignore changes to OVS interfaces which doesn't belong to int bridge. | expand |
On Wed, Sep 23, 2020 at 6:44 PM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@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@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. > Acked-by: Han Zhou <hzhou@ovn.org> > Signed-off-by: Numan Siddique <numans@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, 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. 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 >
On Thu, Sep 24, 2020 at 8:12 AM Han Zhou <hzhou@ovn.org> wrote: > On Wed, Sep 23, 2020 at 6:44 PM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@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@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@ovn.org> > > Signed-off-by: Numan Siddique <numans@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 ? 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. 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. 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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Wed, Sep 23, 2020 at 8:18 PM Numan Siddique <numans@ovn.org> wrote: > > > On Thu, Sep 24, 2020 at 8:12 AM Han Zhou <hzhou@ovn.org> wrote: > >> On Wed, Sep 23, 2020 at 6:44 PM <numans@ovn.org> wrote: >> >> >> > >> >> >> > From: Numan Siddique <numans@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@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@ovn.org> >> >> >> > Signed-off-by: Numan Siddique <numans@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@openvswitch.org >> >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> >> >> >> > >
On Thu, Sep 24, 2020 at 9:24 AM Han Zhou <hzhou@ovn.org> wrote: > On Wed, Sep 23, 2020 at 8:18 PM Numan Siddique <numans@ovn.org> wrote: > > > > > > > On Thu, Sep 24, 2020 at 8:12 AM Han Zhou <hzhou@ovn.org> wrote: > > > >> On Wed, Sep 23, 2020 at 6:44 PM <numans@ovn.org> wrote: > >> > >> > >> > > >> > >> > >> > From: Numan Siddique <numans@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@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@ovn.org> > >> > >> > >> > Signed-off-by: Numan Siddique <numans@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@openvswitch.org > >> > >> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > >> > >> > >> > >> > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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 && + 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