On Thu, Feb 17, 2022 at 5:50 PM Han Zhou <hz...@ovn.org> wrote:
>
> On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique <num...@ovn.org> wrote:
> >
> > On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov <odiv...@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 <hz...@ovn.org> wrote:
> > > >
> > > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique <num...@ovn.org
> <mailto:num...@ovn.org>> wrote:
> > > >>
> > > >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <odiv...@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 <odiv...@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
> > > >>> d...@openvswitch.org
> > > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >>>
> > > >> _______________________________________________
> > > >> dev mailing list
> > > >> d...@openvswitch.org
> > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > _______________________________________________
> > > > dev mailing list
> > > > d...@openvswitch.org <mailto:d...@openvswitch.org>
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
> > > _______________________________________________
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
