Hi Numan, Han, please check this out (v3): https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/
Regards, Vladislav Odintsov > On 18 Feb 2022, at 20:16, Han Zhou <[email protected]> wrote: > > On Thu, Feb 17, 2022 at 3:57 PM Numan Siddique <[email protected]> wrote: >> >> On Thu, Feb 17, 2022 at 5:50 PM Han Zhou <[email protected]> wrote: >>> >>> On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique <[email protected]> wrote: >>>> >>>> On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov <[email protected]> >>> 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 <[email protected]> wrote: >>>>>> >>>>>> On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique <[email protected] >>> <mailto:[email protected]>> wrote: >>>>>>> >>>>>>> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov < > [email protected]> >>>>>> 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 <[email protected]> >>>>>>> >>>>>>> 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 >>>>>>>> [email protected] >>>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> dev mailing list >>>>>>> [email protected] >>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>> _______________________________________________ >>>>>> dev mailing list >>>>>> [email protected] <mailto:[email protected]> >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev < >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>>>> _______________________________________________ >>>>> 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 > _______________________________________________ > 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
