| Message ID | 1582815458-20453-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-northd: Fix IGMP_Group port extraction. | expand |
Acked-by: Mark Michelson <mmichels@redhat.com> Should this be backported to branch-20.03 as well? On 2/27/20 9:57 AM, Dumitru Ceara wrote: > It can happen that all ports on which IGMP/MLD join reports were > received for a multicast group are already configured to flood multicast > traffic. In such cases we can safely skip the IGMP_Group record > completely. Otherwise we risk trying to install an entry in the > Southbound DB Multicast_Group table with 0 ports, triggering a > transaction error. > > Reported-by: Ying Xu <yinxu@redhat.com> > Reported-at: https://bugzilla.redhat.com/1805652 > Fixes: 79308138891a ("ovn-northd: Add static IP multicast flood configuration") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > northd/ovn-northd.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index d007ba6..e661380 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -3692,7 +3692,7 @@ static struct ovn_port ** > ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, > size_t *n_ports, struct hmap *ovn_ports) > { > - struct ovn_port **ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); > + struct ovn_port **ports = NULL; > > *n_ports = 0; > for (size_t i = 0; i < sb_igmp_group->n_ports; i++) { > @@ -3712,6 +3712,10 @@ ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, > continue; > } > > + if (ports == NULL) { > + ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); > + } > + > ports[(*n_ports)] = port; > ovn_port_find(ovn_ports, sb_igmp_group->ports[i]->logical_port); > if (ports[(*n_ports)]) { > @@ -10617,6 +10621,18 @@ build_mcast_groups(struct northd_context *ctx, > continue; > } > > + /* Extract the IGMP group ports from the SB entry. */ > + size_t n_igmp_ports; > + struct ovn_port **igmp_ports = > + ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); > + > + /* It can be that all ports in the IGMP group record already have > + * mcast_flood=true and then we can skip the group completely. > + */ > + if (!igmp_ports) { > + continue; > + } > + > /* Add the IGMP group entry. Will also try to allocate an ID for it > * if the multicast group already exists. > */ > @@ -10624,12 +10640,7 @@ build_mcast_groups(struct northd_context *ctx, > ovn_igmp_group_add(ctx, igmp_groups, od, &group_address, > sb_igmp->address); > > - /* Extract the IGMP group ports from the SB entry and store them > - * in the IGMP group. > - */ > - size_t n_igmp_ports; > - struct ovn_port **igmp_ports = > - ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); > + /* Add the extracted ports to the IGMP group. */ > ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports); > } > >
On 2/27/20 4:27 PM, Mark Michelson wrote: > Acked-by: Mark Michelson <mmichels@redhat.com> > Thanks Mark! I had however to send a v2 to fix an additional bug in the same function: https://patchwork.ozlabs.org/patch/1245917/ > Should this be backported to branch-20.03 as well? Yes, if possible. Thanks, Dumitru > > On 2/27/20 9:57 AM, Dumitru Ceara wrote: >> It can happen that all ports on which IGMP/MLD join reports were >> received for a multicast group are already configured to flood multicast >> traffic. In such cases we can safely skip the IGMP_Group record >> completely. Otherwise we risk trying to install an entry in the >> Southbound DB Multicast_Group table with 0 ports, triggering a >> transaction error. >> >> Reported-by: Ying Xu <yinxu@redhat.com> >> Reported-at: https://bugzilla.redhat.com/1805652 >> Fixes: 79308138891a ("ovn-northd: Add static IP multicast flood >> configuration") >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> northd/ovn-northd.c | 25 ++++++++++++++++++------- >> 1 file changed, 18 insertions(+), 7 deletions(-) >> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index d007ba6..e661380 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -3692,7 +3692,7 @@ static struct ovn_port ** >> ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, >> size_t *n_ports, struct hmap *ovn_ports) >> { >> - struct ovn_port **ports = xmalloc(sb_igmp_group->n_ports * sizeof >> *ports); >> + struct ovn_port **ports = NULL; >> *n_ports = 0; >> for (size_t i = 0; i < sb_igmp_group->n_ports; i++) { >> @@ -3712,6 +3712,10 @@ ovn_igmp_group_get_ports(const struct >> sbrec_igmp_group *sb_igmp_group, >> continue; >> } >> + if (ports == NULL) { >> + ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); >> + } >> + >> ports[(*n_ports)] = port; >> ovn_port_find(ovn_ports, >> sb_igmp_group->ports[i]->logical_port); >> if (ports[(*n_ports)]) { >> @@ -10617,6 +10621,18 @@ build_mcast_groups(struct northd_context *ctx, >> continue; >> } >> + /* Extract the IGMP group ports from the SB entry. */ >> + size_t n_igmp_ports; >> + struct ovn_port **igmp_ports = >> + ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); >> + >> + /* It can be that all ports in the IGMP group record already >> have >> + * mcast_flood=true and then we can skip the group completely. >> + */ >> + if (!igmp_ports) { >> + continue; >> + } >> + >> /* Add the IGMP group entry. Will also try to allocate an ID >> for it >> * if the multicast group already exists. >> */ >> @@ -10624,12 +10640,7 @@ build_mcast_groups(struct northd_context *ctx, >> ovn_igmp_group_add(ctx, igmp_groups, od, &group_address, >> sb_igmp->address); >> - /* Extract the IGMP group ports from the SB entry and store >> them >> - * in the IGMP group. >> - */ >> - size_t n_igmp_ports; >> - struct ovn_port **igmp_ports = >> - ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); >> + /* Add the extracted ports to the IGMP group. */ >> ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports); >> } >> >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index d007ba6..e661380 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -3692,7 +3692,7 @@ static struct ovn_port ** ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, size_t *n_ports, struct hmap *ovn_ports) { - struct ovn_port **ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); + struct ovn_port **ports = NULL; *n_ports = 0; for (size_t i = 0; i < sb_igmp_group->n_ports; i++) { @@ -3712,6 +3712,10 @@ ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, continue; } + if (ports == NULL) { + ports = xmalloc(sb_igmp_group->n_ports * sizeof *ports); + } + ports[(*n_ports)] = port; ovn_port_find(ovn_ports, sb_igmp_group->ports[i]->logical_port); if (ports[(*n_ports)]) { @@ -10617,6 +10621,18 @@ build_mcast_groups(struct northd_context *ctx, continue; } + /* Extract the IGMP group ports from the SB entry. */ + size_t n_igmp_ports; + struct ovn_port **igmp_ports = + ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); + + /* It can be that all ports in the IGMP group record already have + * mcast_flood=true and then we can skip the group completely. + */ + if (!igmp_ports) { + continue; + } + /* Add the IGMP group entry. Will also try to allocate an ID for it * if the multicast group already exists. */ @@ -10624,12 +10640,7 @@ build_mcast_groups(struct northd_context *ctx, ovn_igmp_group_add(ctx, igmp_groups, od, &group_address, sb_igmp->address); - /* Extract the IGMP group ports from the SB entry and store them - * in the IGMP group. - */ - size_t n_igmp_ports; - struct ovn_port **igmp_ports = - ovn_igmp_group_get_ports(sb_igmp, &n_igmp_ports, ports); + /* Add the extracted ports to the IGMP group. */ ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports); }
It can happen that all ports on which IGMP/MLD join reports were received for a multicast group are already configured to flood multicast traffic. In such cases we can safely skip the IGMP_Group record completely. Otherwise we risk trying to install an entry in the Southbound DB Multicast_Group table with 0 ports, triggering a transaction error. Reported-by: Ying Xu <yinxu@redhat.com> Reported-at: https://bugzilla.redhat.com/1805652 Fixes: 79308138891a ("ovn-northd: Add static IP multicast flood configuration") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovn-northd.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)