| Message ID | 20210921162415.3478-1-venugopali@nvidia.com |
|---|---|
| State | Superseded |
| Delegated to: | Han Zhou |
| Headers | show |
| Series | [ovs-dev] ovn-controller: Allow specifying tos option for tunnel interface | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
On Tue, Sep 21, 2021 at 9:24 AM <venugopali@nvidia.com> wrote: > > From: Venugopal Iyer <venugopali@nvidia.com> > > Currently, OVN tunnel interface supports the csum option along > with remote_ip and key. There are applications (e.g. RoCE) that rely > on setting the DSCP bits and expect it to be moved to the outer/ > tunnel header as well. > > This commit adds an "ovn-encap-tos" external-id that can be used to > set the tos option on the OVS tunnel interface, using: > > ovs-vsctl set Open_vSwitch . external_ids:ovn-encap-tos=inherit > > Tested by setting the external_id (as above) and checking the geneve > interfaces created, e.g: > > options : {csum="true", key=flow, remote_ip="X.X.X.X", tos=inherit} > > Also, added a simple test case to make sure the tos option is carried to the > tunnel interface when set. > > Signed-off-by: venu iyer (venugopali@nvidia.com) Thanks Venu for the patch. There are some checkpatch failures due to the email format and line length. The patch looks good overall. Please find some minor comments below regarding documentation and tests. > --- > controller/encaps.c | 25 ++++++++++++++---- > controller/encaps.h | 1 + > controller/ovn-controller.8.xml | 7 +++++ > controller/ovn-controller.c | 1 + > tests/ovn-controller.at | 45 +++++++++++++++++++++++++++++++++ > 5 files changed, 74 insertions(+), 5 deletions(-) > > diff --git a/controller/encaps.c b/controller/encaps.c > index fc93bf1ee..da24448f5 100644 > --- a/controller/encaps.c > +++ b/controller/encaps.c > @@ -152,7 +152,8 @@ encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id, > > static void > tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, > - const char *new_chassis_id, const struct sbrec_encap *encap) > + const char *new_chassis_id, const struct sbrec_encap *encap, > + const struct ovsrec_open_vswitch_table *ovs_table) > { > struct smap options = SMAP_INITIALIZER(&options); > smap_add(&options, "remote_ip", encap->ip); > @@ -202,6 +203,18 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, > smap_add(&options, "remote_name", new_chassis_id); > } > > + const struct ovsrec_open_vswitch *cfg = > + ovsrec_open_vswitch_table_first(ovs_table); > + /* If the tos option is configured, get it */ > + if (cfg) { > + const char *encap_tos = smap_get_def(&cfg->external_ids, > + "ovn-encap-tos", "none"); > + > + if (encap_tos && strcmp(encap_tos, "none")) { > + smap_add(&options, "tos", encap_tos); > + } > + } > + > /* If there's an existing chassis record that does not need any change, > * keep it. Otherwise, create a new record (if there was an existing > * record, the new record will supplant it and encaps_run() will delete > @@ -270,7 +283,8 @@ preferred_encap(const struct sbrec_chassis *chassis_rec) > * as there are VTEP of that type (differentiated by remote_ip) on that chassis. > */ > static int > -chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_sb_global *sbg, struct tunnel_ctx *tc) > +chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_sb_global *sbg, > + const struct ovsrec_open_vswitch_table *ovs_table, struct tunnel_ctx *tc) > { > struct sbrec_encap *encap = preferred_encap(chassis_rec); > int tuncnt = 0; > @@ -286,7 +300,7 @@ chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_s > if (tun_type != pref_type) { > continue; > } > - tunnel_add(tc, sbg, chassis_rec->name, chassis_rec->encaps[i]); > + tunnel_add(tc, sbg, chassis_rec->name, chassis_rec->encaps[i], ovs_table); > tuncnt++; > } > return tuncnt; > @@ -316,11 +330,12 @@ chassis_tzones_overlap(const struct sset *transport_zones, > > void > encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > - const struct ovsrec_bridge_table *bridge_table, > + const struct ovsrec_bridge_table *bridge_table, > const struct ovsrec_bridge *br_int, > const struct sbrec_chassis_table *chassis_table, > const struct sbrec_chassis *this_chassis, > const struct sbrec_sb_global *sbg, > + const struct ovsrec_open_vswitch_table *ovs_table, > const struct sset *transport_zones) > { > if (!ovs_idl_txn || !br_int) { > @@ -390,7 +405,7 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > continue; > } > > - if (chassis_tunnel_add(chassis_rec, sbg, &tc) == 0) { > + if (chassis_tunnel_add(chassis_rec, sbg, ovs_table, &tc) == 0) { > VLOG_INFO("Creating encap for '%s' failed", chassis_rec->name); > continue; > } > diff --git a/controller/encaps.h b/controller/encaps.h > index f488393c4..25d44b034 100644 > --- a/controller/encaps.h > +++ b/controller/encaps.h > @@ -35,6 +35,7 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > const struct sbrec_chassis_table *, > const struct sbrec_chassis *, > const struct sbrec_sb_global *, > + const struct ovsrec_open_vswitch_table *, > const struct sset *transport_zones); > > bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn, > diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml > index 8c180f576..b419cab05 100644 > --- a/controller/ovn-controller.8.xml > +++ b/controller/ovn-controller.8.xml > @@ -201,6 +201,13 @@ > performance loss. In such cases, encapsulation checksums can be disabled. > </dd> > > + <dt><code>external_ids:ovn-encap-tos</code></dt> > + <dd> > + <code>ovn-encap-tos</code> indicates whether the tos (DSCP) needs > + to be copied, from the inner to the outer/encap header, on tunnels > + created on this chassis. This description seems inaccurate, because this implementation not only supports "inherit" but any possible values that is supported by options:tos in the Interface table of Open_vSwitch. We'd better just say this value will be applied to the Open_vSwitch:options:tos, and "inherit" can be used as an example. > + </dd> > + > <dt><code>external_ids:ovn-cms-options</code></dt> > <dd> > A list of options that will be consumed by the CMS Plugin and which > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index aa7941eeb..a719beb0e 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -3593,6 +3593,7 @@ main(int argc, char *argv[]) > sbrec_chassis_table_get(ovnsb_idl_loop.idl), > chassis, > sbrec_sb_global_first(ovnsb_idl_loop.idl), > + ovs_table, > &transport_zones); > > stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME, > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 4ae218ed6..cd1617981 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -723,3 +723,48 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=0 > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +# Checks that ovn-controller honors the change to tunnel interface if we > +# set ovn-encap-tos option > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-controller - change Encap ToS option]) > +AT_KEYWORDS([ovn]) > +ovn_start > + > +net_add n1 > +sim_add hv > +as hv > +ovs-vsctl \ > + -- add-br br-phys \ > + -- add-br br-eth0 \ > + -- add-br br-eth1 \ > + -- add-br br-eth2 > +ovn_attach n1 br-phys 192.168.0.1 > + > +check_tunnel_property () { > + test "`ovs-vsctl get interface ovn-fakech-0 $1`" = "$2" > +} > + > +# Start off with a remote chassis supporting geneve > +ovn-sbctl chassis-add fakechassis geneve 192.168.0.2 > +OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > + > +as hv > +ovs-vsctl \ > + -- set Open_vSwitch . external-ids:ovn-encap-tos="inherit" > + > +# now, wait for a couple of secs > +sleep 2 > + > +tos_option=$(ovs-vsctl get interface ovn-fakech-0 options:tos) > +expected_tos_option="inherit" > +AT_CHECK([test "$tos_option" = "$expected_tos_option"], [0], []) I would be better to add steps to modify the value and then clear the value, make sure the changes are reflected properly. Thanks, Han > + > +# Gracefully terminate daemons > +OVN_CLEANUP_SBOX([hv]) > +OVN_CLEANUP_VSWITCH([main]) > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +AT_CLEANUP > +]) > -- > 2.17.1 >
diff --git a/controller/encaps.c b/controller/encaps.c index fc93bf1ee..da24448f5 100644 --- a/controller/encaps.c +++ b/controller/encaps.c @@ -152,7 +152,8 @@ encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id, static void tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, - const char *new_chassis_id, const struct sbrec_encap *encap) + const char *new_chassis_id, const struct sbrec_encap *encap, + const struct ovsrec_open_vswitch_table *ovs_table) { struct smap options = SMAP_INITIALIZER(&options); smap_add(&options, "remote_ip", encap->ip); @@ -202,6 +203,18 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, smap_add(&options, "remote_name", new_chassis_id); } + const struct ovsrec_open_vswitch *cfg = + ovsrec_open_vswitch_table_first(ovs_table); + /* If the tos option is configured, get it */ + if (cfg) { + const char *encap_tos = smap_get_def(&cfg->external_ids, + "ovn-encap-tos", "none"); + + if (encap_tos && strcmp(encap_tos, "none")) { + smap_add(&options, "tos", encap_tos); + } + } + /* If there's an existing chassis record that does not need any change, * keep it. Otherwise, create a new record (if there was an existing * record, the new record will supplant it and encaps_run() will delete @@ -270,7 +283,8 @@ preferred_encap(const struct sbrec_chassis *chassis_rec) * as there are VTEP of that type (differentiated by remote_ip) on that chassis. */ static int -chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_sb_global *sbg, struct tunnel_ctx *tc) +chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_sb_global *sbg, + const struct ovsrec_open_vswitch_table *ovs_table, struct tunnel_ctx *tc) { struct sbrec_encap *encap = preferred_encap(chassis_rec); int tuncnt = 0; @@ -286,7 +300,7 @@ chassis_tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_s if (tun_type != pref_type) { continue; } - tunnel_add(tc, sbg, chassis_rec->name, chassis_rec->encaps[i]); + tunnel_add(tc, sbg, chassis_rec->name, chassis_rec->encaps[i], ovs_table); tuncnt++; } return tuncnt; @@ -316,11 +330,12 @@ chassis_tzones_overlap(const struct sset *transport_zones, void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, - const struct ovsrec_bridge_table *bridge_table, + const struct ovsrec_bridge_table *bridge_table, const struct ovsrec_bridge *br_int, const struct sbrec_chassis_table *chassis_table, const struct sbrec_chassis *this_chassis, const struct sbrec_sb_global *sbg, + const struct ovsrec_open_vswitch_table *ovs_table, const struct sset *transport_zones) { if (!ovs_idl_txn || !br_int) { @@ -390,7 +405,7 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, continue; } - if (chassis_tunnel_add(chassis_rec, sbg, &tc) == 0) { + if (chassis_tunnel_add(chassis_rec, sbg, ovs_table, &tc) == 0) { VLOG_INFO("Creating encap for '%s' failed", chassis_rec->name); continue; } diff --git a/controller/encaps.h b/controller/encaps.h index f488393c4..25d44b034 100644 --- a/controller/encaps.h +++ b/controller/encaps.h @@ -35,6 +35,7 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, const struct sbrec_chassis_table *, const struct sbrec_chassis *, const struct sbrec_sb_global *, + const struct ovsrec_open_vswitch_table *, const struct sset *transport_zones); bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn, diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml index 8c180f576..b419cab05 100644 --- a/controller/ovn-controller.8.xml +++ b/controller/ovn-controller.8.xml @@ -201,6 +201,13 @@ performance loss. In such cases, encapsulation checksums can be disabled. </dd> + <dt><code>external_ids:ovn-encap-tos</code></dt> + <dd> + <code>ovn-encap-tos</code> indicates whether the tos (DSCP) needs + to be copied, from the inner to the outer/encap header, on tunnels + created on this chassis. + </dd> + <dt><code>external_ids:ovn-cms-options</code></dt> <dd> A list of options that will be consumed by the CMS Plugin and which diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index aa7941eeb..a719beb0e 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -3593,6 +3593,7 @@ main(int argc, char *argv[]) sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis, sbrec_sb_global_first(ovnsb_idl_loop.idl), + ovs_table, &transport_zones); stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME, diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 4ae218ed6..cd1617981 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -723,3 +723,48 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=0 OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +# Checks that ovn-controller honors the change to tunnel interface if we +# set ovn-encap-tos option +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller - change Encap ToS option]) +AT_KEYWORDS([ovn]) +ovn_start + +net_add n1 +sim_add hv +as hv +ovs-vsctl \ + -- add-br br-phys \ + -- add-br br-eth0 \ + -- add-br br-eth1 \ + -- add-br br-eth2 +ovn_attach n1 br-phys 192.168.0.1 + +check_tunnel_property () { + test "`ovs-vsctl get interface ovn-fakech-0 $1`" = "$2" +} + +# Start off with a remote chassis supporting geneve +ovn-sbctl chassis-add fakechassis geneve 192.168.0.2 +OVS_WAIT_UNTIL([check_tunnel_property type geneve]) + +as hv +ovs-vsctl \ + -- set Open_vSwitch . external-ids:ovn-encap-tos="inherit" + +# now, wait for a couple of secs +sleep 2 + +tos_option=$(ovs-vsctl get interface ovn-fakech-0 options:tos) +expected_tos_option="inherit" +AT_CHECK([test "$tos_option" = "$expected_tos_option"], [0], []) + +# Gracefully terminate daemons +OVN_CLEANUP_SBOX([hv]) +OVN_CLEANUP_VSWITCH([main]) +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +AT_CLEANUP +])