> > Hi Lorenzo, see below for some findings. Hi Mark,
thx for the fast review. > > On 12/22/20 3:54 PM, Lorenzo Bianconi wrote: > > Introduce the capability to transmit BFD packets in ovn-controller. > > Introduce BFD tables in nb/sb dbs in order to configure BFD parameters > > (e.g. min_tx, min_rx, ..) for ovn-controller. > > > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > --- > > controller/ovn-controller.c | 1 + > > controller/pinctrl.c | 297 +++++++++++++++++++++++++++++++++++- > > controller/pinctrl.h | 2 + > > lib/ovn-l7.h | 19 +++ > > northd/ovn-northd.c | 188 +++++++++++++++++++++++ > > ovn-nb.ovsschema | 24 ++- > > ovn-nb.xml | 66 ++++++++ > > ovn-sb.ovsschema | 27 +++- > > ovn-sb.xml | 78 ++++++++++ > > 9 files changed, 697 insertions(+), 5 deletions(-) > > [...] > > +} > > + > > +static void > > +bfd_table_check_default(const struct nbrec_bfd *nb_bt) > > All of these defaults should be mentioned in ovn-nb.xml > It's also probably worth creating constants for the default values. > ack, will fix it in v6 > > +{ > > + if (!nb_bt->status) { > > + /* default state is admin_down */ > > + nbrec_bfd_set_status(nb_bt, "admin_down"); > > + } > > + > > + if (!nb_bt->min_tx) { > > + /* default value for min_tx is 1s */ > > + nbrec_bfd_set_min_tx(nb_bt, 1000); > > + } > > + > > + if (!nb_bt->min_rx) { > > + /* default value for min_rx is 1s */ > > + nbrec_bfd_set_min_rx(nb_bt, 1000); > > + } > > ovn-nb.xml says "If this value is zero, the transmitting system does not > want the remote system to send any periodic BFD Control packets." This > seems to indicate that 0 is an allowed value. But then the code here > sets min_rx to 1000 ms if you set it to 0. right, will fix it in v6 > > > + > > + if (!nb_bt->detect_mult) { > > + /* default value for detect_mult is 5 */ > > + nbrec_bfd_set_detect_mult(nb_bt, 5); > > + } > > +} > > + > > +static void > > +build_bfd_update_sb_conf(const struct nbrec_bfd *nb_bt, > > + const struct sbrec_bfd *sb_bt) > > +{ > > + if (strcmp(nb_bt->dst_ip, sb_bt->dst_ip)) { > > + sbrec_bfd_set_dst_ip(sb_bt, nb_bt->dst_ip); > > + } > > + > > + if (strcmp(nb_bt->logical_port, sb_bt->logical_port)) { > > + sbrec_bfd_set_logical_port(sb_bt, nb_bt->logical_port); > > + } > > + > > + if (strcmp(nb_bt->status, sb_bt->status)) { > > + sbrec_bfd_set_status(sb_bt, nb_bt->status); > > + } > > + > > + if (nb_bt->detect_mult != sb_bt->detect_mult) { > > + sbrec_bfd_set_detect_mult(sb_bt, nb_bt->detect_mult); > > + } > > + > > + if (nb_bt->min_tx != sb_bt->min_tx) { > > + sbrec_bfd_set_min_tx(sb_bt, nb_bt->min_tx); > > + } > > + > > + if (nb_bt->min_rx != sb_bt->min_rx) { > > + sbrec_bfd_set_min_rx(sb_bt, nb_bt->min_rx); > > + } > > +} > > + > > +static void > > +build_bfd_table(struct northd_context *ctx, struct hmap *bfd_connections) > > +{ > > + struct hmap sb_only = HMAP_INITIALIZER(&sb_only); > > + struct bfd_entry *bfd_e, *next_bfd_e; > > + const struct sbrec_bfd *sb_bt; > > + uint32_t hash; > > + > > + SBREC_BFD_FOR_EACH (sb_bt, ctx->ovnsb_idl) { > > + bfd_e = xmalloc(sizeof *bfd_e); > > + bfd_e->sb_bt = sb_bt; > > + bfd_e->found = false; > > + hash = hash_string(sb_bt->dst_ip, 0); > > + hash = hash_string(sb_bt->logical_port, hash); > > + hmap_insert(&sb_only, &bfd_e->hmap_node, hash); > > + } > > + > > + const struct nbrec_bfd *nb_bt; > > + NBREC_BFD_FOR_EACH (nb_bt, ctx->ovnnb_idl) { > > + bfd_table_check_default(nb_bt); > > + > > + bfd_e = bfd_port_lookup(&sb_only, nb_bt->logical_port, > > nb_bt->dst_ip); > > + if (!bfd_e) { > > + sb_bt = sbrec_bfd_insert(ctx->ovnsb_txn); > > + sbrec_bfd_set_logical_port(sb_bt, nb_bt->logical_port); > > + sbrec_bfd_set_dst_ip(sb_bt, nb_bt->dst_ip); > > + sbrec_bfd_set_disc(sb_bt, 1 + random_uint32()); > > + /* RFC 5881 section 4 > > + * The source port MUST be in the range 49152 through 65535. > > + * The same UDP source port number MUST be used for all BFD > > + * Control packets associated with a particular session. > > + * The source port number SHOULD be unique among all BFD > > + * sessions on the system > > + */ > > The RFC says that the source UDP port should be unique among all BFD > sessions on the system. With the code below, it's possible to select the > same UDP source port for multiple BFD sessions. The likelihood grows > depending on the number of BFD sessions. This wouldn't be a huge deal > except that pinctrl uses the UDP source port and destination IP as a way > to uniquely identify a bfd_entry. > > 1) Start assignments with port 49152 and then increase by 1 with each > new BFD entry that is created. > 2) Create a bitmap that holds all possible port assignments. Set the > bits to 1 for ports in use and 0 for ports not in use. Find the first 0 > in the bitmap and then add 49152 to its index. This requires more memory > than (1) (1000x more memory, in fact) but it also allows for you to > reuse ports once they are no longer in use. > > With both of these approaches, you lose the randomness of the port > assignment, but you guarantee uniqueness. I thought about it, but I was hopping in random_range() :) I will fix it in v6. > > > + uint16_t udp_src = random_range(16383) + 49152; > > + sbrec_bfd_set_src_port(sb_bt, udp_src); > > + sbrec_bfd_set_status(sb_bt, nb_bt->status); > > + sbrec_bfd_set_min_tx(sb_bt, nb_bt->min_tx); > > + sbrec_bfd_set_min_rx(sb_bt, nb_bt->min_rx); > > + sbrec_bfd_set_detect_mult(sb_bt, nb_bt->detect_mult); > > + } else if (strcmp(bfd_e->sb_bt->status, nb_bt->status)) { > > + if (!strcmp(nb_bt->status, "admin_down") || > > + !strcmp(bfd_e->sb_bt->status, "admin_down")) { > > + sbrec_bfd_set_status(bfd_e->sb_bt, nb_bt->status); > > + } else { > > + nbrec_bfd_set_status(nb_bt, bfd_e->sb_bt->status); > > + } > > + } else { > > + build_bfd_update_sb_conf(nb_bt, bfd_e->sb_bt); > > Why is the SB BFD config only updated when the NB and SB BFD records > have the same state? ack, will fix it in v6 > > > + } > > + if (bfd_e) { > > + struct bfd_entry *bfd_conn = xmemdup(bfd_e, sizeof *bfd_e); > > + hash = hash_string(bfd_e->sb_bt->dst_ip, 0); > > + hash = hash_string(bfd_e->sb_bt->logical_port, hash); > > + bfd_conn->ref = false; > > + hmap_insert(bfd_connections, &bfd_conn->hmap_node, hash); > > + > > + bfd_e->found = true; > > I think the "found" field should be removed from the bfd_entry struct. > It doesn't have any intrinsic value for the bfd_entry. Instead, its only > relevance is with regards to the logic in this function. > > A good alternative would be to remove bfd_e from the sb_only hmap right > here. By doing so, all bfd_entries left in sb_only need to have their SB > records removed. > > > + } > > + } > > + > > + HMAP_FOR_EACH_SAFE (bfd_e, next_bfd_e, hmap_node, &sb_only) { > > + if (!bfd_e->found && bfd_e->sb_bt) { > > + sbrec_bfd_delete(bfd_e->sb_bt); > > + } > > + hmap_remove(&sb_only, &bfd_e->hmap_node); > > + free(bfd_e); > > + } > > If you take my above advice, this loop can be simplified to: > > HMAP_FOR_EACH_POP(bfd_e, hmap_node, &sb_only) { > if (bfd_e->sb_bt) { > sbrec_bfd_delete(bfd_e->sb_bt); > } > free(bfd_e); > } > nice, I will fix it in v6. Regards, Lorenzo > > + hmap_destroy(&sb_only); > > +} > > > > /* Returns a string of the IP address of the router port 'op' that > > * overlaps with 'ip_s". If one is not found, returns NULL. > > @@ -12410,6 +12582,7 @@ ovnnb_db_run(struct northd_context *ctx, > > struct hmap igmp_groups; > > struct shash meter_groups = SHASH_INITIALIZER(&meter_groups); > > struct hmap lbs; > > + struct hmap bfd_connections = HMAP_INITIALIZER(&bfd_connections); > > > > /* Sync ipsec configuration. > > * Copy nb_cfg from northbound to southbound database. > > @@ -12504,6 +12677,7 @@ ovnnb_db_run(struct northd_context *ctx, > > build_ip_mcast(ctx, datapaths); > > build_mcast_groups(ctx, datapaths, ports, &mcast_groups, > > &igmp_groups); > > build_meter_groups(ctx, &meter_groups); > > + build_bfd_table(ctx, &bfd_connections); > > build_lflows(ctx, datapaths, ports, &port_groups, &mcast_groups, > > &igmp_groups, &meter_groups, &lbs); > > ovn_update_ipv6_prefix(ports); > > @@ -12529,9 +12703,13 @@ ovnnb_db_run(struct northd_context *ctx, > > HMAP_FOR_EACH_SAFE (pg, next_pg, key_node, &port_groups) { > > ovn_port_group_destroy(&port_groups, pg); > > } > > + > > + bfd_cleanup_connections(ctx, &bfd_connections); > > + > > hmap_destroy(&igmp_groups); > > hmap_destroy(&mcast_groups); > > hmap_destroy(&port_groups); > > + hmap_destroy(&bfd_connections); > > > > struct shash_node *node, *next; > > SHASH_FOR_EACH_SAFE (node, next, &meter_groups) { > > @@ -13465,6 +13643,16 @@ main(int argc, char *argv[]) > > add_column_noalert(ovnsb_idl_loop.idl, > > &sbrec_load_balancer_col_external_ids); > > > > + ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_bfd); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_logical_port); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_dst_ip); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_status); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_min_tx); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_min_rx); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_detect_mult); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_disc); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_src_port); > > + > > struct ovsdb_idl_index *sbrec_chassis_by_name > > = chassis_index_create(ovnsb_idl_loop.idl); > > > > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > > index b77a2308c..a880482a4 100644 > > --- a/ovn-nb.ovsschema > > +++ b/ovn-nb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Northbound", > > - "version": "5.30.0", > > - "cksum": "3273824429 27172", > > + "version": "5.31.0", > > + "cksum": "3663844734 28178", > > "tables": { > > "NB_Global": { > > "columns": { > > @@ -526,5 +526,25 @@ > > "type": {"key": "string", "value": "string", > > "min": 0, "max": "unlimited"}}}, > > "indexes": [["name"]], > > + "isRoot": true}, > > + "BFD": { > > + "columns": { > > + "logical_port": {"type": "string"}, > > + "dst_ip": {"type": "string"}, > > + "min_tx": {"type": {"key": {"type": "integer"}}}, > > + "min_rx": {"type": {"key": {"type": "integer"}}}, > > + "detect_mult": {"type": {"key": {"type": "integer"}}}, > > + "status": { > > + "type": {"key": {"type": "string", > > + "enum": ["set", ["down", "init", "up", > > + "admin_down"]]}, > > + "min": 0, "max": 1}}, > > + "external_ids": { > > + "type": {"key": "string", "value": "string", > > + "min": 0, "max": "unlimited"}}, > > + "options": { > > + "type": {"key": "string", "value": "string", > > + "min": 0, "max": "unlimited"}}}, > > + "indexes": [["logical_port", "dst_ip"]], > > "isRoot": true}} > > } > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index ec6405ff5..8ba1c3c82 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -3741,4 +3741,70 @@ > > </column> > > </group> > > </table> > > + > > + <table name="BFD"> > > + <p> > > + Contains BFD parameter for ovn-controller bfd configuration > > + </p> > > + > > + <group title="Configuration"> > > + <column name="logical_port"> > > + OVN logical port when BFD engine is running. > > + </column> > > + > > + <column name="dst_ip"> > > + BFD peer IP address. > > + </column> > > + > > + <column name="min_tx"> > > + This is the minimum interval, in milliseconds, that the local > > + system would like to use when transmitting BFD Control packets, > > + less any jitter applied. The value zero is reserved. > > + </column> > > + > > + <column name="min_rx"> > > + This is the minimum interval, in milliseconds, between received > > + BFD Control packets that this system is capable of supporting, > > + less any jitter applied by the sender. If this value is zero, > > + the transmitting system does not want the remote system to send > > + any periodic BFD Control packets. > > + </column> > > + > > + <column name="detect_mult"> > > + Detection time multiplier. The negotiated transmit interval, > > + multiplied by this value, provides the Detection Time for the > > + receiving system in Asynchronous mode. > > + </column> > > + > > + <column name="options"> > > + Reserved for future use. > > + </column> > > + > > + <column name="external_ids"> > > + See <em>External IDs</em> at the beginning of this document. > > + </column> > > + </group> > > + > > + <group title="Status Reporting"> > > + <column name="status"> > > + <p> > > + BFD port logical states. Possible values are: > > + <ul> > > + <li> > > + <code>admin_down</code> > > + </li> > > + <li> > > + <code>down</code> > > + </li> > > + <li> > > + <code>init</code> > > + </li> > > + <li> > > + <code>up</code> > > + </li> > > + </ul> > > + </p> > > + </column> > > + </group> > > + </table> > > </database> > > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > > index 5228839b8..97db6de39 100644 > > --- a/ovn-sb.ovsschema > > +++ b/ovn-sb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Southbound", > > - "version": "20.12.0", > > - "cksum": "3969471120 24441", > > + "version": "20.13.0", > > + "cksum": "3035725595 25676", > > "tables": { > > "SB_Global": { > > "columns": { > > @@ -484,6 +484,29 @@ > > "external_ids": { > > "type": {"key": "string", "value": "string", > > "min": 0, "max": "unlimited"}}}, > > + "isRoot": true}, > > + "BFD": { > > + "columns": { > > + "src_port": {"type": {"key": {"type": "integer", > > + "minInteger": 49152, > > + "maxInteger": 65535}}}, > > + "disc": {"type": {"key": {"type": "integer"}}}, > > + "logical_port": {"type": "string"}, > > + "dst_ip": {"type": "string"}, > > + "min_tx": {"type": {"key": {"type": "integer"}}}, > > + "min_rx": {"type": {"key": {"type": "integer"}}}, > > + "detect_mult": {"type": {"key": {"type": "integer"}}}, > > + "status": { > > + "type": {"key": {"type": "string", > > + "enum": ["set", ["down", "init", "up", > > + "admin_down"]]}}}, > > + "external_ids": { > > + "type": {"key": "string", "value": "string", > > + "min": 0, "max": "unlimited"}}, > > + "options": { > > + "type": {"key": "string", "value": "string", > > + "min": 0, "max": "unlimited"}}}, > > + "indexes": [["logical_port", "dst_ip", "src_port", "disc"]], > > "isRoot": true} > > } > > } > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > index c13994848..ccfe2e415 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -4231,4 +4231,82 @@ tcp.flags = RST; > > </column> > > </group> > > </table> > > + > > + <table name="BFD"> > > + <p> > > + Contains BFD parameter for ovn-controller bfd configuration > > + </p> > > + > > + <group title="Configuration"> > > + <column name="src_port"> > > + udp source port used in bfd control packets. > > + The source port MUST be in the range 49152 through 65535 > > + (RFC5881 section 4). > > + </column> > > + > > + <column name="disc"> > > + A unique, nonzero discriminator value generated by the transmitting > > + system, used to demultiplex multiple BFD sessions between the same > > pair > > + of systems. > > + </column> > > + > > + <column name="logical_port"> > > + OVN logical port when BFD engine is running. > > + </column> > > + > > + <column name="dst_ip"> > > + BFD peer IP address. > > + </column> > > + > > + <column name="min_tx"> > > + This is the minimum interval, in milliseconds, that the local > > + system would like to use when transmitting BFD Control packets, > > + less any jitter applied. The value zero is reserved. > > + </column> > > + > > + <column name="min_rx"> > > + This is the minimum interval, in milliseconds, between received > > + BFD Control packets that this system is capable of supporting, > > + less any jitter applied by the sender. If this value is zero, > > + the transmitting system does not want the remote system to send > > + any periodic BFD Control packets. > > + </column> > > + > > + <column name="detect_mult"> > > + Detection time multiplier. The negotiated transmit interval, > > + multiplied by this value, provides the Detection Time for the > > + receiving system in Asynchronous mode. > > + </column> > > + > > + <column name="options"> > > + Reserved for future use. > > + </column> > > + > > + <column name="external_ids"> > > + See <em>External IDs</em> at the beginning of this document. > > + </column> > > + </group> > > + > > + <group title="Status Reporting"> > > + <column name="status"> > > + <p> > > + BFD port logical states. Possible values are: > > + <ul> > > + <li> > > + <code>admin_down</code> > > + </li> > > + <li> > > + <code>down</code> > > + </li> > > + <li> > > + <code>init</code> > > + </li> > > + <li> > > + <code>up</code> > > + </li> > > + </ul> > > + </p> > > + </column> > > + </group> > > + </table> > > </database> > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
