Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Vladislav Odintsov]

Hi Numan, Han,

please check this out (v3):
https://patchwork.ozlabs.org/project/ovn/patch/20220218183814.2976667-1-odiv...@gmail.com/

Regards,
Vladislav Odintsov

> On 18 Feb 2022, at 20:16, Han Zhou  wrote:
> 
> On Thu, Feb 17, 2022 at 3:57 PM Numan Siddique  wrote:
>> 
>> On Thu, Feb 17, 2022 at 5:50 PM Han Zhou  wrote:
>>> 
>>> On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique  wrote:
 
 On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov 
>>> 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  wrote:
>> 
>> On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique >> > 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 
>>> 
>>> 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(, "dst_port", dst_port);
}
 

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Han Zhou]

On Thu, Feb 17, 2022 at 3:57 PM Numan Siddique  wrote:
>
> On Thu, Feb 17, 2022 at 5:50 PM Han Zhou  wrote:
> >
> > On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique  wrote:
> > >
> > > On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov 
> > 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  wrote:
> > > > >
> > > > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique  > > 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 
> > > > >>
> > > > >> 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(, "dst_port", dst_port);
> > > > >>> }
> > > > >>>
> > > > >>> +const struct 

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Numan Siddique]

On Thu, Feb 17, 2022 at 5:50 PM Han Zhou  wrote:
>
> On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique  wrote:
> >
> > On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov 
> 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  wrote:
> > > >
> > > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique  > wrote:
> > > >>
> > > >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov 
> > > > 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 
> > > >>
> > > >> 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(, "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(>external_ids,
> > > >>> +   "ovn-encap-tos", "none");
> > > >>> +
> > > >>> +if (encap_tos && strcmp(encap_tos, "none")) {
> > > >>> +smap_add(, "tos", 

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Han Zhou]

On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique  wrote:
>
> On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov 
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  wrote:
> > >
> > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique mailto:num...@ovn.org>> wrote:
> > >>
> > >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov 
> > > 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 
> > >>
> > >> 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(, "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(>external_ids,
> > >>> +   "ovn-encap-tos", "none");
> > >>> +
> > >>> +if (encap_tos && strcmp(encap_tos, "none")) {
> > >>> +smap_add(, "tos", encap_tos);
> > >>> +}
> > >>> +
> > >>> +/* If ovn-set-local-ip option is configured, get it */
> > >>> +set_local_ip = smap_get_bool(>external_ids,
> > > "ovn-set-local-ip",
> > >>> + false);
> > >>> +}
> > >>> +
> > >>> /* Add auth info if ipsec is enabled. */
> > >>> if (sbg->ipsec) {
> > >>> +set_local_ip = true;
> > >>> +smap_add(, "remote_name", 

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Numan Siddique]

On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov  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  wrote:
> >
> > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique  > > wrote:
> >>
> >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov 
> > 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 
> >>
> >> 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(, "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(>external_ids,
> >>> +   "ovn-encap-tos", "none");
> >>> +
> >>> +if (encap_tos && strcmp(encap_tos, "none")) {
> >>> +smap_add(, "tos", encap_tos);
> >>> +}
> >>> +
> >>> +/* If ovn-set-local-ip option is configured, get it */
> >>> +set_local_ip = smap_get_bool(>external_ids,
> > "ovn-set-local-ip",
> >>> + false);
> >>> +}
> >>> +
> >>> /* Add auth info if ipsec is enabled. */
> >>> if (sbg->ipsec) {
> >>> +set_local_ip = true;
> >>> +smap_add(, "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(, "local_ip", local_ip);
> >>> }
> >>> -smap_add(, "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(>external_ids,
> >>> -   "ovn-encap-tos", "none");
> >>> -
> >>> -if (encap_tos && strcmp(encap_tos, "none")) {
> >>> -smap_add(, "tos", encap_tos);
> >>> -}
> >>> }
> >>>
> >>> /* If there's an existing chassis record that does not need any
> > 

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Vladislav Odintsov]

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  wrote:
> 
> On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique  > wrote:
>> 
>> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov 
> 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 
>> 
>> 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(, "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(>external_ids,
>>> +   "ovn-encap-tos", "none");
>>> +
>>> +if (encap_tos && strcmp(encap_tos, "none")) {
>>> +smap_add(, "tos", encap_tos);
>>> +}
>>> +
>>> +/* If ovn-set-local-ip option is configured, get it */
>>> +set_local_ip = smap_get_bool(>external_ids,
> "ovn-set-local-ip",
>>> + false);
>>> +}
>>> +
>>> /* Add auth info if ipsec is enabled. */
>>> if (sbg->ipsec) {
>>> +set_local_ip = true;
>>> +smap_add(, "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(, "local_ip", local_ip);
>>> }
>>> -smap_add(, "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(>external_ids,
>>> -   "ovn-encap-tos", "none");
>>> -
>>> -if (encap_tos && strcmp(encap_tos, "none")) {
>>> -smap_add(, "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
>>> 3 (30 seconds).
>>>   
>>> +  external_ids:ovn-set-local-ip
>>> +  
>>> +The boolean flag indicates if ovn-controller when
> create
>>> +tunnel ports should set local_ip parameter.  Can
> be
>>> +heplful to pin source outer IP for the tunnel when multiple
> 

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Han Zhou]

On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique  wrote:
>
> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov 
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 
>
> 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(, "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(>external_ids,
> > +   "ovn-encap-tos", "none");
> > +
> > +if (encap_tos && strcmp(encap_tos, "none")) {
> > +smap_add(, "tos", encap_tos);
> > +}
> > +
> > +/* If ovn-set-local-ip option is configured, get it */
> > +set_local_ip = smap_get_bool(>external_ids,
"ovn-set-local-ip",
> > + false);
> > +}
> > +
> >  /* Add auth info if ipsec is enabled. */
> >  if (sbg->ipsec) {
> > +set_local_ip = true;
> > +smap_add(, "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(, "local_ip", local_ip);
> >  }
> > -smap_add(, "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(>external_ids,
> > -   "ovn-encap-tos", "none");
> > -
> > -if (encap_tos && strcmp(encap_tos, "none")) {
> > -smap_add(, "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
> >  3 (30 seconds).
> >
> > +  external_ids:ovn-set-local-ip
> > +  
> > +The boolean flag indicates if ovn-controller when
create
> > +tunnel ports should set local_ip parameter.  Can
be
> > +heplful to pin source outer IP for the tunnel when multiple
interfaces
> > +are used on the host for overlay traffic.
> > +  
> >  
> >
> >  
> > 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])

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Vladislav Odintsov]

Hi Numan,

I’ve submitted the v2, please check this out:
https://patchwork.ozlabs.org/project/ovn/patch/20220215145442.2868060-1-odiv...@gmail.com/

Regards,
Vladislav Odintsov

> On 11 Feb 2022, at 22:12, Numan Siddique  wrote:
> 
> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov  > 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 > >
> 
> 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(, "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(>external_ids,
>> +   "ovn-encap-tos", "none");
>> +
>> +if (encap_tos && strcmp(encap_tos, "none")) {
>> +smap_add(, "tos", encap_tos);
>> +}
>> +
>> +/* If ovn-set-local-ip option is configured, get it */
>> +set_local_ip = smap_get_bool(>external_ids, "ovn-set-local-ip",
>> + false);
>> +}
>> +
>> /* Add auth info if ipsec is enabled. */
>> if (sbg->ipsec) {
>> +set_local_ip = true;
>> +smap_add(, "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(, "local_ip", local_ip);
>> }
>> -smap_add(, "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(>external_ids,
>> -   "ovn-encap-tos", "none");
>> -
>> -if (encap_tos && strcmp(encap_tos, "none")) {
>> -smap_add(, "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
>> 3 (30 seconds).
>>   
>> +  external_ids:ovn-set-local-ip
>> +  
>> +The boolean flag indicates if ovn-controller when 
>> create
>> +tunnel ports should set local_ip parameter.  Can be
>> +heplful to pin source outer IP for the tunnel when multiple 
>> interfaces
>> +are used on the host for overlay traffic.
>> +  
>> 
>> 
>> 
>> 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\""])
>> +
>> # 

[ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Vladislav Odintsov]

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 
---
 controller/chassis.c| 12 +++
 controller/encaps.c | 37 +
 controller/ovn-controller.8.xml |  7 +++
 ovn-sb.xml  |  9 
 tests/ovn-controller.at |  9 
 5 files changed, 61 insertions(+), 13 deletions(-)

diff --git a/controller/chassis.c b/controller/chassis.c
index 8a1559653..1a3341079 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -64,6 +64,8 @@ struct ovs_chassis_cfg {
 struct ds iface_types;
 /* Is this chassis an interconnection gateway. */
 bool is_interconn;
+/* Whether we should configure local_ip tunnel port option.*/
+bool set_local_ip;
 };
 
 static void
@@ -192,6 +194,12 @@ get_is_interconn(const struct smap *ext_ids)
 return smap_get_bool(ext_ids, "ovn-is-interconn", false);
 }
 
