> On Fri, Dec 11, 2020 at 5:56 PM Lorenzo Bianconi
> <[email protected]> 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]>
>
> Hi Lorenzo,
>
> Thanks for v2. Please see below for a few comments.
Hi Numan,
thx for the review.
>
> Numan
>
> > ---
[...]
> > struct northd_state {
> > @@ -11547,6 +11548,105 @@ build_lflows(struct northd_context *ctx, struct
> > hmap *datapaths,
> > }
> > }
> >
> > +static struct ovsdb_idl_index *
> > +bfd_index_create(struct ovsdb_idl *idl)
> > +{
> > + return ovsdb_idl_index_create2(idl, &sbrec_bfd_col_logical_port,
> > + &sbrec_bfd_col_dst_ip);
> > +}
> > +
>
> I really don't see a reason to create an idl_index for the build/sync
> of BFD from NB DB to SB DB.
ack, I will fix it in v3.
>
>
> > +static const struct sbrec_bfd *
> > +bfd_port_lookup(struct ovsdb_idl_index *bfd_index,
> > + const char *logical_port, const char *dst_ip)
> > +{
> > + struct sbrec_bfd *target = sbrec_bfd_index_init_row(bfd_index);
> > + sbrec_bfd_index_set_logical_port(target, logical_port);
> > + sbrec_bfd_index_set_dst_ip(target, dst_ip);
> > +
> > + struct sbrec_bfd *bfd_entry = sbrec_bfd_index_find(bfd_index, target);
> > + sbrec_bfd_index_destroy_row(target);
> > +
> > + return bfd_entry;
> > +}
> > +
>
> I think it is better if the above function looks up the BFD structure from
> a local hashmap.
ack, I will fix it in v3.
>
>
> > +struct bfd_entry {
> > + struct hmap_node hmap_node;
> > +
> > + char *logical_port;
> > + char *dst_ip;
> > +
>
> I think there is no need to have 'logical_port' and 'dst_ip' in this structure
> since the same can be accessed from 'sb_bt'.
ack, I will fix it in v3.
>
>
> > + const struct sbrec_bfd *sb_bt;
> > + bool found;
> > +};
> > +
> > +static void
> > +build_bfd_table(struct northd_context *ctx)
> > +{
> > + struct hmap sb_only = HMAP_INITIALIZER(&sb_only);
> > + struct bfd_entry *bfd_e, *next_bfd_e;
> > + const struct sbrec_bfd *sb_bt;
> > +
> > + SBREC_BFD_FOR_EACH (sb_bt, ctx->ovnsb_idl) {
> > + bfd_e = xmalloc(sizeof *bfd_e);
> > + bfd_e->logical_port = xstrdup(sb_bt->logical_port);
> > + bfd_e->dst_ip = xstrdup(sb_bt->dst_ip);
>
> > + bfd_e->sb_bt = sb_bt;
> > + hmap_insert(&sb_only, &bfd_e->hmap_node,
> > + hash_string(bfd_e->dst_ip, 0));
>
> I think it's better to hash with both logical_port and dst_ip.
> Also set found to false here.
ack, I will fix it in v3.
>
>
>
> > + }
> > +
>
> > + const struct nbrec_bfd *nb_bt;
> > + NBREC_BFD_FOR_EACH (nb_bt, ctx->ovnnb_idl) {
> > + sb_bt = bfd_port_lookup(ctx->sbrec_bfd_by_logical_port,
> > + nb_bt->logical_port, nb_bt->dst_ip);
>
> Instead of doing a lookup in the SB IDL, I think it's better to look up
> from the 'sb_only' hmap just built above.
>
> If the lookup is successful, you could set found = true.
ack, I will fix it in v3.
>
>
> > + if (!sb_bt) {
> > + 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
> > + */
> > + uint16_t udp_src = random_range(16383) + 49152;
> > + sbrec_bfd_set_src_port(sb_bt, udp_src);
> > + sbrec_bfd_set_status(sb_bt, "down");
> > + 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(sb_bt->status, nb_bt->status)) {
> > + if (!strcmp(nb_bt->status, "admin_down") ||
> > + !strcmp(sb_bt->status, "admin_down")) {
> > + sbrec_bfd_set_status(sb_bt, nb_bt->status);
> > + } else {
> > + nbrec_bfd_set_status(nb_bt, sb_bt->status);
> > + }
> > + }
> > + HMAP_FOR_EACH_WITH_HASH (bfd_e, hmap_node,
> > + hash_string(nb_bt->dst_ip, 0), &sb_only) {
> > + if (!strcmp(bfd_e->logical_port, nb_bt->logical_port) &&
> > + !strcmp(bfd_e->dst_ip, nb_bt->dst_ip)) {
> > + bfd_e->found = true;
> > + break;
> > + }
> > + }
>
> I don't think you would need this hmap loop if you use 'sb_only' hmap
> for lookup.
right.
>
> > + }
> > +
> > + 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);
> > + }
> > + free(bfd_e->logical_port);
> > + free(bfd_e->dst_ip);
> > + hmap_remove(&sb_only, &bfd_e->hmap_node);
> > + free(bfd_e);
> > + }
> > + hmap_destroy(&sb_only);
> > +}
> > +
> > static void
> > sync_address_set(struct northd_context *ctx, const char *name,
> > const char **addrs, size_t n_addrs,
> > @@ -12344,6 +12444,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);
> > build_lflows(ctx, datapaths, ports, &port_groups, &mcast_groups,
> > &igmp_groups, &meter_groups, &lbs);
> > ovn_update_ipv6_prefix(ports);
> > @@ -13303,6 +13404,10 @@ main(int argc, char *argv[])
> > add_column_noalert(ovnsb_idl_loop.idl,
> > &sbrec_load_balancer_col_external_ids);
> >
> > + 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);
> > +
> > struct ovsdb_idl_index *sbrec_chassis_by_name
> > = chassis_index_create(ovnsb_idl_loop.idl);
> >
> > @@ -13315,6 +13420,9 @@ main(int argc, char *argv[])
> > struct ovsdb_idl_index *sbrec_ip_mcast_by_dp
> > = ip_mcast_index_create(ovnsb_idl_loop.idl);
> >
> > + struct ovsdb_idl_index *sbrec_bfd_by_logical_port
> > + = bfd_index_create(ovnsb_idl_loop.idl);
> > +
> > unixctl_command_register("sb-connection-status", "", 0, 0,
> > ovn_conn_show, ovnsb_idl_loop.idl);
> >
> > @@ -13347,6 +13455,7 @@ main(int argc, char *argv[])
> > .sbrec_ha_chassis_grp_by_name =
> > sbrec_ha_chassis_grp_by_name,
> > .sbrec_mcast_group_by_name_dp =
> > sbrec_mcast_group_by_name_dp,
> > .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
> > + .sbrec_bfd_by_logical_port = sbrec_bfd_by_logical_port,
> > };
> >
> > if (!state.had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl))
> > {
> > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> > index af77dd138..788a08447 100644
> > --- a/ovn-nb.ovsschema
> > +++ b/ovn-nb.ovsschema
> > @@ -1,7 +1,7 @@
> > {
> > "name": "OVN_Northbound",
> > - "version": "5.29.0",
> > - "cksum": "328602112 27064",
> > + "version": "5.30.0",
> > + "cksum": "2396072339 27861",
> > "tables": {
> > "NB_Global": {
> > "columns": {
> > @@ -524,5 +524,21 @@
> > "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"]]}}},
> > + "options": {
> > + "type": {"key": "string", "value": "string",
> > + "min": 0, "max": "unlimited"}}},
>
> I think it's better to add an external_ids column to both NB and SB dbs.
>
> With the above schema, the user when creating the BFD row has to
> specify the status.
> Since this column is updated by ovn-northd, I think users should be allowed to
> create a BFD row without setting any value.
>
> I'd suggest changing the status to
>
> "status": {
> "type": {"key": {"type": "string",
> "min": 0, "max": 1,
> "enum": ["set", ["down", "init", "up",
> "admin_down"]]}}},
ack, I will fix it in v3.
>
>
> Could you also please add tests in ovn-northd.at which creates/deletes
> BFD rows in NB DB and make sure that it is synced properly in SB DB.
>
> Thanks
It is already there :) I added it in patch "controller: bfd: introduce BFD
state machine"
Regards,
Lorenzo
>
>
> Numan
>
>
>
> > + "indexes": [["logical_port", "dst_ip"]],
> > "isRoot": true}}
> > }
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index e7a8d6833..b6cac3c22 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -3712,4 +3712,66 @@
> > </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>
> > + </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..5d78efe72 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": "1664079143 25516",
> > "tables": {
> > "SB_Global": {
> > "columns": {
> > @@ -484,6 +484,26 @@
> > "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"]]}}},
> > + "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..a5da5635f 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -4231,4 +4231,78 @@ 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>
> > + </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>
> > --
> > 2.29.2
> >
> > _______________________________________________
> > 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
