[ovs-dev,ovn] ovn-northd: Avoid empty address list when limiting ARP/ND broadcast.
diff mbox series

Message ID 1573735429-25895-1-git-send-email-dceara@redhat.com
State Accepted
Headers show
Series
  • [ovs-dev,ovn] ovn-northd: Avoid empty address list when limiting ARP/ND broadcast.
Related show

Commit Message

Dumitru Ceara Nov. 14, 2019, 12:43 p.m. UTC
Reported-by: Numan Siddique <numans@ovn.org>
Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever possible.")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 northd/ovn-northd.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Numan Siddique Nov. 14, 2019, 1:14 p.m. UTC | #1
On Thu, Nov 14, 2019 at 6:14 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> Reported-by: Numan Siddique <numans@ovn.org>
> Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever possible.")
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>

Thanks Dumitru for the fix. I applied this to master.

Thanks
Numan

> ---
>  northd/ovn-northd.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index d6beb97..6742bc0 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5337,10 +5337,14 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
>          }
>      }
>
> -    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET, sw_op,
> -                                            sw_od, 75, lflows);
> -    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6, sw_op,
> -                                            sw_od, 75, lflows);
> +    if (!sset_is_empty(&all_ips_v4)) {
> +        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET, sw_op,
> +                                                sw_od, 75, lflows);
> +    }
> +    if (!sset_is_empty(&all_ips_v6)) {
> +        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6, sw_op,
> +                                                sw_od, 75, lflows);
> +    }
>
>      sset_destroy(&all_ips_v4);
>      sset_destroy(&all_ips_v6);
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Dumitru Ceara Nov. 14, 2019, 2:21 p.m. UTC | #2
On Thu, Nov 14, 2019 at 2:14 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Thu, Nov 14, 2019 at 6:14 PM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> > Reported-by: Numan Siddique <numans@ovn.org>
> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever possible.")
> > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>
> Thanks Dumitru for the fix. I applied this to master.
>
> Thanks
> Numan

Thanks Numan and sorry for missing this case in the first place.

>
> > ---
> >  northd/ovn-northd.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index d6beb97..6742bc0 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -5337,10 +5337,14 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
> >          }
> >      }
> >
> > -    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET, sw_op,
> > -                                            sw_od, 75, lflows);
> > -    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6, sw_op,
> > -                                            sw_od, 75, lflows);
> > +    if (!sset_is_empty(&all_ips_v4)) {
> > +        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET, sw_op,
> > +                                                sw_od, 75, lflows);
> > +    }
> > +    if (!sset_is_empty(&all_ips_v6)) {
> > +        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6, sw_op,
> > +                                                sw_od, 75, lflows);
> > +    }
> >
> >      sset_destroy(&all_ips_v4);
> >      sset_destroy(&all_ips_v6);
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
Han Zhou Nov. 14, 2019, 6:22 p.m. UTC | #3
On Thu, Nov 14, 2019 at 6:22 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On Thu, Nov 14, 2019 at 2:14 PM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Thu, Nov 14, 2019 at 6:14 PM Dumitru Ceara <dceara@redhat.com> wrote:
> > >
> > > Reported-by: Numan Siddique <numans@ovn.org>
> > > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
whenever possible.")
> > > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> >
> > Thanks Dumitru for the fix. I applied this to master.
> >
> > Thanks
> > Numan
>
> Thanks Numan and sorry for missing this case in the first place.

Sorry for missing this in review. Just want to confirm, is this a bug or
just improvement? I think it is not a bug, because the set is empty {}, it
should just get ignored in the end by ovn-controller when translating to
OpenFlow rules, correct?
>
> >
> > > ---
> > >  northd/ovn-northd.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index d6beb97..6742bc0 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -5337,10 +5337,14 @@ build_lswitch_rport_arp_req_flows(struct
ovn_port *op,
> > >          }
> > >      }
> > >
> > > -    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET,
sw_op,
> > > -                                            sw_od, 75, lflows);
> > > -    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6,
sw_op,
> > > -                                            sw_od, 75, lflows);
> > > +    if (!sset_is_empty(&all_ips_v4)) {
> > > +        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4,
AF_INET, sw_op,
> > > +                                                sw_od, 75, lflows);
> > > +    }
> > > +    if (!sset_is_empty(&all_ips_v6)) {
> > > +        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6,
AF_INET6, sw_op,
> > > +                                                sw_od, 75, lflows);
> > > +    }
> > >
> > >      sset_destroy(&all_ips_v4);
> > >      sset_destroy(&all_ips_v6);
> > > --
> > > 1.8.3.1
> > >
> > > _______________________________________________
> > > 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
Numan Siddique Nov. 14, 2019, 6:43 p.m. UTC | #4
On Thu, Nov 14, 2019, 11:53 PM Han Zhou <zhouhan@gmail.com> wrote:

