[ovs-dev,v2,1/2] ovn-ic fix multiple routers in an az

previously if multiple routers in the same az are connected to the same
transit switch then ovn-ic would only propagate the routes of one of
these routers to the ic-sb.
This commit fixes this behaviour and allows multiple routers in a single
az to use route advertisements.

Co-authored-by: Maxim Korezkij <maxim.korezkij@mail.schwarz>
Signed-off-by: Maxim Korezkij <maxim.korezkij@mail.schwarz>
Signed-off-by: Felix Huettner <felix.huettner@mail.schwarz>
---
 ic/ovn-ic.c     | 27 +++++++++++-----
 tests/ovn-ic.at | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 8 deletions(-)

--
2.42.0

Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für die Verwertung durch den vorgesehenen Empfänger bestimmt.
Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender bitte unverzüglich in Kenntnis und löschen diese E Mail.

Hinweise zum Datenschutz finden Sie hier<https://www.datenschutz.schwarz>.


This e-mail may contain confidential content and is intended only for the specified recipient/s.
If you are not the intended recipient, please inform the sender immediately and delete this e-mail.

Information on data protection can be found here<https://www.datenschutz.schwarz>.

On Wed, Aug 30, 2023 at 8:20 AM Felix Huettner via dev <
ovs-dev@openvswitch.org> wrote:

> previously if multiple routers in the same az are connected to the same
> transit switch then ovn-ic would only propagate the routes of one of
> these routers to the ic-sb.
> This commit fixes this behaviour and allows multiple routers in a single
> az to use route advertisements.
>
> Co-authored-by: Maxim Korezkij <maxim.korezkij@mail.schwarz>
> Signed-off-by: Maxim Korezkij <maxim.korezkij@mail.schwarz>
> Signed-off-by: Felix Huettner <felix.huettner@mail.schwarz>
>

Hi Felix,

I have a few minor comments, see below.

---
>  ic/ovn-ic.c     | 27 +++++++++++-----
>  tests/ovn-ic.at | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+), 8 deletions(-)
>
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index db7e86bc1..ec749e25f 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -1587,9 +1587,9 @@ build_ts_routes_to_adv(struct ic_context *ctx,
>  }
>
>  static void
> -advertise_lr_routes(struct ic_context *ctx,
> -                    const struct icsbrec_availability_zone *az,
> -                    struct ic_router_info *ic_lr)
> +collect_lr_routes(struct ic_context *ctx,
> +                    struct ic_router_info *ic_lr,
> +                    struct shash *routes_ad_by_ts)
>

The arguments are not properly aligned.


