[ovs-dev] [PATCH ovn v8 1/5] ic: maintain route origin - connected/static

Vladislav Odintsov odivlad at gmail.com
Wed Nov 17 10:55:49 UTC 2021


Yeap, I’ll add documentation on this option, thanks.

I don’t see any scenario for user to modify this field.
The only one that I could imagine is an override for already existed static route with a user-defined static made as "connected" one.
I don’t think it has a real world use case, but maybe I’m wrong here.

Regards,
Vladislav Odintsov

> On 17 Nov 2021, at 05:13, Numan Siddique <numans at ovn.org> wrote:
> 
> On Sat, Nov 13, 2021 at 4:44 AM Vladislav Odintsov <odivlad at gmail.com <mailto:odivlad at gmail.com>> 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 <odivlad at gmail.com <mailto:odivlad at gmail.com>>
> 
> 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 <numans at ovn.org <mailto:numans at ovn.org>>
> 
> 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
>> dev at openvswitch.org <mailto:dev at openvswitch.org>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org <mailto:dev at openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>


More information about the dev mailing list