Re: [ovs-dev] [PATCH ovn v2 3/4] ic: prevent advertising/learning multiple same routes

[Dumitru Ceara]

On 12/15/22 09:31, Vladislav Odintsov wrote:
> Hi Dumitru,
> 
> Regards,
> Vladislav Odintsov
> 
>> On 12 Dec 2022, at 17:57, Dumitru Ceara  wrote:
>>
>> On 12/12/22 14:39, Vladislav Odintsov wrote:
>>> Hi Numan, Dumitru,
>>>
>>> I forgot one thing about a possible upgrade recommendation for the users.
>>> Fix can be applied much easier: just upgrading all ovn-ic in all AZs first 
>>> and then converting DB schema. This will remove all duplicates.
>>> The latter can be requested manually invoking systemctl restart ovn-ic-db 
>>> (at least for RH, there is a separate systemd unit for IC databases) or 
>>> calling ovsdb-client convert … command directly.
>>>
>>
>> This is a good point, especially because we don't define anywhere
>> (AFAIK) an order of upgrading components when it comes to ovn-ic.
>>
>> We should document this in ovn-upgrades.rst.
>>
>>> Would it be an acceptable compromise for this situation?
>>
>> This has a long term implication though: ovn-ic must always be backwards
>> compatible with ovn-ic-SB schema changes.  That is not the case with
>> ovn-northd and SB schema.  If people think that's ok, I'm fine with that
>> too but we need to make sure we properly document this.
>>
> 
> As nobody answers, should I:
> 1. Split patch #3 by two: fix of duplicates (in order it do be backportable 
> to older branches) and the second part is an index changes + upgrade docs, 
> which will be a part of a major upgrade and will not be considered for 
> backporting.
> 2. Describe in the second part, that if user upgrades from any version prior 
> to this major release it should upgrade all ovn-ic daemons on all 
> availability zones first and then upgrade ovn-ic-sb schema?
> 
> If that has no objections, I’ll send a new patchset for a final review.
> 

I think that makes sense.  Thanks again for working on this, looking
forward to the new revision.

Regards,
Dumitru

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2 3/4] ic: prevent advertising/learning multiple same routes

[Vladislav Odintsov]

Hi Dumitru,

Regards,
Vladislav Odintsov

> On 12 Dec 2022, at 17:57, Dumitru Ceara  wrote:
> 
> On 12/12/22 14:39, Vladislav Odintsov wrote:
>> Hi Numan, Dumitru,
>> 
>> I forgot one thing about a possible upgrade recommendation for the users.
>> Fix can be applied much easier: just upgrading all ovn-ic in all AZs first 
>> and then converting DB schema. This will remove all duplicates.
>> The latter can be requested manually invoking systemctl restart ovn-ic-db 
>> (at least for RH, there is a separate systemd unit for IC databases) or 
>> calling ovsdb-client convert … command directly.
>> 
> 
> This is a good point, especially because we don't define anywhere
> (AFAIK) an order of upgrading components when it comes to ovn-ic.
> 
> We should document this in ovn-upgrades.rst.
> 
>> Would it be an acceptable compromise for this situation?
> 
> This has a long term implication though: ovn-ic must always be backwards
> compatible with ovn-ic-SB schema changes.  That is not the case with
> ovn-northd and SB schema.  If people think that's ok, I'm fine with that
> too but we need to make sure we properly document this.
> 

As nobody answers, should I:
1. Split patch #3 by two: fix of duplicates (in order it do be backportable to 
older branches) and the second part is an index changes + upgrade docs, which 
will be a part of a major upgrade and will not be considered for backporting.
2. Describe in the second part, that if user upgrades from any version prior to 
this major release it should upgrade all ovn-ic daemons on all availability 
zones first and then upgrade ovn-ic-sb schema?

If that has no objections, I’ll send a new patchset for a final review.

> Regards,
> Dumitru
> 
>> 
>> Regards,
>> Vladislav Odintsov
>> 
>>> On 12 Dec 2022, at 16:20, Numan Siddique  wrote:
>>> 
>>> On Mon, Dec 12, 2022 at 5:41 AM Dumitru Ceara  wrote:
 
 On 12/9/22 19:42, Vladislav Odintsov wrote:
> Hi Dumitru,
 
 Hi Vladislav,
 