>  {
>      const struct nbrec_nb_global *nb_global =
>          nbrec_nb_global_first(ctx->ovnnb_idl);
> @@ -1600,7 +1600,7 @@ advertise_lr_routes(struct ic_context *ctx,
>      struct lport_addresses ts_port_addrs;
>      const struct icnbrec_transit_switch *key;
>
> -    struct hmap routes_ad = HMAP_INITIALIZER(&routes_ad);
> +    struct hmap *routes_ad;
>      for (int i = 0; i < ic_lr->n_isb_pbs; i++) {
>          isb_pb = ic_lr->isb_pbs[i];
>          key = icnbrec_transit_switch_index_init_row(
> @@ -1609,6 +1609,12 @@ advertise_lr_routes(struct ic_context *ctx,
>          ts_name = icnbrec_transit_switch_index_find(
>              ctx->icnbrec_transit_switch_by_name, key)->name;
>          icnbrec_transit_switch_index_destroy_row(key);
> +        routes_ad = shash_find_data(routes_ad_by_ts, ts_name);
> +        if (!routes_ad) {
> +            routes_ad = xzalloc(sizeof *routes_ad);
> +            hmap_init(routes_ad);
> +            shash_add(routes_ad_by_ts, ts_name, routes_ad);
> +        }
>
>          if (!extract_lsp_addresses(isb_pb->address, &ts_port_addrs)) {
>              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> @@ -1620,12 +1626,10 @@ advertise_lr_routes(struct ic_context *ctx,
>          }
>          lrp_name = get_lrp_name_by_ts_port_name(ctx,
> isb_pb->logical_port);
>          route_table = get_route_table_by_lrp_name(ctx, lrp_name);
> -        build_ts_routes_to_adv(ctx, ic_lr, &routes_ad, &ts_port_addrs,
> +        build_ts_routes_to_adv(ctx, ic_lr, routes_ad, &ts_port_addrs,
>                                 nb_global, route_table);
> -        advertise_routes(ctx, az, ts_name, &routes_ad);
>          destroy_lport_addresses(&ts_port_addrs);
>      }
> -    hmap_destroy(&routes_ad);
>  }
>
>  static void
> @@ -1726,14 +1730,21 @@ route_run(struct ic_context *ctx,
>      icsbrec_port_binding_index_destroy_row(isb_pb_key);
>
>      struct ic_router_info *ic_lr;
> +    struct shash routes_ad_by_ts = SHASH_INITIALIZER(&routes_ad_by_ts);
>      HMAP_FOR_EACH_SAFE (ic_lr, node, &ic_lrs) {
> -        advertise_lr_routes(ctx, az, ic_lr);
> +        collect_lr_routes(ctx, ic_lr, &routes_ad_by_ts);
>          sync_learned_routes(ctx, az, ic_lr);
>          free(ic_lr->isb_pbs);
>          hmap_destroy(&ic_lr->routes_learned);
>          hmap_remove(&ic_lrs, &ic_lr->node);
>          free(ic_lr);
>      }
> +    struct shash_node *node;
> +    SHASH_FOR_EACH_SAFE (node, &routes_ad_by_ts) {
>

The SHASH iteration doesn't have to be SAFE, we are not removing anything
from it during the iteration.


> +        advertise_routes(ctx, az, node->name, node->data);
> +        hmap_destroy(node->data);
> +    }
> +    shash_destroy_free_data(&routes_ad_by_ts);
>      hmap_destroy(&ic_lrs);
>  }
>
> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
> index a654e59fe..8ef2362c4 100644
> --- a/tests/ovn-ic.at
> +++ b/tests/ovn-ic.at
> @@ -1164,3 +1164,89 @@ AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr12 |
> grep dst-ip | sort], [0], [d
>
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-ic -- route sync -- multiple logical routers])
> +
> +ovn_init_ic_db
> +ovn-ic-nbctl ts-add ts1
> +
> +for i in 1 2; do
> +    ovn_start az$i
> +    ovn_as az$i
> +
> +    # Enable route learning at AZ level
> +    ovn-nbctl set nb_global . options:ic-route-learn=true
> +    # Enable route advertising at AZ level
> +    ovn-nbctl set nb_global . options:ic-route-adv=true
> +done
> +
> +# Create new transit switches and LRs. Test topology is next:
> +#
> +# logical router (lr11) - transit switch (ts1) - logical router (lr21)
> +#                                              \- logical router (lr22)
> +#
> +# each LR has one connected subnet except TS port
> +
> +
> +# create lr11, lr21, lr22, ts1 and connect them
> +ovn-ic-nbctl ts-add ts1
> +
> +ovn_as az1
> +
> +lr=lr11
> +ovn-nbctl lr-add $lr
> +
> +lrp=lrp-$lr-ts1
> +lsp=lsp-ts1-$lr
> +# Create LRP and connect to TS
> +ovn-nbctl lrp-add $lr $lrp aa:aa:aa:aa:a1:01 169.254.10.11/24
> +ovn-nbctl lsp-add ts1 $lsp \
> +        -- lsp-set-addresses $lsp router \
> +        -- lsp-set-type $lsp router \
> +        -- lsp-set-options $lsp router-port=$lrp
> +
> +ovn_as az2
> +for i in 1 2; do
> +    lr=lr2$i
> +    ovn-nbctl lr-add $lr
> +
> +    lrp=lrp-$lr-ts1
> +    lsp=lsp-ts1-$lr
> +    # Create LRP and connect to TS
> +    ovn-nbctl lrp-add $lr $lrp aa:aa:aa:aa:a2:0$i 169.254.10.2$i/24
> +    ovn-nbctl lsp-add ts1 $lsp \
> +            -- lsp-set-addresses $lsp router \
> +            -- lsp-set-type $lsp router \
> +            -- lsp-set-options $lsp router-port=$lrp
> +done
> +
> +
> +# Create directly-connected routes
> +ovn_as az1 ovn-nbctl lrp-add lr11 lrp-lr11 aa:aa:aa:aa:bb:01 "
> 192.168.0.1/24"
> +ovn_as az2 ovn-nbctl lrp-add lr21 lrp-lr21 aa:aa:aa:aa:bc:01 "
> 192.168.1.1/24"
> +ovn_as az2 ovn-nbctl lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bc:02 "
> 192.168.2.1/24"
> +
> +# Test direct routes from lr21 and lr22 were learned to lr11
> +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 |
> +             grep learned | awk '{print $1, $2}' | sort ], [0], [dnl
> +192.168.1.0/24 169.254.10.21
> +192.168.2.0/24 169.254.10.22
> +])
> +
> +# Test direct routes from lr11 and lr22 were learned to lr21
> +AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr21 | grep 192.168 |
> +             grep learned | awk '{print $1, $2}' | sort ], [0], [dnl
> +192.168.0.0/24 169.254.10.11
> +])
> +
> +# Test direct routes from lr11 and lr21 were learned to lr22
> +AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr22 | grep 192.168 |
> +             grep learned | awk '{print $1, $2}' | sort ], [0], [dnl
> +192.168.0.0/24 169.254.10.11
> +])
> +
> +OVN_CLEANUP_IC([az1], [az2])
> +
> +AT_CLEANUP
> +])
> \ No newline at end of file
>
>
Missing newline at the end of the file.


