| 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. | expand |
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 >
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 > > >
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
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 > >
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 {}.
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);
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(-)