Message ID | 20220209233314.51948-1-odivlad@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [ovs-dev,v2] controller: add ovn-set-local-ip option | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <odivlad@gmail.com> wrote: > > When transport node has multiple interfaces (vlans) and > ovn-encap-ip on different hosts need to be configured > from different VLANs source IP for encapsulated packet > can be not the same, which is expected by remote system. > > Explicitely setting local_ip resolves such problem. > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> Hi Vladislav, Can you please add the code to copy the new added config from OVS database to the other_config column of Chassis table in Southbound db ? Please take a look at "struct ovs_chassis_cfg" in controller/chassis.c Thanks Numan > --- > controller/encaps.c | 37 +++++++++++++++++++++------------ > controller/ovn-controller.8.xml | 7 +++++++ > tests/ovn-controller.at | 9 ++++++++ > 3 files changed, 40 insertions(+), 13 deletions(-) > > diff --git a/controller/encaps.c b/controller/encaps.c > index 66e0cd8cd..3b0c92931 100644 > --- a/controller/encaps.c > +++ b/controller/encaps.c > @@ -23,6 +23,7 @@ > #include "openvswitch/vlog.h" > #include "lib/ovn-sb-idl.h" > #include "ovn-controller.h" > +#include "smap.h" > > VLOG_DEFINE_THIS_MODULE(encaps); > > @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, > smap_add(&options, "dst_port", dst_port); > } > > + const struct ovsrec_open_vswitch *cfg = > + ovsrec_open_vswitch_table_first(ovs_table); > + > + bool set_local_ip = false; > + if (cfg) { > + /* If the tos option is configured, get it */ > + 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 ovn-set-local-ip option is configured, get it */ > + set_local_ip = smap_get_bool(&cfg->external_ids, "ovn-set-local-ip", > + false); > + } > + > /* Add auth info if ipsec is enabled. */ > if (sbg->ipsec) { > + set_local_ip = true; > + smap_add(&options, "remote_name", new_chassis_id); > + } > + > + if (set_local_ip) { > const struct sbrec_chassis *this_chassis = tc->this_chassis; > const char *local_ip = NULL; > > @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, > if (local_ip) { > smap_add(&options, "local_ip", local_ip); > } > - 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, > diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml > index e9708fe64..cc9a7d1c2 100644 > --- a/controller/ovn-controller.8.xml > +++ b/controller/ovn-controller.8.xml > @@ -304,6 +304,13 @@ > of how many entries there are in the cache. By default this is set to > 30000 (30 seconds). > </dd> > + <dt><code>external_ids:ovn-set-local-ip</code></dt> > + <dd> > + The boolean flag indicates if <code>ovn-controller</code> when create > + tunnel ports should set <code>local_ip</code> parameter. Can be > + heplful to pin source outer IP for the tunnel when multiple interfaces > + are used on the host for overlay traffic. > + </dd> > </dl> > > <p> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 2f39e5f3e..9e6302e5a 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > ovs-vsctl del-port ovn-fakech-0 > OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > +# set `ovn-set-local-ip` option to true and check if tunnel parameters > +OVS_WAIT_WHILE([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > +ovs-vsctl set open . external_ids:ovn-set-local-ip=true > +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > + > +# Change the local_ip on the OVS side and check than OVN fixes it > +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" > +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > + > # Gracefully terminate daemons > OVN_CLEANUP_SBOX([hv]) > OVN_CLEANUP_VSWITCH([main]) > -- > 2.30.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Hi Numan, I’ve submitted the v2, please check this out: https://patchwork.ozlabs.org/project/ovn/patch/20220215145442.2868060-1-odivlad@gmail.com/ Regards, Vladislav Odintsov > On 11 Feb 2022, at 22:12, Numan Siddique <numans@ovn.org> wrote: > > On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>> wrote: >> >> When transport node has multiple interfaces (vlans) and >> ovn-encap-ip on different hosts need to be configured >> from different VLANs source IP for encapsulated packet >> can be not the same, which is expected by remote system. >> >> Explicitely setting local_ip resolves such problem. >> >> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>> > > Hi Vladislav, > > Can you please add the code to copy the new added config from OVS > database to the > other_config column of Chassis table in Southbound db ? > > Please take a look at "struct ovs_chassis_cfg" in controller/chassis.c > > Thanks > Numan > >> --- >> controller/encaps.c | 37 +++++++++++++++++++++------------ >> controller/ovn-controller.8.xml | 7 +++++++ >> tests/ovn-controller.at | 9 ++++++++ >> 3 files changed, 40 insertions(+), 13 deletions(-) >> >> diff --git a/controller/encaps.c b/controller/encaps.c >> index 66e0cd8cd..3b0c92931 100644 >> --- a/controller/encaps.c >> +++ b/controller/encaps.c >> @@ -23,6 +23,7 @@ >> #include "openvswitch/vlog.h" >> #include "lib/ovn-sb-idl.h" >> #include "ovn-controller.h" >> +#include "smap.h" >> >> VLOG_DEFINE_THIS_MODULE(encaps); >> >> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, >> smap_add(&options, "dst_port", dst_port); >> } >> >> + const struct ovsrec_open_vswitch *cfg = >> + ovsrec_open_vswitch_table_first(ovs_table); >> + >> + bool set_local_ip = false; >> + if (cfg) { >> + /* If the tos option is configured, get it */ >> + 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 ovn-set-local-ip option is configured, get it */ >> + set_local_ip = smap_get_bool(&cfg->external_ids, "ovn-set-local-ip", >> + false); >> + } >> + >> /* Add auth info if ipsec is enabled. */ >> if (sbg->ipsec) { >> + set_local_ip = true; >> + smap_add(&options, "remote_name", new_chassis_id); >> + } >> + >> + if (set_local_ip) { >> const struct sbrec_chassis *this_chassis = tc->this_chassis; >> const char *local_ip = NULL; >> >> @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, >> if (local_ip) { >> smap_add(&options, "local_ip", local_ip); >> } >> - 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, >> diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml >> index e9708fe64..cc9a7d1c2 100644 >> --- a/controller/ovn-controller.8.xml >> +++ b/controller/ovn-controller.8.xml >> @@ -304,6 +304,13 @@ >> of how many entries there are in the cache. By default this is set to >> 30000 (30 seconds). >> </dd> >> + <dt><code>external_ids:ovn-set-local-ip</code></dt> >> + <dd> >> + The boolean flag indicates if <code>ovn-controller</code> when create >> + tunnel ports should set <code>local_ip</code> parameter. Can be >> + heplful to pin source outer IP for the tunnel when multiple interfaces >> + are used on the host for overlay traffic. >> + </dd> >> </dl> >> >> <p> >> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >> index 2f39e5f3e..9e6302e5a 100644 >> --- a/tests/ovn-controller.at >> +++ b/tests/ovn-controller.at >> @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) >> ovs-vsctl del-port ovn-fakech-0 >> OVS_WAIT_UNTIL([check_tunnel_property type geneve]) >> >> +# set `ovn-set-local-ip` option to true and check if tunnel parameters >> +OVS_WAIT_WHILE([check_tunnel_property options:local_ip "\"192.168.0.1\""]) >> +ovs-vsctl set open . external_ids:ovn-set-local-ip=true >> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) >> + >> +# Change the local_ip on the OVS side and check than OVN fixes it >> +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" >> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) >> + >> # Gracefully terminate daemons >> OVN_CLEANUP_SBOX([hv]) >> OVN_CLEANUP_VSWITCH([main]) >> -- >> 2.30.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique <numans@ovn.org> wrote: > > On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <odivlad@gmail.com> wrote: > > > > When transport node has multiple interfaces (vlans) and > > ovn-encap-ip on different hosts need to be configured > > from different VLANs source IP for encapsulated packet > > can be not the same, which is expected by remote system. > > > > Explicitely setting local_ip resolves such problem. > > > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > > Hi Vladislav, > > Can you please add the code to copy the new added config from OVS > database to the > other_config column of Chassis table in Southbound db ? > > Please take a look at "struct ovs_chassis_cfg" in controller/chassis.c > > Thanks > Numan Hi Numan, May I ask what's the purpose of adding this to other_config in SB? I understand that there are fields in other_config that is of importance for other chassises, so need to be added to SB, but for this one, how would it be used in SB DB? Hi Vladislav, Regarding this: VLOG_ERR("ovn-encap-ip has been configured as a list. This " "is unsupported for IPsec."); Before your change it applies to IPsec only, but with your change this would apply to non-IPsec as well, if ovn-set-local-ip is true. Could you update the error log as well? (it is better to be ratelimited, but it is not the fault of your patch) Thanks, Han > > > --- > > controller/encaps.c | 37 +++++++++++++++++++++------------ > > controller/ovn-controller.8.xml | 7 +++++++ > > tests/ovn-controller.at | 9 ++++++++ > > 3 files changed, 40 insertions(+), 13 deletions(-) > > > > diff --git a/controller/encaps.c b/controller/encaps.c > > index 66e0cd8cd..3b0c92931 100644 > > --- a/controller/encaps.c > > +++ b/controller/encaps.c > > @@ -23,6 +23,7 @@ > > #include "openvswitch/vlog.h" > > #include "lib/ovn-sb-idl.h" > > #include "ovn-controller.h" > > +#include "smap.h" > > > > VLOG_DEFINE_THIS_MODULE(encaps); > > > > @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, > > smap_add(&options, "dst_port", dst_port); > > } > > > > + const struct ovsrec_open_vswitch *cfg = > > + ovsrec_open_vswitch_table_first(ovs_table); > > + > > + bool set_local_ip = false; > > + if (cfg) { > > + /* If the tos option is configured, get it */ > > + 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 ovn-set-local-ip option is configured, get it */ > > + set_local_ip = smap_get_bool(&cfg->external_ids, "ovn-set-local-ip", > > + false); > > + } > > + > > /* Add auth info if ipsec is enabled. */ > > if (sbg->ipsec) { > > + set_local_ip = true; > > + smap_add(&options, "remote_name", new_chassis_id); > > + } > > + > > + if (set_local_ip) { > > const struct sbrec_chassis *this_chassis = tc->this_chassis; > > const char *local_ip = NULL; > > > > @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, > > if (local_ip) { > > smap_add(&options, "local_ip", local_ip); > > } > > - 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, > > diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml > > index e9708fe64..cc9a7d1c2 100644 > > --- a/controller/ovn-controller.8.xml > > +++ b/controller/ovn-controller.8.xml > > @@ -304,6 +304,13 @@ > > of how many entries there are in the cache. By default this is set to > > 30000 (30 seconds). > > </dd> > > + <dt><code>external_ids:ovn-set-local-ip</code></dt> > > + <dd> > > + The boolean flag indicates if <code>ovn-controller</code> when create > > + tunnel ports should set <code>local_ip</code> parameter. Can be > > + heplful to pin source outer IP for the tunnel when multiple interfaces > > + are used on the host for overlay traffic. > > + </dd> > > </dl> > > > > <p> > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > index 2f39e5f3e..9e6302e5a 100644 > > --- a/tests/ovn-controller.at > > +++ b/tests/ovn-controller.at > > @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > ovs-vsctl del-port ovn-fakech-0 > > OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > > > +# set `ovn-set-local-ip` option to true and check if tunnel parameters > > +OVS_WAIT_WHILE([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > > +ovs-vsctl set open . external_ids:ovn-set-local-ip=true > > +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > > + > > +# Change the local_ip on the OVS side and check than OVN fixes it > > +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" > > +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) > > + > > # Gracefully terminate daemons > > OVN_CLEANUP_SBOX([hv]) > > OVN_CLEANUP_VSWITCH([main]) > > -- > > 2.30.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Han, thanks for the note about log message, I’ll fix this in v3 right after the question with other_config is resolved. Frankly speaking I also don’t understand why to sync ovn-set-local-ip option to other_config — I did this only because Numan asked to do :) Regards, Vladislav Odintsov > On 17 Feb 2022, at 09:39, Han Zhou <hzhou@ovn.org> wrote: > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> wrote: >> >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <odivlad@gmail.com> > wrote: >>> >>> When transport node has multiple interfaces (vlans) and >>> ovn-encap-ip on different hosts need to be configured >>> from different VLANs source IP for encapsulated packet >>> can be not the same, which is expected by remote system. >>> >>> Explicitely setting local_ip resolves such problem. >>> >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >> >> Hi Vladislav, >> >> Can you please add the code to copy the new added config from OVS >> database to the >> other_config column of Chassis table in Southbound db ? >> >> Please take a look at "struct ovs_chassis_cfg" in controller/chassis.c >> >> Thanks >> Numan > > Hi Numan, > > May I ask what's the purpose of adding this to other_config in SB? I > understand that there are fields in other_config that is of importance for > other chassises, so need to be added to SB, but for this one, how would it > be used in SB DB? > > Hi Vladislav, > > Regarding this: > VLOG_ERR("ovn-encap-ip has been configured as a list. This " > "is unsupported for IPsec."); > > Before your change it applies to IPsec only, but with your change this > would apply to non-IPsec as well, if ovn-set-local-ip is true. Could you > update the error log as well? (it is better to be ratelimited, but it is > not the fault of your patch) > > Thanks, > Han > >> >>> --- >>> controller/encaps.c | 37 +++++++++++++++++++++------------ >>> controller/ovn-controller.8.xml | 7 +++++++ >>> tests/ovn-controller.at | 9 ++++++++ >>> 3 files changed, 40 insertions(+), 13 deletions(-) >>> >>> diff --git a/controller/encaps.c b/controller/encaps.c >>> index 66e0cd8cd..3b0c92931 100644 >>> --- a/controller/encaps.c >>> +++ b/controller/encaps.c >>> @@ -23,6 +23,7 @@ >>> #include "openvswitch/vlog.h" >>> #include "lib/ovn-sb-idl.h" >>> #include "ovn-controller.h" >>> +#include "smap.h" >>> >>> VLOG_DEFINE_THIS_MODULE(encaps); >>> >>> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct > sbrec_sb_global *sbg, >>> smap_add(&options, "dst_port", dst_port); >>> } >>> >>> + const struct ovsrec_open_vswitch *cfg = >>> + ovsrec_open_vswitch_table_first(ovs_table); >>> + >>> + bool set_local_ip = false; >>> + if (cfg) { >>> + /* If the tos option is configured, get it */ >>> + 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 ovn-set-local-ip option is configured, get it */ >>> + set_local_ip = smap_get_bool(&cfg->external_ids, > "ovn-set-local-ip", >>> + false); >>> + } >>> + >>> /* Add auth info if ipsec is enabled. */ >>> if (sbg->ipsec) { >>> + set_local_ip = true; >>> + smap_add(&options, "remote_name", new_chassis_id); >>> + } >>> + >>> + if (set_local_ip) { >>> const struct sbrec_chassis *this_chassis = tc->this_chassis; >>> const char *local_ip = NULL; >>> >>> @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct > sbrec_sb_global *sbg, >>> if (local_ip) { >>> smap_add(&options, "local_ip", local_ip); >>> } >>> - 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, >>> diff --git a/controller/ovn-controller.8.xml > b/controller/ovn-controller.8.xml >>> index e9708fe64..cc9a7d1c2 100644 >>> --- a/controller/ovn-controller.8.xml >>> +++ b/controller/ovn-controller.8.xml >>> @@ -304,6 +304,13 @@ >>> of how many entries there are in the cache. By default this > is set to >>> 30000 (30 seconds). >>> </dd> >>> + <dt><code>external_ids:ovn-set-local-ip</code></dt> >>> + <dd> >>> + The boolean flag indicates if <code>ovn-controller</code> when > create >>> + tunnel ports should set <code>local_ip</code> parameter. Can > be >>> + heplful to pin source outer IP for the tunnel when multiple > interfaces >>> + are used on the host for overlay traffic. >>> + </dd> >>> </dl> >>> >>> <p> >>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >>> index 2f39e5f3e..9e6302e5a 100644 >>> --- a/tests/ovn-controller.at >>> +++ b/tests/ovn-controller.at >>> @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) >>> ovs-vsctl del-port ovn-fakech-0 >>> OVS_WAIT_UNTIL([check_tunnel_property type geneve]) >>> >>> +# set `ovn-set-local-ip` option to true and check if tunnel parameters >>> +OVS_WAIT_WHILE([check_tunnel_property options:local_ip > "\"192.168.0.1\""]) >>> +ovs-vsctl set open . external_ids:ovn-set-local-ip=true >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > "\"192.168.0.1\""]) >>> + >>> +# Change the local_ip on the OVS side and check than OVN fixes it >>> +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > "\"192.168.0.1\""]) >>> + >>> # Gracefully terminate daemons >>> OVN_CLEANUP_SBOX([hv]) >>> OVN_CLEANUP_VSWITCH([main]) >>> -- >>> 2.30.0 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov <odivlad@gmail.com> wrote: > > Hi Han, > > thanks for the note about log message, I’ll fix this in v3 right after the question with other_config is resolved. > Frankly speaking I also don’t understand why to sync ovn-set-local-ip option to other_config — > I did this only because Numan asked to do :) > > Regards, > Vladislav Odintsov > > > On 17 Feb 2022, at 09:39, Han Zhou <hzhou@ovn.org> wrote: > > > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> wrote: > >> > >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <odivlad@gmail.com> > > wrote: > >>> > >>> When transport node has multiple interfaces (vlans) and > >>> ovn-encap-ip on different hosts need to be configured > >>> from different VLANs source IP for encapsulated packet > >>> can be not the same, which is expected by remote system. > >>> > >>> Explicitely setting local_ip resolves such problem. > >>> > >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > >> > >> Hi Vladislav, > >> > >> Can you please add the code to copy the new added config from OVS > >> database to the > >> other_config column of Chassis table in Southbound db ? > >> > >> Please take a look at "struct ovs_chassis_cfg" in controller/chassis.c > >> > >> Thanks > >> Numan > > > > Hi Numan, > > > > May I ask what's the purpose of adding this to other_config in SB? I > > understand that there are fields in other_config that is of importance for > > other chassises, so need to be added to SB, but for this one, how would it > > be used in SB DB? My understanding is that we just clone all the local ovs configs into chassis's other_config and my suggestion was to make sure we are consistent with the present behaviour. Have we missed copying some of the ovs configs ? I'm actually fine either way. If you think it's better not to copy to the sb db, then I am fine with it. Numan > > Hi Vladislav, > > > > Regarding this: > > VLOG_ERR("ovn-encap-ip has been configured as a list. This " > > "is unsupported for IPsec."); > > > > Before your change it applies to IPsec only, but with your change this > > would apply to non-IPsec as well, if ovn-set-local-ip is true. Could you > > update the error log as well? (it is better to be ratelimited, but it is > > not the fault of your patch) > > > > Thanks, > > Han > > > >> > >>> --- > >>> controller/encaps.c | 37 +++++++++++++++++++++------------ > >>> controller/ovn-controller.8.xml | 7 +++++++ > >>> tests/ovn-controller.at | 9 ++++++++ > >>> 3 files changed, 40 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/controller/encaps.c b/controller/encaps.c > >>> index 66e0cd8cd..3b0c92931 100644 > >>> --- a/controller/encaps.c > >>> +++ b/controller/encaps.c > >>> @@ -23,6 +23,7 @@ > >>> #include "openvswitch/vlog.h" > >>> #include "lib/ovn-sb-idl.h" > >>> #include "ovn-controller.h" > >>> +#include "smap.h" > >>> > >>> VLOG_DEFINE_THIS_MODULE(encaps); > >>> > >>> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct > > sbrec_sb_global *sbg, > >>> smap_add(&options, "dst_port", dst_port); > >>> } > >>> > >>> + const struct ovsrec_open_vswitch *cfg = > >>> + ovsrec_open_vswitch_table_first(ovs_table); > >>> + > >>> + bool set_local_ip = false; > >>> + if (cfg) { > >>> + /* If the tos option is configured, get it */ > >>> + 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 ovn-set-local-ip option is configured, get it */ > >>> + set_local_ip = smap_get_bool(&cfg->external_ids, > > "ovn-set-local-ip", > >>> + false); > >>> + } > >>> + > >>> /* Add auth info if ipsec is enabled. */ > >>> if (sbg->ipsec) { > >>> + set_local_ip = true; > >>> + smap_add(&options, "remote_name", new_chassis_id); > >>> + } > >>> + > >>> + if (set_local_ip) { > >>> const struct sbrec_chassis *this_chassis = tc->this_chassis; > >>> const char *local_ip = NULL; > >>> > >>> @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct > > sbrec_sb_global *sbg, > >>> if (local_ip) { > >>> smap_add(&options, "local_ip", local_ip); > >>> } > >>> - 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, > >>> diff --git a/controller/ovn-controller.8.xml > > b/controller/ovn-controller.8.xml > >>> index e9708fe64..cc9a7d1c2 100644 > >>> --- a/controller/ovn-controller.8.xml > >>> +++ b/controller/ovn-controller.8.xml > >>> @@ -304,6 +304,13 @@ > >>> of how many entries there are in the cache. By default this > > is set to > >>> 30000 (30 seconds). > >>> </dd> > >>> + <dt><code>external_ids:ovn-set-local-ip</code></dt> > >>> + <dd> > >>> + The boolean flag indicates if <code>ovn-controller</code> when > > create > >>> + tunnel ports should set <code>local_ip</code> parameter. Can > > be > >>> + heplful to pin source outer IP for the tunnel when multiple > > interfaces > >>> + are used on the host for overlay traffic. > >>> + </dd> > >>> </dl> > >>> > >>> <p> > >>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > >>> index 2f39e5f3e..9e6302e5a 100644 > >>> --- a/tests/ovn-controller.at > >>> +++ b/tests/ovn-controller.at > >>> @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > >>> ovs-vsctl del-port ovn-fakech-0 > >>> OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > >>> > >>> +# set `ovn-set-local-ip` option to true and check if tunnel parameters > >>> +OVS_WAIT_WHILE([check_tunnel_property options:local_ip > > "\"192.168.0.1\""]) > >>> +ovs-vsctl set open . external_ids:ovn-set-local-ip=true > >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > > "\"192.168.0.1\""]) > >>> + > >>> +# Change the local_ip on the OVS side and check than OVN fixes it > >>> +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" > >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > > "\"192.168.0.1\""]) > >>> + > >>> # Gracefully terminate daemons > >>> OVN_CLEANUP_SBOX([hv]) > >>> OVN_CLEANUP_VSWITCH([main]) > >>> -- > >>> 2.30.0 > >>> > >>> _______________________________________________ > >>> dev mailing list > >>> dev@openvswitch.org > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique <numans@ovn.org> wrote: > > On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov <odivlad@gmail.com> wrote: > > > > Hi Han, > > > > thanks for the note about log message, I’ll fix this in v3 right after the question with other_config is resolved. > > Frankly speaking I also don’t understand why to sync ovn-set-local-ip option to other_config — > > I did this only because Numan asked to do :) > > > > Regards, > > Vladislav Odintsov > > > > > On 17 Feb 2022, at 09:39, Han Zhou <hzhou@ovn.org> wrote: > > > > > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> wrote: > > >> > > >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <odivlad@gmail.com> > > > wrote: > > >>> > > >>> When transport node has multiple interfaces (vlans) and > > >>> ovn-encap-ip on different hosts need to be configured > > >>> from different VLANs source IP for encapsulated packet > > >>> can be not the same, which is expected by remote system. > > >>> > > >>> Explicitely setting local_ip resolves such problem. > > >>> > > >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > > >> > > >> Hi Vladislav, > > >> > > >> Can you please add the code to copy the new added config from OVS > > >> database to the > > >> other_config column of Chassis table in Southbound db ? > > >> > > >> Please take a look at "struct ovs_chassis_cfg" in controller/chassis.c > > >> > > >> Thanks > > >> Numan > > > > > > Hi Numan, > > > > > > May I ask what's the purpose of adding this to other_config in SB? I > > > understand that there are fields in other_config that is of importance for > > > other chassises, so need to be added to SB, but for this one, how would it > > > be used in SB DB? > > My understanding is that we just clone all the local ovs configs into > chassis's other_config and my suggestion was to > make sure we are consistent with the present behaviour. Have we > missed copying some of the ovs configs ? > > I'm actually fine either way. If you think it's better not to copy to > the sb db, then I am fine with it. > Thanks Numan for the confirmation. I was wondering if I missed any important use case of the OVS configs being stored in SB, now it seems it is fine not adding them. There are in fact many OVS configs not in SB, such as ovn-remote, ovn-remote-probe-interval, ovn-openflow-probe-interval, ovn-encap-tos, ovn-match-northd-version, etc. All these are only locally important, so I don't think it is necessary to sync them to SB. Probably there were historical reasons why some of the configs were synced to SB while they were useful only locally, and I can't recall all the details. Maybe we can remove the unnecessary ones from SB in a separate patch to reduce the noise of the SB chassis table, not urgent though. Thanks, Han > Numan > > > > Hi Vladislav, > > > > > > Regarding this: > > > VLOG_ERR("ovn-encap-ip has been configured as a list. This " > > > "is unsupported for IPsec."); > > > > > > Before your change it applies to IPsec only, but with your change this > > > would apply to non-IPsec as well, if ovn-set-local-ip is true. Could you > > > update the error log as well? (it is better to be ratelimited, but it is > > > not the fault of your patch) > > > > > > Thanks, > > > Han > > > > > >> > > >>> --- > > >>> controller/encaps.c | 37 +++++++++++++++++++++------------ > > >>> controller/ovn-controller.8.xml | 7 +++++++ > > >>> tests/ovn-controller.at | 9 ++++++++ > > >>> 3 files changed, 40 insertions(+), 13 deletions(-) > > >>> > > >>> diff --git a/controller/encaps.c b/controller/encaps.c > > >>> index 66e0cd8cd..3b0c92931 100644 > > >>> --- a/controller/encaps.c > > >>> +++ b/controller/encaps.c > > >>> @@ -23,6 +23,7 @@ > > >>> #include "openvswitch/vlog.h" > > >>> #include "lib/ovn-sb-idl.h" > > >>> #include "ovn-controller.h" > > >>> +#include "smap.h" > > >>> > > >>> VLOG_DEFINE_THIS_MODULE(encaps); > > >>> > > >>> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct > > > sbrec_sb_global *sbg, > > >>> smap_add(&options, "dst_port", dst_port); > > >>> } > > >>> > > >>> + const struct ovsrec_open_vswitch *cfg = > > >>> + ovsrec_open_vswitch_table_first(ovs_table); > > >>> + > > >>> + bool set_local_ip = false; > > >>> + if (cfg) { > > >>> + /* If the tos option is configured, get it */ > > >>> + 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 ovn-set-local-ip option is configured, get it */ > > >>> + set_local_ip = smap_get_bool(&cfg->external_ids, > > > "ovn-set-local-ip", > > >>> + false); > > >>> + } > > >>> + > > >>> /* Add auth info if ipsec is enabled. */ > > >>> if (sbg->ipsec) { > > >>> + set_local_ip = true; > > >>> + smap_add(&options, "remote_name", new_chassis_id); > > >>> + } > > >>> + > > >>> + if (set_local_ip) { > > >>> const struct sbrec_chassis *this_chassis = tc->this_chassis; > > >>> const char *local_ip = NULL; > > >>> > > >>> @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct > > > sbrec_sb_global *sbg, > > >>> if (local_ip) { > > >>> smap_add(&options, "local_ip", local_ip); > > >>> } > > >>> - 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, > > >>> diff --git a/controller/ovn-controller.8.xml > > > b/controller/ovn-controller.8.xml > > >>> index e9708fe64..cc9a7d1c2 100644 > > >>> --- a/controller/ovn-controller.8.xml > > >>> +++ b/controller/ovn-controller.8.xml > > >>> @@ -304,6 +304,13 @@ > > >>> of how many entries there are in the cache. By default this > > > is set to > > >>> 30000 (30 seconds). > > >>> </dd> > > >>> + <dt><code>external_ids:ovn-set-local-ip</code></dt> > > >>> + <dd> > > >>> + The boolean flag indicates if <code>ovn-controller</code> when > > > create > > >>> + tunnel ports should set <code>local_ip</code> parameter. Can > > > be > > >>> + heplful to pin source outer IP for the tunnel when multiple > > > interfaces > > >>> + are used on the host for overlay traffic. > > >>> + </dd> > > >>> </dl> > > >>> > > >>> <p> > > >>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > >>> index 2f39e5f3e..9e6302e5a 100644 > > >>> --- a/tests/ovn-controller.at > > >>> +++ b/tests/ovn-controller.at > > >>> @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > >>> ovs-vsctl del-port ovn-fakech-0 > > >>> OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > >>> > > >>> +# set `ovn-set-local-ip` option to true and check if tunnel parameters > > >>> +OVS_WAIT_WHILE([check_tunnel_property options:local_ip > > > "\"192.168.0.1\""]) > > >>> +ovs-vsctl set open . external_ids:ovn-set-local-ip=true > > >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > > > "\"192.168.0.1\""]) > > >>> + > > >>> +# Change the local_ip on the OVS side and check than OVN fixes it > > >>> +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" > > >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > > > "\"192.168.0.1\""]) > > >>> + > > >>> # Gracefully terminate daemons > > >>> OVN_CLEANUP_SBOX([hv]) > > >>> OVN_CLEANUP_VSWITCH([main]) > > >>> -- > > >>> 2.30.0 > > >>> > > >>> _______________________________________________ > > >>> dev mailing list > > >>> dev@openvswitch.org > > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >>> > > >> _______________________________________________ > > >> dev mailing list > > >> dev@openvswitch.org > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev < https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Feb 17, 2022 at 5:50 PM Han Zhou <hzhou@ovn.org> wrote: > > On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique <numans@ovn.org> wrote: > > > > On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov <odivlad@gmail.com> > wrote: > > > > > > Hi Han, > > > > > > thanks for the note about log message, I’ll fix this in v3 right after > the question with other_config is resolved. > > > Frankly speaking I also don’t understand why to sync ovn-set-local-ip > option to other_config — > > > I did this only because Numan asked to do :) > > > > > > Regards, > > > Vladislav Odintsov > > > > > > > On 17 Feb 2022, at 09:39, Han Zhou <hzhou@ovn.org> wrote: > > > > > > > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique <numans@ovn.org > <mailto:numans@ovn.org>> wrote: > > > >> > > > >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <odivlad@gmail.com> > > > > wrote: > > > >>> > > > >>> When transport node has multiple interfaces (vlans) and > > > >>> ovn-encap-ip on different hosts need to be configured > > > >>> from different VLANs source IP for encapsulated packet > > > >>> can be not the same, which is expected by remote system. > > > >>> > > > >>> Explicitely setting local_ip resolves such problem. > > > >>> > > > >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > > > >> > > > >> Hi Vladislav, > > > >> > > > >> Can you please add the code to copy the new added config from OVS > > > >> database to the > > > >> other_config column of Chassis table in Southbound db ? > > > >> > > > >> Please take a look at "struct ovs_chassis_cfg" in > controller/chassis.c > > > >> > > > >> Thanks > > > >> Numan > > > > > > > > Hi Numan, > > > > > > > > May I ask what's the purpose of adding this to other_config in SB? I > > > > understand that there are fields in other_config that is of > importance for > > > > other chassises, so need to be added to SB, but for this one, how > would it > > > > be used in SB DB? > > > > My understanding is that we just clone all the local ovs configs into > > chassis's other_config and my suggestion was to > > make sure we are consistent with the present behaviour. Have we > > missed copying some of the ovs configs ? > > > > I'm actually fine either way. If you think it's better not to copy to > > the sb db, then I am fine with it. > > > > Thanks Numan for the confirmation. I was wondering if I missed any > important use case of the OVS configs being stored in SB, now it seems it > is fine not adding them. There are in fact many OVS configs not in SB, such > as ovn-remote, ovn-remote-probe-interval, ovn-openflow-probe-interval, > ovn-encap-tos, ovn-match-northd-version, etc. All these are only locally > important, so I don't think it is necessary to sync them to SB. Probably > there were historical reasons why some of the configs were synced to SB > while they were useful only locally, and I can't recall all the details. > Maybe we can remove the unnecessary ones from SB in a separate patch to > reduce the noise of the SB chassis table, not urgent though. Sounds good to me. Does @Vladislav Odintsov need to submit another version reverting the change I requested ? Numan > > Thanks, > Han > > > Numan > > > > > > Hi Vladislav, > > > > > > > > Regarding this: > > > > VLOG_ERR("ovn-encap-ip has been configured as a list. > This " > > > > "is unsupported for IPsec."); > > > > > > > > Before your change it applies to IPsec only, but with your change this > > > > would apply to non-IPsec as well, if ovn-set-local-ip is true. Could > you > > > > update the error log as well? (it is better to be ratelimited, but it > is > > > > not the fault of your patch) > > > > > > > > Thanks, > > > > Han > > > > > > > >> > > > >>> --- > > > >>> controller/encaps.c | 37 > +++++++++++++++++++++------------ > > > >>> controller/ovn-controller.8.xml | 7 +++++++ > > > >>> tests/ovn-controller.at | 9 ++++++++ > > > >>> 3 files changed, 40 insertions(+), 13 deletions(-) > > > >>> > > > >>> diff --git a/controller/encaps.c b/controller/encaps.c > > > >>> index 66e0cd8cd..3b0c92931 100644 > > > >>> --- a/controller/encaps.c > > > >>> +++ b/controller/encaps.c > > > >>> @@ -23,6 +23,7 @@ > > > >>> #include "openvswitch/vlog.h" > > > >>> #include "lib/ovn-sb-idl.h" > > > >>> #include "ovn-controller.h" > > > >>> +#include "smap.h" > > > >>> > > > >>> VLOG_DEFINE_THIS_MODULE(encaps); > > > >>> > > > >>> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct > > > > sbrec_sb_global *sbg, > > > >>> smap_add(&options, "dst_port", dst_port); > > > >>> } > > > >>> > > > >>> + const struct ovsrec_open_vswitch *cfg = > > > >>> + ovsrec_open_vswitch_table_first(ovs_table); > > > >>> + > > > >>> + bool set_local_ip = false; > > > >>> + if (cfg) { > > > >>> + /* If the tos option is configured, get it */ > > > >>> + 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 ovn-set-local-ip option is configured, get it */ > > > >>> + set_local_ip = smap_get_bool(&cfg->external_ids, > > > > "ovn-set-local-ip", > > > >>> + false); > > > >>> + } > > > >>> + > > > >>> /* Add auth info if ipsec is enabled. */ > > > >>> if (sbg->ipsec) { > > > >>> + set_local_ip = true; > > > >>> + smap_add(&options, "remote_name", new_chassis_id); > > > >>> + } > > > >>> + > > > >>> + if (set_local_ip) { > > > >>> const struct sbrec_chassis *this_chassis = tc->this_chassis; > > > >>> const char *local_ip = NULL; > > > >>> > > > >>> @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct > > > > sbrec_sb_global *sbg, > > > >>> if (local_ip) { > > > >>> smap_add(&options, "local_ip", local_ip); > > > >>> } > > > >>> - 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, > > > >>> diff --git a/controller/ovn-controller.8.xml > > > > b/controller/ovn-controller.8.xml > > > >>> index e9708fe64..cc9a7d1c2 100644 > > > >>> --- a/controller/ovn-controller.8.xml > > > >>> +++ b/controller/ovn-controller.8.xml > > > >>> @@ -304,6 +304,13 @@ > > > >>> of how many entries there are in the cache. By default this > > > > is set to > > > >>> 30000 (30 seconds). > > > >>> </dd> > > > >>> + <dt><code>external_ids:ovn-set-local-ip</code></dt> > > > >>> + <dd> > > > >>> + The boolean flag indicates if <code>ovn-controller</code> > when > > > > create > > > >>> + tunnel ports should set <code>local_ip</code> parameter. > Can > > > > be > > > >>> + heplful to pin source outer IP for the tunnel when multiple > > > > interfaces > > > >>> + are used on the host for overlay traffic. > > > >>> + </dd> > > > >>> </dl> > > > >>> > > > >>> <p> > > > >>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > > >>> index 2f39e5f3e..9e6302e5a 100644 > > > >>> --- a/tests/ovn-controller.at > > > >>> +++ b/tests/ovn-controller.at > > > >>> @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type > geneve]) > > > >>> ovs-vsctl del-port ovn-fakech-0 > > > >>> OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > > >>> > > > >>> +# set `ovn-set-local-ip` option to true and check if tunnel > parameters > > > >>> +OVS_WAIT_WHILE([check_tunnel_property options:local_ip > > > > "\"192.168.0.1\""]) > > > >>> +ovs-vsctl set open . external_ids:ovn-set-local-ip=true > > > >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > > > > "\"192.168.0.1\""]) > > > >>> + > > > >>> +# Change the local_ip on the OVS side and check than OVN fixes it > > > >>> +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" > > > >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > > > > "\"192.168.0.1\""]) > > > >>> + > > > >>> # Gracefully terminate daemons > > > >>> OVN_CLEANUP_SBOX([hv]) > > > >>> OVN_CLEANUP_VSWITCH([main]) > > > >>> -- > > > >>> 2.30.0 > > > >>> > > > >>> _______________________________________________ > > > >>> dev mailing list > > > >>> dev@openvswitch.org > > > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >>> > > > >> _______________________________________________ > > > >> dev mailing list > > > >> dev@openvswitch.org > > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev < > https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Feb 17, 2022 at 3:57 PM Numan Siddique <numans@ovn.org> wrote: > > On Thu, Feb 17, 2022 at 5:50 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique <numans@ovn.org> wrote: > > > > > > On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov <odivlad@gmail.com> > > wrote: > > > > > > > > Hi Han, > > > > > > > > thanks for the note about log message, I’ll fix this in v3 right after > > the question with other_config is resolved. > > > > Frankly speaking I also don’t understand why to sync ovn-set-local-ip > > option to other_config — > > > > I did this only because Numan asked to do :) > > > > > > > > Regards, > > > > Vladislav Odintsov > > > > > > > > > On 17 Feb 2022, at 09:39, Han Zhou <hzhou@ovn.org> wrote: > > > > > > > > > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique <numans@ovn.org > > <mailto:numans@ovn.org>> wrote: > > > > >> > > > > >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov < odivlad@gmail.com> > > > > > wrote: > > > > >>> > > > > >>> When transport node has multiple interfaces (vlans) and > > > > >>> ovn-encap-ip on different hosts need to be configured > > > > >>> from different VLANs source IP for encapsulated packet > > > > >>> can be not the same, which is expected by remote system. > > > > >>> > > > > >>> Explicitely setting local_ip resolves such problem. > > > > >>> > > > > >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > > > > >> > > > > >> Hi Vladislav, > > > > >> > > > > >> Can you please add the code to copy the new added config from OVS > > > > >> database to the > > > > >> other_config column of Chassis table in Southbound db ? > > > > >> > > > > >> Please take a look at "struct ovs_chassis_cfg" in > > controller/chassis.c > > > > >> > > > > >> Thanks > > > > >> Numan > > > > > > > > > > Hi Numan, > > > > > > > > > > May I ask what's the purpose of adding this to other_config in SB? I > > > > > understand that there are fields in other_config that is of > > importance for > > > > > other chassises, so need to be added to SB, but for this one, how > > would it > > > > > be used in SB DB? > > > > > > My understanding is that we just clone all the local ovs configs into > > > chassis's other_config and my suggestion was to > > > make sure we are consistent with the present behaviour. Have we > > > missed copying some of the ovs configs ? > > > > > > I'm actually fine either way. If you think it's better not to copy to > > > the sb db, then I am fine with it. > > > > > > > Thanks Numan for the confirmation. I was wondering if I missed any > > important use case of the OVS configs being stored in SB, now it seems it > > is fine not adding them. There are in fact many OVS configs not in SB, such > > as ovn-remote, ovn-remote-probe-interval, ovn-openflow-probe-interval, > > ovn-encap-tos, ovn-match-northd-version, etc. All these are only locally > > important, so I don't think it is necessary to sync them to SB. Probably > > there were historical reasons why some of the configs were synced to SB > > while they were useful only locally, and I can't recall all the details. > > Maybe we can remove the unnecessary ones from SB in a separate patch to > > reduce the noise of the SB chassis table, not urgent though. > > Sounds good to me. > > Does @Vladislav Odintsov need to submit another version reverting the > change I requested ? > I think Vladislav mentioned that he will submit v3 to fix the error log, maybe he could revert the other-config change in v3, too. > Numan > > > > > Thanks, > > Han > > > > > Numan > > > > > > > > Hi Vladislav, > > > > > > > > > > Regarding this: > > > > > VLOG_ERR("ovn-encap-ip has been configured as a list. > > This " > > > > > "is unsupported for IPsec."); > > > > > > > > > > Before your change it applies to IPsec only, but with your change this > > > > > would apply to non-IPsec as well, if ovn-set-local-ip is true. Could > > you > > > > > update the error log as well? (it is better to be ratelimited, but it > > is > > > > > not the fault of your patch) > > > > > > > > > > Thanks, > > > > > Han > > > > > > > > > >> > > > > >>> --- > > > > >>> controller/encaps.c | 37 > > +++++++++++++++++++++------------ > > > > >>> controller/ovn-controller.8.xml | 7 +++++++ > > > > >>> tests/ovn-controller.at | 9 ++++++++ > > > > >>> 3 files changed, 40 insertions(+), 13 deletions(-) > > > > >>> > > > > >>> diff --git a/controller/encaps.c b/controller/encaps.c > > > > >>> index 66e0cd8cd..3b0c92931 100644 > > > > >>> --- a/controller/encaps.c > > > > >>> +++ b/controller/encaps.c > > > > >>> @@ -23,6 +23,7 @@ > > > > >>> #include "openvswitch/vlog.h" > > > > >>> #include "lib/ovn-sb-idl.h" > > > > >>> #include "ovn-controller.h" > > > > >>> +#include "smap.h" > > > > >>> > > > > >>> VLOG_DEFINE_THIS_MODULE(encaps); > > > > >>> > > > > >>> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct > > > > > sbrec_sb_global *sbg, > > > > >>> smap_add(&options, "dst_port", dst_port); > > > > >>> } > > > > >>> > > > > >>> + const struct ovsrec_open_vswitch *cfg = > > > > >>> + ovsrec_open_vswitch_table_first(ovs_table); > > > > >>> + > > > > >>> + bool set_local_ip = false; > > > > >>> + if (cfg) { > > > > >>> + /* If the tos option is configured, get it */ > > > > >>> + 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 ovn-set-local-ip option is configured, get it */ > > > > >>> + set_local_ip = smap_get_bool(&cfg->external_ids, > > > > > "ovn-set-local-ip", > > > > >>> + false); > > > > >>> + } > > > > >>> + > > > > >>> /* Add auth info if ipsec is enabled. */ > > > > >>> if (sbg->ipsec) { > > > > >>> + set_local_ip = true; > > > > >>> + smap_add(&options, "remote_name", new_chassis_id); > > > > >>> + } > > > > >>> + > > > > >>> + if (set_local_ip) { > > > > >>> const struct sbrec_chassis *this_chassis = tc->this_chassis; > > > > >>> const char *local_ip = NULL; > > > > >>> > > > > >>> @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct > > > > > sbrec_sb_global *sbg, > > > > >>> if (local_ip) { > > > > >>> smap_add(&options, "local_ip", local_ip); > > > > >>> } > > > > >>> - 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, > > > > >>> diff --git a/controller/ovn-controller.8.xml > > > > > b/controller/ovn-controller.8.xml > > > > >>> index e9708fe64..cc9a7d1c2 100644 > > > > >>> --- a/controller/ovn-controller.8.xml > > > > >>> +++ b/controller/ovn-controller.8.xml > > > > >>> @@ -304,6 +304,13 @@ > > > > >>> of how many entries there are in the cache. By default this > > > > > is set to > > > > >>> 30000 (30 seconds). > > > > >>> </dd> > > > > >>> + <dt><code>external_ids:ovn-set-local-ip</code></dt> > > > > >>> + <dd> > > > > >>> + The boolean flag indicates if <code>ovn-controller</code> > > when > > > > > create > > > > >>> + tunnel ports should set <code>local_ip</code> parameter. > > Can > > > > > be > > > > >>> + heplful to pin source outer IP for the tunnel when multiple > > > > > interfaces > > > > >>> + are used on the host for overlay traffic. > > > > >>> + </dd> > > > > >>> </dl> > > > > >>> > > > > >>> <p> > > > > >>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > > > >>> index 2f39e5f3e..9e6302e5a 100644 > > > > >>> --- a/tests/ovn-controller.at > > > > >>> +++ b/tests/ovn-controller.at > > > > >>> @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type > > geneve]) > > > > >>> ovs-vsctl del-port ovn-fakech-0 > > > > >>> OVS_WAIT_UNTIL([check_tunnel_property type geneve]) > > > > >>> > > > > >>> +# set `ovn-set-local-ip` option to true and check if tunnel > > parameters > > > > >>> +OVS_WAIT_WHILE([check_tunnel_property options:local_ip > > > > > "\"192.168.0.1\""]) > > > > >>> +ovs-vsctl set open . external_ids:ovn-set-local-ip=true > > > > >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > > > > > "\"192.168.0.1\""]) > > > > >>> + > > > > >>> +# Change the local_ip on the OVS side and check than OVN fixes it > > > > >>> +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" > > > > >>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip > > > > > "\"192.168.0.1\""]) > > > > >>> + > > > > >>> # Gracefully terminate daemons > > > > >>> OVN_CLEANUP_SBOX([hv]) > > > > >>> OVN_CLEANUP_VSWITCH([main]) > > > > >>> -- > > > > >>> 2.30.0 > > > > >>> > > > > >>> _______________________________________________ > > > > >>> dev mailing list > > > > >>> dev@openvswitch.org > > > > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > >>> > > > > >> _______________________________________________ > > > > >> dev mailing list > > > > >> dev@openvswitch.org > > > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > > > > > dev mailing list > > > > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev < > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Numan, Han, please check this out (v3): https://patchwork.ozlabs.org/project/ovn/patch/20220218183814.2976667-1-odivlad@gmail.com/ Regards, Vladislav Odintsov > On 18 Feb 2022, at 20:16, Han Zhou <hzhou@ovn.org> wrote: > > On Thu, Feb 17, 2022 at 3:57 PM Numan Siddique <numans@ovn.org> wrote: >> >> On Thu, Feb 17, 2022 at 5:50 PM Han Zhou <hzhou@ovn.org> wrote: >>> >>> On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique <numans@ovn.org> wrote: >>>> >>>> On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov <odivlad@gmail.com> >>> wrote: >>>>> >>>>> Hi Han, >>>>> >>>>> thanks for the note about log message, I’ll fix this in v3 right > after >>> the question with other_config is resolved. >>>>> Frankly speaking I also don’t understand why to sync > ovn-set-local-ip >>> option to other_config — >>>>> I did this only because Numan asked to do :) >>>>> >>>>> Regards, >>>>> Vladislav Odintsov >>>>> >>>>>> On 17 Feb 2022, at 09:39, Han Zhou <hzhou@ovn.org> wrote: >>>>>> >>>>>> On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique <numans@ovn.org >>> <mailto:numans@ovn.org>> wrote: >>>>>>> >>>>>>> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov < > odivlad@gmail.com> >>>>>> wrote: >>>>>>>> >>>>>>>> When transport node has multiple interfaces (vlans) and >>>>>>>> ovn-encap-ip on different hosts need to be configured >>>>>>>> from different VLANs source IP for encapsulated packet >>>>>>>> can be not the same, which is expected by remote system. >>>>>>>> >>>>>>>> Explicitely setting local_ip resolves such problem. >>>>>>>> >>>>>>>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >>>>>>> >>>>>>> Hi Vladislav, >>>>>>> >>>>>>> Can you please add the code to copy the new added config from OVS >>>>>>> database to the >>>>>>> other_config column of Chassis table in Southbound db ? >>>>>>> >>>>>>> Please take a look at "struct ovs_chassis_cfg" in >>> controller/chassis.c >>>>>>> >>>>>>> Thanks >>>>>>> Numan >>>>>> >>>>>> Hi Numan, >>>>>> >>>>>> May I ask what's the purpose of adding this to other_config in > SB? I >>>>>> understand that there are fields in other_config that is of >>> importance for >>>>>> other chassises, so need to be added to SB, but for this one, how >>> would it >>>>>> be used in SB DB? >>>> >>>> My understanding is that we just clone all the local ovs configs into >>>> chassis's other_config and my suggestion was to >>>> make sure we are consistent with the present behaviour. Have we >>>> missed copying some of the ovs configs ? >>>> >>>> I'm actually fine either way. If you think it's better not to copy to >>>> the sb db, then I am fine with it. >>>> >>> >>> Thanks Numan for the confirmation. I was wondering if I missed any >>> important use case of the OVS configs being stored in SB, now it seems > it >>> is fine not adding them. There are in fact many OVS configs not in SB, > such >>> as ovn-remote, ovn-remote-probe-interval, ovn-openflow-probe-interval, >>> ovn-encap-tos, ovn-match-northd-version, etc. All these are only locally >>> important, so I don't think it is necessary to sync them to SB. Probably >>> there were historical reasons why some of the configs were synced to SB >>> while they were useful only locally, and I can't recall all the details. >>> Maybe we can remove the unnecessary ones from SB in a separate patch to >>> reduce the noise of the SB chassis table, not urgent though. >> >> Sounds good to me. >> >> Does @Vladislav Odintsov need to submit another version reverting the >> change I requested ? >> > I think Vladislav mentioned that he will submit v3 to fix the error log, > maybe he could revert the other-config change in v3, too. > >> Numan >> >>> >>> Thanks, >>> Han >>> >>>> Numan >>>> >>>>>> Hi Vladislav, >>>>>> >>>>>> Regarding this: >>>>>> VLOG_ERR("ovn-encap-ip has been configured as a > list. >>> This " >>>>>> "is unsupported for IPsec."); >>>>>> >>>>>> Before your change it applies to IPsec only, but with your change > this >>>>>> would apply to non-IPsec as well, if ovn-set-local-ip is true. > Could >>> you >>>>>> update the error log as well? (it is better to be ratelimited, > but it >>> is >>>>>> not the fault of your patch) >>>>>> >>>>>> Thanks, >>>>>> Han >>>>>> >>>>>>> >>>>>>>> --- >>>>>>>> controller/encaps.c | 37 >>> +++++++++++++++++++++------------ >>>>>>>> controller/ovn-controller.8.xml | 7 +++++++ >>>>>>>> tests/ovn-controller.at | 9 ++++++++ >>>>>>>> 3 files changed, 40 insertions(+), 13 deletions(-) >>>>>>>> >>>>>>>> diff --git a/controller/encaps.c b/controller/encaps.c >>>>>>>> index 66e0cd8cd..3b0c92931 100644 >>>>>>>> --- a/controller/encaps.c >>>>>>>> +++ b/controller/encaps.c >>>>>>>> @@ -23,6 +23,7 @@ >>>>>>>> #include "openvswitch/vlog.h" >>>>>>>> #include "lib/ovn-sb-idl.h" >>>>>>>> #include "ovn-controller.h" >>>>>>>> +#include "smap.h" >>>>>>>> >>>>>>>> VLOG_DEFINE_THIS_MODULE(encaps); >>>>>>>> >>>>>>>> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const > struct >>>>>> sbrec_sb_global *sbg, >>>>>>>> smap_add(&options, "dst_port", dst_port); >>>>>>>> } >>>>>>>> >>>>>>>> + const struct ovsrec_open_vswitch *cfg = >>>>>>>> + ovsrec_open_vswitch_table_first(ovs_table); >>>>>>>> + >>>>>>>> + bool set_local_ip = false; >>>>>>>> + if (cfg) { >>>>>>>> + /* If the tos option is configured, get it */ >>>>>>>> + 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 ovn-set-local-ip option is configured, get it */ >>>>>>>> + set_local_ip = smap_get_bool(&cfg->external_ids, >>>>>> "ovn-set-local-ip", >>>>>>>> + false); >>>>>>>> + } >>>>>>>> + >>>>>>>> /* Add auth info if ipsec is enabled. */ >>>>>>>> if (sbg->ipsec) { >>>>>>>> + set_local_ip = true; >>>>>>>> + smap_add(&options, "remote_name", new_chassis_id); >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (set_local_ip) { >>>>>>>> const struct sbrec_chassis *this_chassis = > tc->this_chassis; >>>>>>>> const char *local_ip = NULL; >>>>>>>> >>>>>>>> @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const > struct >>>>>> sbrec_sb_global *sbg, >>>>>>>> if (local_ip) { >>>>>>>> smap_add(&options, "local_ip", local_ip); >>>>>>>> } >>>>>>>> - 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, >>>>>>>> diff --git a/controller/ovn-controller.8.xml >>>>>> b/controller/ovn-controller.8.xml >>>>>>>> index e9708fe64..cc9a7d1c2 100644 >>>>>>>> --- a/controller/ovn-controller.8.xml >>>>>>>> +++ b/controller/ovn-controller.8.xml >>>>>>>> @@ -304,6 +304,13 @@ >>>>>>>> of how many entries there are in the cache. By default > this >>>>>> is set to >>>>>>>> 30000 (30 seconds). >>>>>>>> </dd> >>>>>>>> + <dt><code>external_ids:ovn-set-local-ip</code></dt> >>>>>>>> + <dd> >>>>>>>> + The boolean flag indicates if > <code>ovn-controller</code> >>> when >>>>>> create >>>>>>>> + tunnel ports should set <code>local_ip</code> > parameter. >>> Can >>>>>> be >>>>>>>> + heplful to pin source outer IP for the tunnel when > multiple >>>>>> interfaces >>>>>>>> + are used on the host for overlay traffic. >>>>>>>> + </dd> >>>>>>>> </dl> >>>>>>>> >>>>>>>> <p> >>>>>>>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >>>>>>>> index 2f39e5f3e..9e6302e5a 100644 >>>>>>>> --- a/tests/ovn-controller.at >>>>>>>> +++ b/tests/ovn-controller.at >>>>>>>> @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type >>> geneve]) >>>>>>>> ovs-vsctl del-port ovn-fakech-0 >>>>>>>> OVS_WAIT_UNTIL([check_tunnel_property type geneve]) >>>>>>>> >>>>>>>> +# set `ovn-set-local-ip` option to true and check if tunnel >>> parameters >>>>>>>> +OVS_WAIT_WHILE([check_tunnel_property options:local_ip >>>>>> "\"192.168.0.1\""]) >>>>>>>> +ovs-vsctl set open . external_ids:ovn-set-local-ip=true >>>>>>>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip >>>>>> "\"192.168.0.1\""]) >>>>>>>> + >>>>>>>> +# Change the local_ip on the OVS side and check than OVN fixes > it >>>>>>>> +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" >>>>>>>> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip >>>>>> "\"192.168.0.1\""]) >>>>>>>> + >>>>>>>> # Gracefully terminate daemons >>>>>>>> OVN_CLEANUP_SBOX([hv]) >>>>>>>> OVN_CLEANUP_VSWITCH([main]) >>>>>>>> -- >>>>>>>> 2.30.0 >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> dev mailing list >>>>>>>> dev@openvswitch.org >>>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> dev mailing list >>>>>>> dev@openvswitch.org >>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>> _______________________________________________ >>>>>> dev mailing list >>>>>> dev@openvswitch.org <mailto:dev@openvswitch.org> >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev < >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>>>> _______________________________________________ >>>>> dev mailing list >>>>> dev@openvswitch.org >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/controller/encaps.c b/controller/encaps.c index 66e0cd8cd..3b0c92931 100644 --- a/controller/encaps.c +++ b/controller/encaps.c @@ -23,6 +23,7 @@ #include "openvswitch/vlog.h" #include "lib/ovn-sb-idl.h" #include "ovn-controller.h" +#include "smap.h" VLOG_DEFINE_THIS_MODULE(encaps); @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, smap_add(&options, "dst_port", dst_port); } + const struct ovsrec_open_vswitch *cfg = + ovsrec_open_vswitch_table_first(ovs_table); + + bool set_local_ip = false; + if (cfg) { + /* If the tos option is configured, get it */ + 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 ovn-set-local-ip option is configured, get it */ + set_local_ip = smap_get_bool(&cfg->external_ids, "ovn-set-local-ip", + false); + } + /* Add auth info if ipsec is enabled. */ if (sbg->ipsec) { + set_local_ip = true; + smap_add(&options, "remote_name", new_chassis_id); + } + + if (set_local_ip) { const struct sbrec_chassis *this_chassis = tc->this_chassis; const char *local_ip = NULL; @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, if (local_ip) { smap_add(&options, "local_ip", local_ip); } - 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, diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml index e9708fe64..cc9a7d1c2 100644 --- a/controller/ovn-controller.8.xml +++ b/controller/ovn-controller.8.xml @@ -304,6 +304,13 @@ of how many entries there are in the cache. By default this is set to 30000 (30 seconds). </dd> + <dt><code>external_ids:ovn-set-local-ip</code></dt> + <dd> + The boolean flag indicates if <code>ovn-controller</code> when create + tunnel ports should set <code>local_ip</code> parameter. Can be + heplful to pin source outer IP for the tunnel when multiple interfaces + are used on the host for overlay traffic. + </dd> </dl> <p> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 2f39e5f3e..9e6302e5a 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve]) ovs-vsctl del-port ovn-fakech-0 OVS_WAIT_UNTIL([check_tunnel_property type geneve]) +# set `ovn-set-local-ip` option to true and check if tunnel parameters +OVS_WAIT_WHILE([check_tunnel_property options:local_ip "\"192.168.0.1\""]) +ovs-vsctl set open . external_ids:ovn-set-local-ip=true +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) + +# Change the local_ip on the OVS side and check than OVN fixes it +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1" +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""]) + # Gracefully terminate daemons OVN_CLEANUP_SBOX([hv]) OVN_CLEANUP_VSWITCH([main])
When transport node has multiple interfaces (vlans) and ovn-encap-ip on different hosts need to be configured from different VLANs source IP for encapsulated packet can be not the same, which is expected by remote system. Explicitely setting local_ip resolves such problem. Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- controller/encaps.c | 37 +++++++++++++++++++++------------ controller/ovn-controller.8.xml | 7 +++++++ tests/ovn-controller.at | 9 ++++++++ 3 files changed, 40 insertions(+), 13 deletions(-)