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.
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
