On Sat, Nov 13, 2021 at 4:44 AM Vladislav Odintsov <[email protected]> wrote: > > This commits adds ability to save route's origin while IC learning. > Directly connected routes are saved in IC SB DB with "connected" > origin column value. > Static routes have "static" value in origin column. > > This logic would be used in next patch to compute priority > for lr_in_ip_routing stage lflows. > > Signed-off-by: Vladislav Odintsov <[email protected]>
Hi Vladislav, The patch LGTM. I think you missed documenting the option "origin" for the NB Logical_Router_Static_Route option. Was this intentional? From what I understand, ovn-ic will set this option. So for a setup where ovn-ic is not used, can a user/CMS configure this option ? I think it would be good to document this in ovn-nb.xml. With the documentation added: Acked-by: Numan Siddique <[email protected]> Numan > --- > ic/ovn-ic.c | 34 +++++++++++++++++++-------- > lib/ovn-util.h | 3 +++ > ovn-ic-sb.ovsschema | 7 ++++-- > ovn-ic-sb.xml | 10 ++++++++ > tests/ovn-ic.at | 57 +++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 99 insertions(+), 12 deletions(-) > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index 303e93a4f..70abae108 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -854,6 +854,7 @@ struct ic_route_info { > struct in6_addr prefix; > unsigned int plen; > struct in6_addr nexthop; > + const char *origin; > > /* Either nb_route or nb_lrp is set and the other one must be NULL. > * - For a route that is learned from IC-SB, or a static route that is > @@ -867,22 +868,25 @@ struct ic_route_info { > > static uint32_t > ic_route_hash(const struct in6_addr *prefix, unsigned int plen, > - const struct in6_addr *nexthop) > + const struct in6_addr *nexthop, const char *origin) > { > uint32_t basis = hash_bytes(prefix, sizeof *prefix, (uint32_t)plen); > + basis = hash_string(origin, basis); > return hash_bytes(nexthop, sizeof *nexthop, basis); > } > > static struct ic_route_info * > ic_route_find(struct hmap *routes, const struct in6_addr *prefix, > - unsigned int plen, const struct in6_addr *nexthop) > + unsigned int plen, const struct in6_addr *nexthop, > + const char *origin) > { > struct ic_route_info *r; > - uint32_t hash = ic_route_hash(prefix, plen, nexthop); > + uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin); > HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) { > if (ipv6_addr_equals(&r->prefix, prefix) && > r->plen == plen && > - ipv6_addr_equals(&r->nexthop, nexthop)) { > + ipv6_addr_equals(&r->nexthop, nexthop) && > + !strcmp(r->origin, origin)) { > return r; > } > } > @@ -926,13 +930,15 @@ add_to_routes_learned(struct hmap *routes_learned, > &prefix, &plen, &nexthop)) { > return false; > } > + const char *origin = smap_get_def(&nb_route->options, "origin", ""); > 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; > hmap_insert(routes_learned, &ic_route->node, > - ic_route_hash(&prefix, plen, &nexthop)); > + ic_route_hash(&prefix, plen, &nexthop, origin)); > return true; > } > > @@ -1093,8 +1099,9 @@ add_to_routes_ad(struct hmap *routes_ad, > ic_route->plen = plen; > ic_route->nexthop = nexthop; > ic_route->nb_route = nb_route; > + ic_route->origin = ROUTE_ORIGIN_STATIC; > hmap_insert(routes_ad, &ic_route->node, > - ic_route_hash(&prefix, plen, &nexthop)); > + ic_route_hash(&prefix, plen, &nexthop, ROUTE_ORIGIN_STATIC)); > } > > static void > @@ -1143,8 +1150,10 @@ add_network_to_routes_ad(struct hmap *routes_ad, const > char *network, > ic_route->plen = plen; > ic_route->nexthop = nexthop; > ic_route->nb_lrp = nb_lrp; > + ic_route->origin = ROUTE_ORIGIN_CONNECTED; > hmap_insert(routes_ad, &ic_route->node, > - ic_route_hash(&prefix, plen, &nexthop)); > + ic_route_hash(&prefix, plen, &nexthop, > + ROUTE_ORIGIN_CONNECTED)); > } > > static bool > @@ -1206,7 +1215,8 @@ sync_learned_route(struct ic_context *ctx, > continue; > } > struct ic_route_info *route_learned > - = ic_route_find(&ic_lr->routes_learned, &prefix, plen, &nexthop); > + = ic_route_find(&ic_lr->routes_learned, &prefix, plen, &nexthop, > + isb_route->origin); > if (route_learned) { > /* Sync external-ids */ > struct uuid ext_id; > @@ -1233,6 +1243,8 @@ sync_learned_route(struct ic_context *ctx, > UUID_ARGS(&isb_route->header_.uuid)); > nbrec_logical_router_static_route_update_external_ids_setkey( > nb_route, "ic-learned-route", uuid_s); > + nbrec_logical_router_static_route_update_options_setkey( > + nb_route, "origin", isb_route->origin); > free(uuid_s); > nbrec_logical_router_update_static_routes_addvalue( > ic_lr->lr, nb_route); > @@ -1297,8 +1309,9 @@ advertise_route(struct ic_context *ctx, > icsbrec_route_delete(isb_route); > continue; > } > - struct ic_route_info *route_adv = > - ic_route_find(routes_ad, &prefix, plen, &nexthop); > + struct ic_route_info *route_adv = ic_route_find(routes_ad, &prefix, > + plen, &nexthop, > + isb_route->origin); > if (!route_adv) { > /* Delete the extra route from IC-SB. */ > VLOG_DBG("Delete route %s -> %s from IC-SB, which is not found" > @@ -1338,6 +1351,7 @@ advertise_route(struct ic_context *ctx, > } > icsbrec_route_set_ip_prefix(isb_route, prefix_s); > icsbrec_route_set_nexthop(isb_route, nexthop_s); > + icsbrec_route_set_origin(isb_route, route_adv->origin); > free(prefix_s); > free(nexthop_s); > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index 2fa92e069..a923c3b65 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -25,6 +25,9 @@ > #define ovn_print_version(MIN_OFP, MAX_OFP) \ > ovs_print_version(MIN_OFP, MAX_OFP) > > +#define ROUTE_ORIGIN_CONNECTED "connected" > +#define ROUTE_ORIGIN_STATIC "static" > + > struct nbrec_logical_router_port; > struct sbrec_logical_flow; > struct svec; > diff --git a/ovn-ic-sb.ovsschema b/ovn-ic-sb.ovsschema > index 5364b21b4..42ce85d7d 100644 > --- a/ovn-ic-sb.ovsschema > +++ b/ovn-ic-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_IC_Southbound", > - "version": "1.0.0", > - "cksum": "108951192 6585", > + "version": "1.1.0", > + "cksum": "423535838 6733", > "tables": { > "IC_SB_Global": { > "columns": { > @@ -94,6 +94,9 @@ > "refTable": "Availability_Zone"}}}, > "ip_prefix": {"type": "string"}, > "nexthop": {"type": "string"}, > + "origin": {"type": {"key": { > + "type": "string", > + "enum": ["set", ["connected", "static"]]}}}, > "external_ids": { > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}, > diff --git a/ovn-ic-sb.xml b/ovn-ic-sb.xml > index 3582cff47..d8338e4d3 100644 > --- a/ovn-ic-sb.xml > +++ b/ovn-ic-sb.xml > @@ -313,6 +313,16 @@ > <column name="nexthop"> > Nexthop IP address for this route. > </column> > + > + <column name="origin"> > + Can be one of <code>connected</code> or <code>static</code>. Routes > to > + directly-connected subnets - LRP's CIDRs are inserted to OVN IC SB DB > + with <code>connected</code> value in <ref column="origin"/>. Static > + routes are inserted to OVN IC SB DB with <code>static</code> value. > + Next when route is learned to another AZ NB DB by ovn-ic, route > origin > + is synced to <ref table="Logical_Router_Static_Route" > column="options" > + key="origin"/>. > + </column> > </group> > > <group title="Common Columns"> > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > index 9086974a3..7e8498b2f 100644 > --- a/tests/ovn-ic.at > +++ b/tests/ovn-ic.at > @@ -423,6 +423,63 @@ AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | > # Test routes from lr12 didn't leak as learned to lr21 > AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr21], [0], []) > > +# cleanup > +ovn-ic-nbctl --if-exists ts-del ts1 > +ovn_as az1 ovn-nbctl lr-del lr11 > +ovn_as az1 ovn-nbctl lr-del lr21 > +ovn_as az2 ovn-nbctl lr-del lr12 > +ovn_as az2 ovn-nbctl lr-del lr22 > + > +# check routes origin advertisement and learning > + > +# setup topology with connected, static and source routes > +ovn-ic-nbctl ts-add ts1 > +for i in 1 2; do > + 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 > + > + # Create LRP and connect to TS > + ovn-nbctl lr-add lr$i > + ovn-nbctl lrp-add lr$i lrp-lr$i-ts1 aa:aa:aa:aa:aa:0$i 169.254.100.$i/24 > + ovn-nbctl lsp-add ts1 lsp-ts1-lr$i \ > + -- lsp-set-addresses lsp-ts1-lr$i router \ > + -- lsp-set-type lsp-ts1-lr$i router \ > + -- lsp-set-options lsp-ts1-lr$i router-port=lrp-lr$i-ts1 > + > + ovn-nbctl lrp-add lr$i lrp-lr$i-p$i 00:00:00:00:00:0$i 192.168.$i.1/24 > + > + # Create static routes > + ovn-nbctl lr-route-add lr$i 10.11.$i.0/24 169.254.0.1 > + > + # Create a src-ip route, which shouldn't be synced > + ovn-nbctl --policy=src-ip lr-route-add lr$i 10.22.$i.0/24 169.254.0.2 > +done > + > +for i in 1 2; do > + OVS_WAIT_UNTIL([ovn_as az$i ovn-nbctl lr-route-list lr$i | grep learned]) > +done > + > +# check that advertised routes in ic-sb have correct origin > +ovn-ic-sbctl list route > +wait_row_count ic-sb:Route 1 ip_prefix=10.11.1.0/24 origin=static > +wait_row_count ic-sb:Route 1 ip_prefix=192.168.1.1/24 origin=connected > +wait_row_count ic-sb:Route 1 ip_prefix=10.11.2.0/24 origin=static > +wait_row_count ic-sb:Route 1 ip_prefix=192.168.2.1/24 origin=connected > + > +# check that learned routes in ic-sb have correct origin > + > +ovn_as az1 > +wait_row_count nb:Logical_Router_Static_Route 1 ip_prefix=10.11.2.0/24 > options:origin=static > +wait_row_count nb:Logical_Router_Static_Route 1 ip_prefix=192.168.2.1/24 > options:origin=connected > + > +ovn_as az2 > +wait_row_count nb:Logical_Router_Static_Route 1 ip_prefix=10.11.1.0/24 > options:origin=static > +wait_row_count nb:Logical_Router_Static_Route 1 ip_prefix=192.168.1.1/24 > options:origin=connected > + > OVN_CLEANUP_IC([az1], [az2]) > > AT_CLEANUP > -- > 2.30.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