> --
> 2.42.0
>
> Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für
> die Verwertung durch den vorgesehenen Empfänger bestimmt.
> Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender
> bitte unverzüglich in Kenntnis und löschen diese E Mail.
>
> Hinweise zum Datenschutz finden Sie hier<https://www.datenschutz.schwarz>.
>
>
> This e-mail may contain confidential content and is intended only for the
> specified recipient/s.
> If you are not the intended recipient, please inform the sender
> immediately and delete this e-mail.
>
> Information on data protection can be found here<
> https://www.datenschutz.schwarz>.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


Thanks,
Ales

Hi Ales,

thanks for the feedback. That will all be addressed in v3.

Regards
Felix
On Tue, Sep 12, 2023 at 08:18:18AM +0200, Ales Musil wrote:
> On Wed, Aug 30, 2023 at 8:20 AM Felix Huettner via dev <
> ovs-dev@openvswitch.org> wrote:
>
> > previously if multiple routers in the same az are connected to the same
> > transit switch then ovn-ic would only propagate the routes of one of
> > these routers to the ic-sb.
> > This commit fixes this behaviour and allows multiple routers in a single
> > az to use route advertisements.
> >
> > Co-authored-by: Maxim Korezkij <maxim.korezkij@mail.schwarz>
> > Signed-off-by: Maxim Korezkij <maxim.korezkij@mail.schwarz>
> > Signed-off-by: Felix Huettner <felix.huettner@mail.schwarz>
> >
>
> Hi Felix,
>
> I have a few minor comments, see below.
>
> ---
> >  ic/ovn-ic.c     | 27 +++++++++++-----
> >  tests/ovn-ic.at | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 105 insertions(+), 8 deletions(-)
> >
> > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> > index db7e86bc1..ec749e25f 100644
> > --- a/ic/ovn-ic.c
> > +++ b/ic/ovn-ic.c
> > @@ -1587,9 +1587,9 @@ build_ts_routes_to_adv(struct ic_context *ctx,
> >  }
> >
> >  static void
> > -advertise_lr_routes(struct ic_context *ctx,
> > -                    const struct icsbrec_availability_zone *az,
> > -                    struct ic_router_info *ic_lr)
> > +collect_lr_routes(struct ic_context *ctx,
> > +                    struct ic_router_info *ic_lr,
> > +                    struct shash *routes_ad_by_ts)
> >
>
> The arguments are not properly aligned.
>
>
> >  {
> >      const struct nbrec_nb_global *nb_global =
> >          nbrec_nb_global_first(ctx->ovnnb_idl);
> > @@ -1600,7 +1600,7 @@ advertise_lr_routes(struct ic_context *ctx,
> >      struct lport_addresses ts_port_addrs;
> >      const struct icnbrec_transit_switch *key;
> >
> > -    struct hmap routes_ad = HMAP_INITIALIZER(&routes_ad);
> > +    struct hmap *routes_ad;
> >      for (int i = 0; i < ic_lr->n_isb_pbs; i++) {
> >          isb_pb = ic_lr->isb_pbs[i];
> >          key = icnbrec_transit_switch_index_init_row(
> > @@ -1609,6 +1609,12 @@ advertise_lr_routes(struct ic_context *ctx,
> >          ts_name = icnbrec_transit_switch_index_find(
> >              ctx->icnbrec_transit_switch_by_name, key)->name;
> >          icnbrec_transit_switch_index_destroy_row(key);
> > +        routes_ad = shash_find_data(routes_ad_by_ts, ts_name);
> > +        if (!routes_ad) {
> > +            routes_ad = xzalloc(sizeof *routes_ad);
> > +            hmap_init(routes_ad);
> > +            shash_add(routes_ad_by_ts, ts_name, routes_ad);
> > +        }
> >
> >          if (!extract_lsp_addresses(isb_pb->address, &ts_port_addrs)) {
> >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > @@ -1620,12 +1626,10 @@ advertise_lr_routes(struct ic_context *ctx,
> >          }
> >          lrp_name = get_lrp_name_by_ts_port_name(ctx,
> > isb_pb->logical_port);
> >          route_table = get_route_table_by_lrp_name(ctx, lrp_name);
> > -        build_ts_routes_to_adv(ctx, ic_lr, &routes_ad, &ts_port_addrs,
> > +        build_ts_routes_to_adv(ctx, ic_lr, routes_ad, &ts_port_addrs,
> >                                 nb_global, route_table);
> > -        advertise_routes(ctx, az, ts_name, &routes_ad);
> >          destroy_lport_addresses(&ts_port_addrs);
> >      }
> > -    hmap_destroy(&routes_ad);
> >  }
> >
> >  static void
> > @@ -1726,14 +1730,21 @@ route_run(struct ic_context *ctx,
> >      icsbrec_port_binding_index_destroy_row(isb_pb_key);
> >
> >      struct ic_router_info *ic_lr;
> > +    struct shash routes_ad_by_ts = SHASH_INITIALIZER(&routes_ad_by_ts);
> >      HMAP_FOR_EACH_SAFE (ic_lr, node, &ic_lrs) {
> > -        advertise_lr_routes(ctx, az, ic_lr);
> > +        collect_lr_routes(ctx, ic_lr, &routes_ad_by_ts);
> >          sync_learned_routes(ctx, az, ic_lr);
> >          free(ic_lr->isb_pbs);
> >          hmap_destroy(&ic_lr->routes_learned);
> >          hmap_remove(&ic_lrs, &ic_lr->node);
> >          free(ic_lr);
> >      }
> > +    struct shash_node *node;
> > +    SHASH_FOR_EACH_SAFE (node, &routes_ad_by_ts) {
> >
>
> The SHASH iteration doesn't have to be SAFE, we are not removing anything
> from it during the iteration.
>
>
> > +        advertise_routes(ctx, az, node->name, node->data);
> > +        hmap_destroy(node->data);
> > +    }
> > +    shash_destroy_free_data(&routes_ad_by_ts);
> >      hmap_destroy(&ic_lrs);
> >  }
> >
> > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
> > index a654e59fe..8ef2362c4 100644
> > --- a/tests/ovn-ic.at
> > +++ b/tests/ovn-ic.at
> > @@ -1164,3 +1164,89 @@ AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr12 |
> > grep dst-ip | sort], [0], [d
> >
> >  AT_CLEANUP
> >  ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ovn-ic -- route sync -- multiple logical routers])
> > +
> > +ovn_init_ic_db
> > +ovn-ic-nbctl ts-add ts1
> > +
> > +for i in 1 2; do
> > +    ovn_start az$i
> > +    ovn_as az$i
> > +
> > +    # Enable route learning at AZ level
> > +    ovn-nbctl set nb_global . options:ic-route-learn=true
> > +    # Enable route advertising at AZ level
> > +    ovn-nbctl set nb_global . options:ic-route-adv=true
> > +done
> > +
> > +# Create new transit switches and LRs. Test topology is next:
> > +#
> > +# logical router (lr11) - transit switch (ts1) - logical router (lr21)
> > +#                                              \- logical router (lr22)
> > +#
> > +# each LR has one connected subnet except TS port
> > +
> > +
> > +# create lr11, lr21, lr22, ts1 and connect them
> > +ovn-ic-nbctl ts-add ts1
> > +
> > +ovn_as az1
> > +
> > +lr=lr11
> > +ovn-nbctl lr-add $lr
> > +
> > +lrp=lrp-$lr-ts1
> > +lsp=lsp-ts1-$lr
> > +# Create LRP and connect to TS
> > +ovn-nbctl lrp-add $lr $lrp aa:aa:aa:aa:a1:01 169.254.10.11/24
> > +ovn-nbctl lsp-add ts1 $lsp \
> > +        -- lsp-set-addresses $lsp router \
> > +        -- lsp-set-type $lsp router \
> > +        -- lsp-set-options $lsp router-port=$lrp
> > +
> > +ovn_as az2
> > +for i in 1 2; do
> > +    lr=lr2$i
> > +    ovn-nbctl lr-add $lr
> > +
> > +    lrp=lrp-$lr-ts1
> > +    lsp=lsp-ts1-$lr
> > +    # Create LRP and connect to TS
> > +    ovn-nbctl lrp-add $lr $lrp aa:aa:aa:aa:a2:0$i 169.254.10.2$i/24
> > +    ovn-nbctl lsp-add ts1 $lsp \
> > +            -- lsp-set-addresses $lsp router \
> > +            -- lsp-set-type $lsp router \
> > +            -- lsp-set-options $lsp router-port=$lrp
> > +done
> > +
> > +
> > +# Create directly-connected routes
> > +ovn_as az1 ovn-nbctl lrp-add lr11 lrp-lr11 aa:aa:aa:aa:bb:01 "
> > 192.168.0.1/24"
> > +ovn_as az2 ovn-nbctl lrp-add lr21 lrp-lr21 aa:aa:aa:aa:bc:01 "
> > 192.168.1.1/24"
> > +ovn_as az2 ovn-nbctl lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bc:02 "
> > 192.168.2.1/24"
> > +
> > +# Test direct routes from lr21 and lr22 were learned to lr11
> > +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 |
> > +             grep learned | awk '{print $1, $2}' | sort ], [0], [dnl
> > +192.168.1.0/24 169.254.10.21
> > +192.168.2.0/24 169.254.10.22
> > +])
> > +
> > +# Test direct routes from lr11 and lr22 were learned to lr21
> > +AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr21 | grep 192.168 |
> > +             grep learned | awk '{print $1, $2}' | sort ], [0], [dnl
> > +192.168.0.0/24 169.254.10.11
> > +])
> > +
> > +# Test direct routes from lr11 and lr21 were learned to lr22
> > +AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr22 | grep 192.168 |
> > +             grep learned | awk '{print $1, $2}' | sort ], [0], [dnl
> > +192.168.0.0/24 169.254.10.11
> > +])
> > +
> > +OVN_CLEANUP_IC([az1], [az2])
> > +
> > +AT_CLEANUP
> > +])
> > \ No newline at end of file
> >
> >
> Missing newline at the end of the file.
>
>
> > --
> > 2.42.0
> >
> > Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für
> > die Verwertung durch den vorgesehenen Empfänger bestimmt.
> > Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender
> > bitte unverzüglich in Kenntnis und löschen diese E Mail.
> >
> > Hinweise zum Datenschutz finden Sie hier<https://www.datenschutz.schwarz>.
> >
> >
> > This e-mail may contain confidential content and is intended only for the
> > specified recipient/s.
> > If you are not the intended recipient, please inform the sender
> > immediately and delete this e-mail.
> >
> > Information on data protection can be found here<
> > https://www.datenschutz.schwarz>.
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
>
> Thanks,
> Ales
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amusil@redhat.com    IM: amusil
> <https://red.ht/sig>
Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für die Verwertung durch den vorgesehenen Empfänger bestimmt.
Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender bitte unverzüglich in Kenntnis und löschen diese E Mail.

Hinweise zum Datenschutz finden Sie hier<https://www.datenschutz.schwarz>.


This e-mail may contain confidential content and is intended only for the specified recipient/s.
If you are not the intended recipient, please inform the sender immediately and delete this e-mail.

Information on data protection can be found here<https://www.datenschutz.schwarz>.