+static bool
+get_set_local_ip(const struct smap *ext_ids)
+{
+return smap_get_bool(ext_ids, "ovn-set-local-ip", false);
+}
+
 static void
 update_chassis_transport_zones(const struct sset *transport_zones,
const struct sbrec_chassis *chassis_rec)
@@ -324,6 +332,8 @@ chassis_parse_ovs_config(const struct 
ovsrec_open_vswitch_table *ovs_table,
 
 ovs_cfg->is_interconn = get_is_interconn(>external_ids);
 
+ovs_cfg->set_local_ip = get_set_local_ip(>external_ids);
+
 return true;
 }
 
@@ -350,6 +360,8 @@ chassis_build_other_config(const struct ovs_chassis_cfg 
*ovs_cfg,
 smap_replace(config, "is-interconn",
  ovs_cfg->is_interconn ? "true" : "false");
 smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true");
+smap_replace(config, "ovn-set-local-ip",
+ ovs_cfg->set_local_ip ? "true" : "false");
 }
 
 /*
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(, "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(>external_ids,
+   "ovn-encap-tos", "none");
+
+if (encap_tos && strcmp(encap_tos, "none")) {
+smap_add(, "tos", encap_tos);
+}
+
+/* If ovn-set-local-ip option is configured, get it */
+set_local_ip = smap_get_bool(>external_ids, "ovn-set-local-ip",
+ false);
+}
+
 /* Add auth info if ipsec is enabled. */
 if (sbg->ipsec) {
+set_local_ip = true;
+smap_add(, "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(, "local_ip", local_ip);
 }
-smap_add(, "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(>external_ids,
-   "ovn-encap-tos", "none");
-
-if (encap_tos && strcmp(encap_tos, "none")) {
-smap_add(, "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
 3 (30 seconds).
   
+  external_ids:ovn-set-local-ip
+  
+The boolean flag indicates if ovn-controller when create
+tunnel ports should set local_ip parameter.  Can be
+heplful to pin source outer IP for the tunnel when multiple interfaces
+are used on the host for overlay traffic.
+  
 
 
 
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 9ddacdf09..46b8192d8 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -300,6 +300,15 @@
   See ovn-controller(8) for more information.
 
 
+
+  ovn-controller populates this 

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Numan Siddique]

On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov  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 

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(, "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(>external_ids,
> +   "ovn-encap-tos", "none");
> +
> +if (encap_tos && strcmp(encap_tos, "none")) {
> +smap_add(, "tos", encap_tos);
> +}
> +
> +/* If ovn-set-local-ip option is configured, get it */
> +set_local_ip = smap_get_bool(>external_ids, "ovn-set-local-ip",
> + false);
> +}
> +
>  /* Add auth info if ipsec is enabled. */
>  if (sbg->ipsec) {
> +set_local_ip = true;
> +smap_add(, "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(, "local_ip", local_ip);
>  }
> -smap_add(, "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(>external_ids,
> -   "ovn-encap-tos", "none");
> -
> -if (encap_tos && strcmp(encap_tos, "none")) {
> -smap_add(, "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
>  3 (30 seconds).
>
> +  external_ids:ovn-set-local-ip
> +  
> +The boolean flag indicates if ovn-controller when create
> +tunnel ports should set local_ip parameter.  Can be
> +heplful to pin source outer IP for the tunnel when multiple 
> interfaces
> +are used on the host for overlay traffic.
> +  
>  
>
>  
> 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

[ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Vladislav Odintsov]

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 
---
 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(, "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(>external_ids,
+   "ovn-encap-tos", "none");
+
+if (encap_tos && strcmp(encap_tos, "none")) {
+smap_add(, "tos", encap_tos);
+}
+
+/* If ovn-set-local-ip option is configured, get it */
+set_local_ip = smap_get_bool(>external_ids, "ovn-set-local-ip",
+ false);
+}
+
 /* Add auth info if ipsec is enabled. */
 if (sbg->ipsec) {
+set_local_ip = true;
+smap_add(, "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(, "local_ip", local_ip);
 }
-smap_add(, "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(>external_ids,
-   "ovn-encap-tos", "none");
-
-if (encap_tos && strcmp(encap_tos, "none")) {
-smap_add(, "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
 3 (30 seconds).
   
+  external_ids:ovn-set-local-ip
+  
+The boolean flag indicates if ovn-controller when create
+tunnel ports should set local_ip parameter.  Can be
+heplful to pin source outer IP for the tunnel when multiple interfaces
+are used on the host for overlay traffic.
+  
 
 
 
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