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

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>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/binding.c | 21 +++++++++++++++++++++
 tests/ovn.at         | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

On Wed, Sep 23, 2020 at 10:10 AM <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>
> Signed-off-by: Numan Siddique <numans@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.

>  /* 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
>

On Thu, Sep 24, 2020 at 5:55 AM Han Zhou <hzhou@ovn.org> wrote:

> On Wed, Sep 23, 2020 at 10:10 AM <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>
> > Signed-off-by: Numan Siddique <numans@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

>  /* 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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>

On Wed, Sep 23, 2020 at 5:40 PM Numan Siddique <numans@ovn.org> wrote:

>
>
> On Thu, Sep 24, 2020 at 5:55 AM Han Zhou <hzhou@ovn.org> wrote:
>
>> On Wed, Sep 23, 2020 at 10:10 AM <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>
>> > Signed-off-by: Numan Siddique <numans@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@ovn.org>


> >  /* 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@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>

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

> On Wed, Sep 23, 2020 at 5:40 PM Numan Siddique <numans@ovn.org> wrote:
>
> >
> >
> > On Thu, Sep 24, 2020 at 5:55 AM Han Zhou <hzhou@ovn.org> wrote:
> >
> >> On Wed, Sep 23, 2020 at 10:10 AM <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>
> >> > Signed-off-by: Numan Siddique <numans@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@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@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>