> 
> please see answers inline.
> 
> Regards,
> Vladislav Odintsov
> 
>> On 9 Dec 2022, at 17:37, Dumitru Ceara  wrote:
>> 
>> On 12/6/22 11:20, Vladislav Odintsov wrote:
>>> Signed-off-by: Vladislav Odintsov >> >
>>> ---
>> 
>> Hi Vladislav,
>> 
>>> ic/ovn-ic.c | 83 +++--
>>> ovn-ic-sb.ovsschema |  6 ++--
>>> tests/ovn-ic.at | 60 
>>> 3 files changed, 114 insertions(+), 35 deletions(-)
>>> 
>>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>>> index 9e2369fef..64307d8c4 100644
>>> --- a/ic/ovn-ic.c
>>> +++ b/ic/ovn-ic.c
>>> @@ -884,10 +884,12 @@ ic_route_hash(const struct in6_addr *prefix, 
>>> unsigned int plen,
>>> static struct ic_route_info *
>>> ic_route_find(struct hmap *routes, const struct in6_addr *prefix,
>>> unsigned int plen, const struct in6_addr *nexthop,
>>> -  const char *origin, char *route_table)
>>> +  const char *origin, const char *route_table, uint32_t 
>>> hash)
>>> {
>>>   struct ic_route_info *r;
>>> -uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, 
>>> route_table);
>>> +if (!hash) {
>>> +hash = ic_route_hash(prefix, plen, nexthop, origin, 
>>> route_table);
>>> +}
>>>   HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) {
>>>   if (ipv6_addr_equals(>prefix, prefix) &&
>>>   r->plen == plen &&
>>> @@ -945,8 +947,8 @@ add_to_routes_learned(struct hmap *routes_learned,
>>>   }
>>>   const char *origin = smap_get_def(_route->options, "origin", "");
>>>   if (ic_route_find(routes_learned, , plen, , origin,
>>> -  nb_route->route_table)) {
>>> -/* Route is already added to learned in previous iteration. */
>>> +  nb_route->route_table, 0)) {
>>> +/* Route was added to learned on previous iteration. */
>>>   return true;
>>>   }
>>> 
>>> @@ -1093,10 +1095,42 @@ route_need_advertise(const char *policy,
>>> }
>>> 
>>> static void
>>> -add_to_routes_ad(struct hmap *routes_ad,
>>> - const struct nbrec_logical_router_static_route 
>>> *nb_route,
>>> - const struct lport_addresses *nexthop_addresses,
>>> - const struct smap *nb_options, const char 
>>> *route_table)
>>> +add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix,
>>> + unsigned int plen, const struct in6_addr nexthop,
>>> + const char *origin, const char *route_table,
>>> + const struct nbrec_logical_router_port *nb_lrp,
>>> + const struct nbrec_logical_router_static_route 
>>> *nb_route)
>>> +{
>>> +if (route_table == NULL) {
>>> +

Re: [ovs-dev] [PATCH ovn v2 3/4] ic: prevent advertising/learning multiple same routes

[Dumitru Ceara]

On 12/12/22 14:39, Vladislav Odintsov wrote:
> Hi Numan, Dumitru,
> 
> I forgot one thing about a possible upgrade recommendation for the users.
> Fix can be applied much easier: just upgrading all ovn-ic in all AZs first 
> and then converting DB schema. This will remove all duplicates.
> The latter can be requested manually invoking systemctl restart ovn-ic-db (at 
> least for RH, there is a separate systemd unit for IC databases) or calling 
> ovsdb-client convert … command directly.
> 

This is a good point, especially because we don't define anywhere
(AFAIK) an order of upgrading components when it comes to ovn-ic.

We should document this in ovn-upgrades.rst.

> Would it be an acceptable compromise for this situation?

This has a long term implication though: ovn-ic must always be backwards
compatible with ovn-ic-SB schema changes.  That is not the case with
ovn-northd and SB schema.  If people think that's ok, I'm fine with that
too but we need to make sure we properly document this.

Regards,
Dumitru

> 
> Regards,
> Vladislav Odintsov
> 
>> On 12 Dec 2022, at 16:20, Numan Siddique  wrote:
>>
>> On Mon, Dec 12, 2022 at 5:41 AM Dumitru Ceara  wrote:
>>>
>>> On 12/9/22 19:42, Vladislav Odintsov wrote:
 Hi Dumitru,
>>>
>>> Hi Vladislav,
>>>

 please see answers inline.

 Regards,
 Vladislav Odintsov

> On 9 Dec 2022, at 17:37, Dumitru Ceara  wrote:
>
> On 12/6/22 11:20, Vladislav Odintsov wrote:
>> Signed-off-by: Vladislav Odintsov > >
>> ---
>
> Hi Vladislav,
>
>> ic/ovn-ic.c | 83 +++--
>> ovn-ic-sb.ovsschema |  6 ++--
>> tests/ovn-ic.at | 60 
>> 3 files changed, 114 insertions(+), 35 deletions(-)
>>
>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>> index 9e2369fef..64307d8c4 100644
>> --- a/ic/ovn-ic.c
>> +++ b/ic/ovn-ic.c
>> @@ -884,10 +884,12 @@ ic_route_hash(const struct in6_addr *prefix, 
>> unsigned int plen,
>> static struct ic_route_info *
>> ic_route_find(struct hmap *routes, const struct in6_addr *prefix,
>>  unsigned int plen, const struct in6_addr *nexthop,
>> -  const char *origin, char *route_table)
>> +  const char *origin, const char *route_table, uint32_t 
>> hash)
>> {
>>struct ic_route_info *r;
>> -uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, 
>> route_table);
>> +if (!hash) {
>> +hash = ic_route_hash(prefix, plen, nexthop, origin, 
>> route_table);
>> +}
>>HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) {
>>if (ipv6_addr_equals(>prefix, prefix) &&
>>r->plen == plen &&
>> @@ -945,8 +947,8 @@ add_to_routes_learned(struct hmap *routes_learned,
>>}
>>const char *origin = smap_get_def(_route->options, "origin", "");
>>if (ic_route_find(routes_learned, , plen, , origin,
>> -  nb_route->route_table)) {
>> -/* Route is already added to learned in previous iteration. */
>> +  nb_route->route_table, 0)) {
>> +/* Route was added to learned on previous iteration. */
>>return true;
>>}
>>
>> @@ -1093,10 +1095,42 @@ route_need_advertise(const char *policy,
>> }
>>
>> static void
>> -add_to_routes_ad(struct hmap *routes_ad,
>> - const struct nbrec_logical_router_static_route 
>> *nb_route,
>> - const struct lport_addresses *nexthop_addresses,
>> - const struct smap *nb_options, const char *route_table)
>> +add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix,
>> + unsigned int plen, const struct in6_addr nexthop,
>> + const char *origin, const char *route_table,
>> + const struct nbrec_logical_router_port *nb_lrp,
>> + const struct nbrec_logical_router_static_route 
>> *nb_route)
>> +{
>> +if (route_table == NULL) {
>> +route_table = "";
>> +}
>> +
>> +uint hash = ic_route_hash(, plen, , origin, 
>> route_table);
>> +
>> +if (!ic_route_find(routes_ad, , plen, , origin, 
>> route_table,
>> +   hash)) {
>> +struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
>> +ic_route->prefix = prefix;
>> +ic_route->plen = plen;
>> +ic_route->nexthop = nexthop;
>> +ic_route->nb_route = nb_route;
>> +ic_route->origin = origin;
>> +ic_route->route_table = route_table;
>> +ic_route->nb_lrp = nb_lrp;
>> +hmap_insert(routes_ad, _route->node, hash);
>> +} else {
>> +VLOG_WARN("Duplicate route advertisement was 

Re: [ovs-dev] [PATCH ovn v2 3/4] ic: prevent advertising/learning multiple same routes

[Vladislav Odintsov]

Hi Numan, Dumitru,

I forgot one thing about a possible upgrade recommendation for the users.
Fix can be applied much easier: just upgrading all ovn-ic in all AZs first and 
then converting DB schema. This will remove all duplicates.
The latter can be requested manually invoking systemctl restart ovn-ic-db (at 
least for RH, there is a separate systemd unit for IC databases) or calling 
ovsdb-client convert … command directly.

Would it be an acceptable compromise for this situation?

Regards,
Vladislav Odintsov

> On 12 Dec 2022, at 16:20, Numan Siddique  wrote:
> 
> On Mon, Dec 12, 2022 at 5:41 AM Dumitru Ceara  wrote:
>> 
>> On 12/9/22 19:42, Vladislav Odintsov wrote:
>>> Hi Dumitru,
>> 
>> Hi Vladislav,
>> 
>>> 
>>> please see answers inline.
>>> 
>>> Regards,
>>> Vladislav Odintsov
>>> 
 On 9 Dec 2022, at 17:37, Dumitru Ceara  wrote:
 
 On 12/6/22 11:20, Vladislav Odintsov wrote:
> Signed-off-by: Vladislav Odintsov  >
> ---
 
 Hi Vladislav,
 
> ic/ovn-ic.c | 83 +++--
> ovn-ic-sb.ovsschema |  6 ++--
> tests/ovn-ic.at | 60 
> 3 files changed, 114 insertions(+), 35 deletions(-)
> 
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index 9e2369fef..64307d8c4 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -884,10 +884,12 @@ ic_route_hash(const struct in6_addr *prefix, 
> unsigned int plen,
> static struct ic_route_info *
> ic_route_find(struct hmap *routes, const struct in6_addr *prefix,
>  unsigned int plen, const struct in6_addr *nexthop,
> -  const char *origin, char *route_table)
> +  const char *origin, const char *route_table, uint32_t hash)
> {
>struct ic_route_info *r;
> -uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, 
> route_table);
> +if (!hash) {
> +hash = ic_route_hash(prefix, plen, nexthop, origin, route_table);
> +}
>HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) {
>if (ipv6_addr_equals(>prefix, prefix) &&
>r->plen == plen &&
> @@ -945,8 +947,8 @@ add_to_routes_learned(struct hmap *routes_learned,
>}
>const char *origin = smap_get_def(_route->options, "origin", "");
>if (ic_route_find(routes_learned, , plen, , origin,
> -  nb_route->route_table)) {
> -/* Route is already added to learned in previous iteration. */
> +  nb_route->route_table, 0)) {
> +/* Route was added to learned on previous iteration. */
>return true;
>}
> 
> @@ -1093,10 +1095,42 @@ route_need_advertise(const char *policy,
> }
> 
> static void
> -add_to_routes_ad(struct hmap *routes_ad,
> - const struct nbrec_logical_router_static_route 
> *nb_route,
> - const struct lport_addresses *nexthop_addresses,
> - const struct smap *nb_options, const char *route_table)
> +add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix,
> + unsigned int plen, const struct in6_addr nexthop,
> + const char *origin, const char *route_table,
> + const struct nbrec_logical_router_port *nb_lrp,
> + const struct nbrec_logical_router_static_route 
> *nb_route)
> +{
> +if (route_table == NULL) {
> +route_table = "";
> +}
> +
> +uint hash = ic_route_hash(, plen, , origin, 
> route_table);
> +
> +if (!ic_route_find(routes_ad, , plen, , origin, 
> route_table,
> +   hash)) {
> +struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
> +ic_route->prefix = prefix;
> +ic_route->plen = plen;
> +ic_route->nexthop = nexthop;
> +ic_route->nb_route = nb_route;
> +ic_route->origin = origin;
> +ic_route->route_table = route_table;
> +ic_route->nb_lrp = nb_lrp;
> +hmap_insert(routes_ad, _route->node, hash);
> +} else {
> +VLOG_WARN("Duplicate route advertisement was suppressed! NB 
> route "
> +  "uuid: "UUID_FMT,
> +  UUID_ARGS(_route->header_.uuid));
 
 I think this should be rate limited.
>>> 
>>> Agree, will fix this in v3.
>>> 
 
> +}
> +}
> +
> +static void
> +add_static_to_routes_ad(
> +struct hmap *routes_ad,
> +const struct nbrec_logical_router_static_route *nb_route,
> +const struct lport_addresses *nexthop_addresses,
> +const struct smap *nb_options, const char *route_table)
> {
>if (strcmp(route_table, nb_route->route_table)) {
>if (VLOG_IS_DBG_ENABLED()) {

Re: [ovs-dev] [PATCH ovn v2 3/4] ic: prevent advertising/learning multiple same routes

[Numan Siddique]

On Mon, Dec 12, 2022 at 5:41 AM Dumitru Ceara  wrote:
>
> On 12/9/22 19:42, Vladislav Odintsov wrote:
> > Hi Dumitru,
>
> Hi Vladislav,
>
> >
> > please see answers inline.
> >
> > Regards,
> > Vladislav Odintsov
> >
> >> On 9 Dec 2022, at 17:37, Dumitru Ceara  wrote:
> >>
> >> On 12/6/22 11:20, Vladislav Odintsov wrote:
> >>> Signed-off-by: Vladislav Odintsov  >>> >
> >>> ---
> >>
> >> Hi Vladislav,
> >>
> >>> ic/ovn-ic.c | 83 +++--
> >>> ovn-ic-sb.ovsschema |  6 ++--
> >>> tests/ovn-ic.at | 60 
> >>> 3 files changed, 114 insertions(+), 35 deletions(-)
> >>>
> >>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> >>> index 9e2369fef..64307d8c4 100644
> >>> --- a/ic/ovn-ic.c
> >>> +++ b/ic/ovn-ic.c
> >>> @@ -884,10 +884,12 @@ ic_route_hash(const struct in6_addr *prefix, 
> >>> unsigned int plen,
> >>> static struct ic_route_info *
> >>> ic_route_find(struct hmap *routes, const struct in6_addr *prefix,
> >>>   unsigned int plen, const struct in6_addr *nexthop,
> >>> -  const char *origin, char *route_table)
> >>> +  const char *origin, const char *route_table, uint32_t hash)
> >>> {
> >>> struct ic_route_info *r;
> >>> -uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, 
> >>> route_table);
> >>> +if (!hash) {
> >>> +hash = ic_route_hash(prefix, plen, nexthop, origin, route_table);
> >>> +}
> >>> HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) {
> >>> if (ipv6_addr_equals(>prefix, prefix) &&
> >>> r->plen == plen &&
> >>> @@ -945,8 +947,8 @@ add_to_routes_learned(struct hmap *routes_learned,
> >>> }
> >>> const char *origin = smap_get_def(_route->options, "origin", "");
> >>> if (ic_route_find(routes_learned, , plen, , origin,
> >>> -  nb_route->route_table)) {
> >>> -/* Route is already added to learned in previous iteration. */
> >>> +  nb_route->route_table, 0)) {
> >>> +/* Route was added to learned on previous iteration. */
> >>> return true;
> >>> }
> >>>
> >>> @@ -1093,10 +1095,42 @@ route_need_advertise(const char *policy,
> >>> }
> >>>
> >>> static void
> >>> -add_to_routes_ad(struct hmap *routes_ad,
> >>> - const struct nbrec_logical_router_static_route 
> >>> *nb_route,
> >>> - const struct lport_addresses *nexthop_addresses,
> >>> - const struct smap *nb_options, const char *route_table)
> >>> +add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix,
> >>> + unsigned int plen, const struct in6_addr nexthop,
> >>> + const char *origin, const char *route_table,
> >>> + const struct nbrec_logical_router_port *nb_lrp,
> >>> + const struct nbrec_logical_router_static_route 
> >>> *nb_route)
> >>> +{
> >>> +if (route_table == NULL) {
> >>> +route_table = "";
> >>> +}
> >>> +
> >>> +uint hash = ic_route_hash(, plen, , origin, 
> >>> route_table);
> >>> +
> >>> +if (!ic_route_find(routes_ad, , plen, , origin, 
> >>> route_table,
> >>> +   hash)) {
> >>> +struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
> >>> +ic_route->prefix = prefix;
> >>> +ic_route->plen = plen;
> >>> +ic_route->nexthop = nexthop;
> >>> +ic_route->nb_route = nb_route;
> >>> +ic_route->origin = origin;
> >>> +ic_route->route_table = route_table;
> >>> +ic_route->nb_lrp = nb_lrp;
> >>> +hmap_insert(routes_ad, _route->node, hash);
> >>> +} else {
> >>> +VLOG_WARN("Duplicate route advertisement was suppressed! NB 
> >>> route "
> >>> +  "uuid: "UUID_FMT,
> >>> +  UUID_ARGS(_route->header_.uuid));
> >>
> >> I think this should be rate limited.
> >
> > Agree, will fix this in v3.
> >
> >>
> >>> +}
> >>> +}
> >>> +
> >>> +static void
> >>> +add_static_to_routes_ad(
> >>> +struct hmap *routes_ad,
> >>> +const struct nbrec_logical_router_static_route *nb_route,
> >>> +const struct lport_addresses *nexthop_addresses,
> >>> +const struct smap *nb_options, const char *route_table)
> >>> {
> >>> if (strcmp(route_table, nb_route->route_table)) {
> >>> if (VLOG_IS_DBG_ENABLED()) {
> >>> @@ -1145,16 +1179,8 @@ add_to_routes_ad(struct hmap *routes_ad,
> >>> ds_destroy();
> >>> }
> >>>
> >>> -struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
> >>> -ic_route->prefix = prefix;
> >>> -ic_route->plen = plen;
> >>> -ic_route->nexthop = nexthop;
> >>> -ic_route->nb_route = nb_route;
> >>> -ic_route->origin = ROUTE_ORIGIN_STATIC;
> >>> -ic_route->route_table = nb_route->route_table;
> >>> -hmap_insert(routes_ad, _route->node,
> >>> -ic_route_hash(, plen, , 
> >>> ROUTE_ORIGIN_STATIC,
> >>> -  

Re: [ovs-dev] [PATCH ovn v2 3/4] ic: prevent advertising/learning multiple same routes

[Dumitru Ceara]

On 12/9/22 19:42, Vladislav Odintsov wrote:
> Hi Dumitru,

Hi Vladislav,

> 
> please see answers inline.
> 
> Regards,
> Vladislav Odintsov
> 
>> On 9 Dec 2022, at 17:37, Dumitru Ceara  wrote:
>>
>> On 12/6/22 11:20, Vladislav Odintsov wrote:
>>> Signed-off-by: Vladislav Odintsov >> >
>>> ---
>>
>> Hi Vladislav,
>>
>>> ic/ovn-ic.c | 83 +++--
>>> ovn-ic-sb.ovsschema |  6 ++--
>>> tests/ovn-ic.at | 60 
>>> 3 files changed, 114 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>>> index 9e2369fef..64307d8c4 100644
>>> --- a/ic/ovn-ic.c
>>> +++ b/ic/ovn-ic.c
>>> @@ -884,10 +884,12 @@ ic_route_hash(const struct in6_addr *prefix, unsigned 
>>> int plen,
>>> static struct ic_route_info *
>>> ic_route_find(struct hmap *routes, const struct in6_addr *prefix,
>>>   unsigned int plen, const struct in6_addr *nexthop,
>>> -  const char *origin, char *route_table)
>>> +  const char *origin, const char *route_table, uint32_t hash)
>>> {
>>> struct ic_route_info *r;
>>> -uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, 
>>> route_table);
>>> +if (!hash) {
>>> +hash = ic_route_hash(prefix, plen, nexthop, origin, route_table);
>>> +}
>>> HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) {
>>> if (ipv6_addr_equals(>prefix, prefix) &&
>>> r->plen == plen &&
>>> @@ -945,8 +947,8 @@ add_to_routes_learned(struct hmap *routes_learned,
>>> }
>>> const char *origin = smap_get_def(_route->options, "origin", "");
>>> if (ic_route_find(routes_learned, , plen, , origin,
>>> -  nb_route->route_table)) {
>>> -/* Route is already added to learned in previous iteration. */
>>> +  nb_route->route_table, 0)) {
>>> +/* Route was added to learned on previous iteration. */
>>> return true;
>>> }
>>>
>>> @@ -1093,10 +1095,42 @@ route_need_advertise(const char *policy,
>>> }
>>>
>>> static void
>>> -add_to_routes_ad(struct hmap *routes_ad,
>>> - const struct nbrec_logical_router_static_route *nb_route,
>>> - const struct lport_addresses *nexthop_addresses,
>>> - const struct smap *nb_options, const char *route_table)
>>> +add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix,
>>> + unsigned int plen, const struct in6_addr nexthop,
>>> + const char *origin, const char *route_table,
>>> + const struct nbrec_logical_router_port *nb_lrp,
>>> + const struct nbrec_logical_router_static_route *nb_route)
>>> +{
>>> +if (route_table == NULL) {
>>> +route_table = "";
>>> +}
>>> +
>>> +uint hash = ic_route_hash(, plen, , origin, 
>>> route_table);
>>> +
>>> +if (!ic_route_find(routes_ad, , plen, , origin, 
>>> route_table,
>>> +   hash)) {
>>> +struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
>>> +ic_route->prefix = prefix;
>>> +ic_route->plen = plen;
>>> +ic_route->nexthop = nexthop;
>>> +ic_route->nb_route = nb_route;
>>> +ic_route->origin = origin;
>>> +ic_route->route_table = route_table;
>>> +ic_route->nb_lrp = nb_lrp;
>>> +hmap_insert(routes_ad, _route->node, hash);
>>> +} else {
>>> +VLOG_WARN("Duplicate route advertisement was suppressed! NB route "
>>> +  "uuid: "UUID_FMT,
>>> +  UUID_ARGS(_route->header_.uuid));
>>
>> I think this should be rate limited.
> 
> Agree, will fix this in v3.
> 
>>
>>> +}
>>> +}
>>> +
>>> +static void
>>> +add_static_to_routes_ad(
>>> +struct hmap *routes_ad,
>>> +const struct nbrec_logical_router_static_route *nb_route,
>>> +const struct lport_addresses *nexthop_addresses,
>>> +const struct smap *nb_options, const char *route_table)
>>> {
>>> if (strcmp(route_table, nb_route->route_table)) {
>>> if (VLOG_IS_DBG_ENABLED()) {
>>> @@ -1145,16 +1179,8 @@ add_to_routes_ad(struct hmap *routes_ad,
>>> ds_destroy();
>>> }
>>>
>>> -struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
>>> -ic_route->prefix = prefix;
>>> -ic_route->plen = plen;
>>> -ic_route->nexthop = nexthop;
>>> -ic_route->nb_route = nb_route;
>>> -ic_route->origin = ROUTE_ORIGIN_STATIC;
>>> -ic_route->route_table = nb_route->route_table;
>>> -hmap_insert(routes_ad, _route->node,
>>> -ic_route_hash(, plen, , ROUTE_ORIGIN_STATIC,
>>> -  nb_route->route_table));
>>> +add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_STATIC,
>>> + nb_route->route_table, NULL, nb_route);
>>> }
>>>
>>> static void
>>> @@ -1198,18 +1224,9 @@ add_network_to_routes_ad(struct hmap *routes_ad, 
>>> const char *network,
>>>  

Re: [ovs-dev] [PATCH ovn v2 3/4] ic: prevent advertising/learning multiple same routes

[Vladislav Odintsov]

Hi Dumitru,

please see answers inline.

Regards,
Vladislav Odintsov

> On 9 Dec 2022, at 17:37, Dumitru Ceara  wrote:
> 
> On 12/6/22 11:20, Vladislav Odintsov wrote:
>> Signed-off-by: Vladislav Odintsov > >
>> ---
> 
> Hi Vladislav,
> 
>> ic/ovn-ic.c | 83 +++--
>> ovn-ic-sb.ovsschema |  6 ++--
>> tests/ovn-ic.at | 60 
>> 3 files changed, 114 insertions(+), 35 deletions(-)
>> 
>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>> index 9e2369fef..64307d8c4 100644
>> --- a/ic/ovn-ic.c
>> +++ b/ic/ovn-ic.c
>> @@ -884,10 +884,12 @@ ic_route_hash(const struct in6_addr *prefix, unsigned 
>> int plen,
>> static struct ic_route_info *
>> ic_route_find(struct hmap *routes, const struct in6_addr *prefix,
>>   unsigned int plen, const struct in6_addr *nexthop,
>> -  const char *origin, char *route_table)
>> +  const char *origin, const char *route_table, uint32_t hash)
>> {
>> struct ic_route_info *r;
>> -uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, 
>> route_table);
>> +if (!hash) {
>> +hash = ic_route_hash(prefix, plen, nexthop, origin, route_table);
>> +}
>> HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) {
>> if (ipv6_addr_equals(>prefix, prefix) &&
>> r->plen == plen &&
>> @@ -945,8 +947,8 @@ add_to_routes_learned(struct hmap *routes_learned,
>> }
>> const char *origin = smap_get_def(_route->options, "origin", "");
>> if (ic_route_find(routes_learned, , plen, , origin,
>> -  nb_route->route_table)) {
>> -/* Route is already added to learned in previous iteration. */
>> +  nb_route->route_table, 0)) {
>> +/* Route was added to learned on previous iteration. */
>> return true;
>> }
>> 
>> @@ -1093,10 +1095,42 @@ route_need_advertise(const char *policy,
>> }
>> 
>> static void
>> -add_to_routes_ad(struct hmap *routes_ad,
>> - const struct nbrec_logical_router_static_route *nb_route,
>> - const struct lport_addresses *nexthop_addresses,
>> - const struct smap *nb_options, const char *route_table)
>> +add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix,
>> + unsigned int plen, const struct in6_addr nexthop,
>> + const char *origin, const char *route_table,
>> + const struct nbrec_logical_router_port *nb_lrp,
>> + const struct nbrec_logical_router_static_route *nb_route)
>> +{
>> +if (route_table == NULL) {
>> +route_table = "";
>> +}
>> +
>> +uint hash = ic_route_hash(, plen, , origin, route_table);
>> +
>> +if (!ic_route_find(routes_ad, , plen, , origin, 
>> route_table,
>> +   hash)) {
>> +struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
>> +ic_route->prefix = prefix;
>> +ic_route->plen = plen;
>> +ic_route->nexthop = nexthop;
>> +ic_route->nb_route = nb_route;
>> +ic_route->origin = origin;
>> +ic_route->route_table = route_table;
>> +ic_route->nb_lrp = nb_lrp;
>> +hmap_insert(routes_ad, _route->node, hash);
>> +} else {
>> +VLOG_WARN("Duplicate route advertisement was suppressed! NB route "
>> +  "uuid: "UUID_FMT,
>> +  UUID_ARGS(_route->header_.uuid));
> 
> I think this should be rate limited.

Agree, will fix this in v3.

> 
>> +}
>> +}
>> +
>> +static void
>> +add_static_to_routes_ad(
>> +struct hmap *routes_ad,
>> +const struct nbrec_logical_router_static_route *nb_route,
>> +const struct lport_addresses *nexthop_addresses,
>> +const struct smap *nb_options, const char *route_table)
>> {
>> if (strcmp(route_table, nb_route->route_table)) {
>> if (VLOG_IS_DBG_ENABLED()) {
>> @@ -1145,16 +1179,8 @@ add_to_routes_ad(struct hmap *routes_ad,
>> ds_destroy();
>> }
>> 
>> -struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
>> -ic_route->prefix = prefix;
>> -ic_route->plen = plen;
>> -ic_route->nexthop = nexthop;
>> -ic_route->nb_route = nb_route;
>> -ic_route->origin = ROUTE_ORIGIN_STATIC;
>> -ic_route->route_table = nb_route->route_table;
>> -hmap_insert(routes_ad, _route->node,
>> -ic_route_hash(, plen, , ROUTE_ORIGIN_STATIC,
>> -  nb_route->route_table));
>> +add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_STATIC,
>> + nb_route->route_table, NULL, nb_route);
>> }
>> 
>> static void
>> @@ -1198,18 +1224,9 @@ add_network_to_routes_ad(struct hmap *routes_ad, 
>> const char *network,
>> ds_destroy();
>> }
>> 
>> -struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
>> -ic_route->prefix = prefix;
>> -ic_route->plen = plen;
>> -ic_route->nexthop 

Re: [ovs-dev] [PATCH ovn v2 3/4] ic: prevent advertising/learning multiple same routes

[Dumitru Ceara]

On 12/6/22 11:20, Vladislav Odintsov wrote:
> Signed-off-by: Vladislav Odintsov 
> ---

Hi Vladislav,

>  ic/ovn-ic.c | 83 +++--
>  ovn-ic-sb.ovsschema |  6 ++--
>  tests/ovn-ic.at | 60 
>  3 files changed, 114 insertions(+), 35 deletions(-)
> 
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index 9e2369fef..64307d8c4 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -884,10 +884,12 @@ ic_route_hash(const struct in6_addr *prefix, unsigned 
> int plen,
>  static struct ic_route_info *
>  ic_route_find(struct hmap *routes, const struct in6_addr *prefix,
>unsigned int plen, const struct in6_addr *nexthop,
> -  const char *origin, char *route_table)
> +  const char *origin, const char *route_table, uint32_t hash)
>  {
>  struct ic_route_info *r;
> -uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, 
> route_table);
> +if (!hash) {
> +hash = ic_route_hash(prefix, plen, nexthop, origin, route_table);
> +}
>  HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) {
>  if (ipv6_addr_equals(>prefix, prefix) &&
>  r->plen == plen &&
> @@ -945,8 +947,8 @@ add_to_routes_learned(struct hmap *routes_learned,
>  }
>  const char *origin = smap_get_def(_route->options, "origin", "");
>  if (ic_route_find(routes_learned, , plen, , origin,
> -  nb_route->route_table)) {
> -/* Route is already added to learned in previous iteration. */
> +  nb_route->route_table, 0)) {
> +/* Route was added to learned on previous iteration. */
>  return true;
>  }
>  
> @@ -1093,10 +1095,42 @@ route_need_advertise(const char *policy,
>  }
>  
>  static void
> -add_to_routes_ad(struct hmap *routes_ad,
> - const struct nbrec_logical_router_static_route *nb_route,
> - const struct lport_addresses *nexthop_addresses,
> - const struct smap *nb_options, const char *route_table)
> +add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix,
> + unsigned int plen, const struct in6_addr nexthop,
> + const char *origin, const char *route_table,
> + const struct nbrec_logical_router_port *nb_lrp,
> + const struct nbrec_logical_router_static_route *nb_route)
> +{
> +if (route_table == NULL) {
> +route_table = "";
> +}
> +
> +uint hash = ic_route_hash(, plen, , origin, route_table);
> +
> +if (!ic_route_find(routes_ad, , plen, , origin, 
> route_table,
> +   hash)) {
> +struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
> +ic_route->prefix = prefix;
> +ic_route->plen = plen;
> +ic_route->nexthop = nexthop;
> +ic_route->nb_route = nb_route;
> +ic_route->origin = origin;
> +ic_route->route_table = route_table;
> +ic_route->nb_lrp = nb_lrp;
> +hmap_insert(routes_ad, _route->node, hash);
> +} else {
> +VLOG_WARN("Duplicate route advertisement was suppressed! NB route "
> +  "uuid: "UUID_FMT,
> +  UUID_ARGS(_route->header_.uuid));

I think this should be rate limited.

> +}
> +}
> +
> +static void
> +add_static_to_routes_ad(
> +struct hmap *routes_ad,
> +const struct nbrec_logical_router_static_route *nb_route,
> +const struct lport_addresses *nexthop_addresses,
> +const struct smap *nb_options, const char *route_table)
>  {
>  if (strcmp(route_table, nb_route->route_table)) {
>  if (VLOG_IS_DBG_ENABLED()) {
> @@ -1145,16 +1179,8 @@ add_to_routes_ad(struct hmap *routes_ad,
>  ds_destroy();
>  }
>  
> -struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
> -ic_route->prefix = prefix;
> -ic_route->plen = plen;
> -ic_route->nexthop = nexthop;
> -ic_route->nb_route = nb_route;
> -ic_route->origin = ROUTE_ORIGIN_STATIC;
> -ic_route->route_table = nb_route->route_table;
> -hmap_insert(routes_ad, _route->node,
> -ic_route_hash(, plen, , ROUTE_ORIGIN_STATIC,
> -  nb_route->route_table));
> +add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_STATIC,
> + nb_route->route_table, NULL, nb_route);
>  }
>  
>  static void
> @@ -1198,18 +1224,9 @@ add_network_to_routes_ad(struct hmap *routes_ad, const 
> char *network,
>  ds_destroy();
>  }
>  
> -struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
> -ic_route->prefix = prefix;
> -ic_route->plen = plen;
> -ic_route->nexthop = nexthop;
> -ic_route->nb_lrp = nb_lrp;
> -ic_route->origin = ROUTE_ORIGIN_CONNECTED;
> -
>  /* directly-connected routes go to  route table */
> -ic_route->route_table = NULL;
> -hmap_insert(routes_ad, _route->node,
> -ic_route_hash(, 

[ovs-dev] [PATCH ovn v2 3/4] ic: prevent advertising/learning multiple same routes

[Vladislav Odintsov]

Signed-off-by: Vladislav Odintsov 
---
 ic/ovn-ic.c | 83 +++--
 ovn-ic-sb.ovsschema |  6 ++--
 tests/ovn-ic.at | 60 
 3 files changed, 114 insertions(+), 35 deletions(-)

diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 9e2369fef..64307d8c4 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -884,10 +884,12 @@ ic_route_hash(const struct in6_addr *prefix, unsigned int 
plen,
 static struct ic_route_info *
 ic_route_find(struct hmap *routes, const struct in6_addr *prefix,
   unsigned int plen, const struct in6_addr *nexthop,
-  const char *origin, char *route_table)
+  const char *origin, const char *route_table, uint32_t hash)
 {
 struct ic_route_info *r;
-uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, route_table);
+if (!hash) {
+hash = ic_route_hash(prefix, plen, nexthop, origin, route_table);
+}
 HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) {
 if (ipv6_addr_equals(>prefix, prefix) &&
 r->plen == plen &&
@@ -945,8 +947,8 @@ add_to_routes_learned(struct hmap *routes_learned,
 }
 const char *origin = smap_get_def(_route->options, "origin", "");
 if (ic_route_find(routes_learned, , plen, , origin,
-  nb_route->route_table)) {
-/* Route is already added to learned in previous iteration. */
+  nb_route->route_table, 0)) {
+/* Route was added to learned on previous iteration. */
 return true;
 }
 
@@ -1093,10 +1095,42 @@ route_need_advertise(const char *policy,
 }
 
 static void
-add_to_routes_ad(struct hmap *routes_ad,
- const struct nbrec_logical_router_static_route *nb_route,
- const struct lport_addresses *nexthop_addresses,
- const struct smap *nb_options, const char *route_table)
+add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix,
+ unsigned int plen, const struct in6_addr nexthop,
+ const char *origin, const char *route_table,
+ const struct nbrec_logical_router_port *nb_lrp,
+ const struct nbrec_logical_router_static_route *nb_route)
+{
+if (route_table == NULL) {
+route_table = "";
+}
+
+uint hash = ic_route_hash(, plen, , origin, route_table);
+
+if (!ic_route_find(routes_ad, , plen, , origin, route_table,
+   hash)) {
+struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
+ic_route->prefix = prefix;
+ic_route->plen = plen;
+ic_route->nexthop = nexthop;
+ic_route->nb_route = nb_route;
+ic_route->origin = origin;
+ic_route->route_table = route_table;
+ic_route->nb_lrp = nb_lrp;
+hmap_insert(routes_ad, _route->node, hash);
+} else {
+VLOG_WARN("Duplicate route advertisement was suppressed! NB route "
+  "uuid: "UUID_FMT,
+  UUID_ARGS(_route->header_.uuid));
+}
+}
+
+static void
+add_static_to_routes_ad(
+struct hmap *routes_ad,
+const struct nbrec_logical_router_static_route *nb_route,
+const struct lport_addresses *nexthop_addresses,
+const struct smap *nb_options, const char *route_table)
 {
 if (strcmp(route_table, nb_route->route_table)) {
 if (VLOG_IS_DBG_ENABLED()) {
@@ -1145,16 +1179,8 @@ add_to_routes_ad(struct hmap *routes_ad,
 ds_destroy();
 }
 
-struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
-ic_route->prefix = prefix;
-ic_route->plen = plen;
-ic_route->nexthop = nexthop;
-ic_route->nb_route = nb_route;
-ic_route->origin = ROUTE_ORIGIN_STATIC;
-ic_route->route_table = nb_route->route_table;
-hmap_insert(routes_ad, _route->node,
-ic_route_hash(, plen, , ROUTE_ORIGIN_STATIC,
-  nb_route->route_table));
+add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_STATIC,
+ nb_route->route_table, NULL, nb_route);
 }
 
 static void
@@ -1198,18 +1224,9 @@ add_network_to_routes_ad(struct hmap *routes_ad, const 
char *network,
 ds_destroy();
 }
 
-struct ic_route_info *ic_route = xzalloc(sizeof *ic_route);
-ic_route->prefix = prefix;
-ic_route->plen = plen;
-ic_route->nexthop = nexthop;
-ic_route->nb_lrp = nb_lrp;
-ic_route->origin = ROUTE_ORIGIN_CONNECTED;
-
 /* directly-connected routes go to  route table */
-ic_route->route_table = NULL;
-hmap_insert(routes_ad, _route->node,
-ic_route_hash(, plen, ,
-  ROUTE_ORIGIN_CONNECTED, ""));
+add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_CONNECTED,
+ NULL, nb_lrp, NULL);
 }
 
 static bool
@@ -1369,7 +1386,7 @@ sync_learned_routes(struct ic_context *ctx,
 struct ic_route_info *route_learned
 =