> On Thu, Nov 14, 2019 at 6:22 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> > On Thu, Nov 14, 2019 at 2:14 PM Numan Siddique <numans@ovn.org> wrote:
> > >
> > > On Thu, Nov 14, 2019 at 6:14 PM Dumitru Ceara <dceara@redhat.com>
> wrote:
> > > >
> > > > Reported-by: Numan Siddique <numans@ovn.org>
> > > > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> whenever possible.")
> > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> > >
> > > Thanks Dumitru for the fix. I applied this to master.
> > >
> > > Thanks
> > > Numan
> >
> > Thanks Numan and sorry for missing this case in the first place.
>
> Sorry for missing this in review. Just want to confirm, is this a bug or
> just improvement? I think it is not a bug, because the set is empty {}, it
> should just get ignored in the end by ovn-controller when translating to
> OpenFlow rules, correct?
>

This caused system tests to fail as there were warnings in the
ovn-controller log because of empty set.

Thanks

>
> > >
> > > > ---
> > > >  northd/ovn-northd.c | 12 ++++++++----
> > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > > index d6beb97..6742bc0 100644
> > > > --- a/northd/ovn-northd.c
> > > > +++ b/northd/ovn-northd.c
> > > > @@ -5337,10 +5337,14 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
> > > >          }
> > > >      }
> > > >
> > > > -    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET,
> sw_op,
> > > > -                                            sw_od, 75, lflows);
> > > > -    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6,
> sw_op,
> > > > -                                            sw_od, 75, lflows);
> > > > +    if (!sset_is_empty(&all_ips_v4)) {
> > > > +        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4,
> AF_INET, sw_op,
> > > > +                                                sw_od, 75, lflows);
> > > > +    }
> > > > +    if (!sset_is_empty(&all_ips_v6)) {
> > > > +        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6,
> AF_INET6, sw_op,
> > > > +                                                sw_od, 75, lflows);
> > > > +    }
> > > >
> > > >      sset_destroy(&all_ips_v4);
> > > >      sset_destroy(&all_ips_v6);
> > > > --
> > > > 1.8.3.1
> > > >
> > > > _______________________________________________
> > > > 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
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Han Zhou Nov. 14, 2019, 7:20 p.m. UTC | #5
On Thu, Nov 14, 2019 at 10:56 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On Thu, Nov 14, 2019 at 7:43 PM Numan Siddique <numans@ovn.org> wrote:
> >
> >
> >
> > On Thu, Nov 14, 2019, 11:53 PM Han Zhou <zhouhan@gmail.com> wrote:
> >>
> >> On Thu, Nov 14, 2019 at 6:22 AM Dumitru Ceara <dceara@redhat.com>
wrote:
> >> >
> >> > On Thu, Nov 14, 2019 at 2:14 PM Numan Siddique <numans@ovn.org>
wrote:
> >> > >
> >> > > On Thu, Nov 14, 2019 at 6:14 PM Dumitru Ceara <dceara@redhat.com>
wrote:
> >> > > >
> >> > > > Reported-by: Numan Siddique <numans@ovn.org>
> >> > > > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> >> whenever possible.")
> >> > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> >> > >
> >> > > Thanks Dumitru for the fix. I applied this to master.
> >> > >
> >> > > Thanks
> >> > > Numan
> >> >
> >> > Thanks Numan and sorry for missing this case in the first place.
> >>
> >> Sorry for missing this in review. Just want to confirm, is this a bug
or
> >> just improvement? I think it is not a bug, because the set is empty
{}, it
> >> should just get ignored in the end by ovn-controller when translating
to
> >> OpenFlow rules, correct?
> >
>
> Hi Han,
>
> As far as I see ovn-controller doesn't accept empty set. In
> parse_constant_set() we expect at least one constant:
>
> https://github.com/ovn-org/ovn/blob/master/lib/expr.c#L865
>
> So I would say that this was a bug in the ovn-northd code I added. And
> anyway it doesn't really make sense to add the flows if the address
> sets are empty.
>
> Thanks,
> Dumitru
>
Yes, you are right. I think I messed up with the empty address-set or
port-group, which works, but not empty constant set in {}.

Patch
diff mbox series

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index d6beb97..6742bc0 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5337,10 +5337,14 @@  build_lswitch_rport_arp_req_flows(struct ovn_port *op,
         }
     }
 
-    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET, sw_op,
-                                            sw_od, 75, lflows);
-    build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6, sw_op,
-                                            sw_od, 75, lflows);
+    if (!sset_is_empty(&all_ips_v4)) {
+        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET, sw_op,
+                                                sw_od, 75, lflows);
+    }
+    if (!sset_is_empty(&all_ips_v6)) {
+        build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6, sw_op,
+                                                sw_od, 75, lflows);
+    }
 
     sset_destroy(&all_ips_v4);
     sset_destroy(&all_ips_v6);