> Hi Dumitru, Lorenzo,
>
> my comments are inline.
>
> On 05.02.2025 13:30, Dumitru Ceara wrote:
> > On 1/31/25 6:37 PM, Lorenzo Bianconi wrote:
> >> Fix ovn-ic mode when vxlan is used as encapsulation mode reducing the
> >> maximum local dp key to ((2<<10)-1) in order to make some room for
> >> OVN_MAX_DP_VXLAN_KEY_GLOBAL (vxlan tunnels export just 12 bit for
> >> metadata key).
> >>
> >> Reported-at: https://issues.redhat.com/browse/FDP-1023
> >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> >> ---
> > Hi Lorenzo,
> >
> > I have some comments inline - nothing major I think.
> >
> > However, I'm curious though about Vladislav's opinion of this change.
> > Vladislav could you please confirm whether this doesn't break your
> > existing scenarios?
> IIUC, with v4 the ic vxlan_mode enabling was moved under the
> configuration knob and disabled by default providing previous behavior,
> which is totally fine for us.
Hi Vladislav,
ack, thx for the review.
> >
> > Thanks,
> > Dumitru
> >
> >> - Changes in v4:
> >> Introduce vxlan_mode in option column of IC_NB_Global table to enable
> >> VXLAN protocol for cross-AZ traffic. Default value is false.
> >> - Changes in v3:
> >> Reduce the max local dp key to 1023 just if the cluster is running in
> >> ovn-ic mode
> >> - Changes in v2:
> >> Document local datapath limitation
> >> ---
> >> NEWS | 2 ++
> >> ic/ovn-ic.c | 48 +++++++++++++++++++++++++++++------
> >> lib/ovn-util.h | 4 ++-
> >> northd/en-global-config.c | 53 ++++++++++++++++++++++++++++++++++++++-
> >> northd/en-global-config.h | 2 ++
> >> northd/inc-proc-northd.c | 2 ++
> >> northd/northd.c | 30 +++++++++++++++-------
> >> northd/northd.h | 2 +-
> >> ovn-ic-nb.xml | 7 ++++++
> >> ovn-nb.xml | 9 +++++++
> >> tests/ovn-ic.at | 32 +++++++++++++++++++++++
> >> tests/ovn-northd.at | 22 ++++++++++++++++
> >> 12 files changed, 194 insertions(+), 19 deletions(-)
> >>
[...]
> > I think the term "local datapath" is not clear. Are those "AZ-local"
> > switches? That is "non-transit logical switches"? Maybe we should just
> > call them that instead? What do you think?
> I'd also add here a note about ic-vxlan_mode
> OVN_IC_Northbound.IC_NB_Global option.
ack I will add it in v5.
> >
> >> </column>
> >>
> >> <column name="options" key="always_tunnel"
> >> @@ -888,6 +891,12 @@
> >> database. This kind of logical switch is created and controlled
> >> by <code>ovn-ic</code>.
> >> </column>
> >> + <column name="other_config" key="ic-vxlan_mode"
> >> + type='{"type": "boolean"}'>
> >> + <code>ic-vxlan_mode</code> is set to true by <code>ovn-ic</code>
> >> when
> >> + it runs <code>VXLAN</code> as encapsulation protocol for cross-AZ
> >> + traffic. Default value is false.
> This config option description should be moved into ovn-ic-nb.xml, since
> it is configured in OVN_IC_Northbound database.
please note this is ic-vxlan_mode, I have added vxlan_mode parameter in
ovn-ic-nb.xml.
Regards,
Lorenzo
> >> + </column>
> >> </group>
> >>
> >> <group title="Tunnel Key">
> >> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
> >> index fbcfca2e4..a1eccb165 100644
> >> --- a/tests/ovn-ic.at
> >> +++ b/tests/ovn-ic.at
> >> @@ -94,6 +94,38 @@ OVN_CLEANUP_IC([az1])
> >> AT_CLEANUP
> >> ])
> >>
> >> +OVN_FOR_EACH_NORTHD([
> >> +AT_SETUP([ovn-ic -- VXLAN tunnel key])
> >> +ovn_init_ic_db
> >> +net_add n1
> >> +
> >> +ovn_start az1
> >> +sim_add gw-az1
> >> +as gw-az1
> >> +
> >> +check ovs-vsctl add-br br-phys
> >> +ovn_az_attach az1 n1 br-phys 192.168.1.1
> >> +check ovs-vsctl set open . external-ids:ovn-is-interconn=true
> >> +
> >> +AT_CHECK([ovn-ic-nbctl --wait=sb ts-add ts1])
> >> +
> >> +# Check ISB
> >> +check_row_count ic-sb:Datapath_Binding 1 transit_switch=ts1
> >> +check_column "ts1" ic-sb:Datapath_Binding transit_switch
> >> +check_column "ts1" nb:Logical_Switch name
> >> +
> >> +wait_column "ic-vxlan_mode=false interconn-ts=ts1
> >> requested-tnl-key=16711682" nb:Logical_Switch other_config name="ts1"
> >> +# Check tunnel key fits in VXLAN space
> >> +check ovn-ic-nbctl --wait=sb set IC_NB_Global . options:vxlan_mode=true
> >> +wait_column "ic-vxlan_mode=true interconn-ts=ts1 requested-tnl-key=1025"
> >> nb:Logical_Switch other_config name="ts1"
> >> +
> >> +check ovn-ic-nbctl --wait=sb set IC_NB_Global . options:vxlan_mode=false
> >> +wait_column "ic-vxlan_mode=false interconn-ts=ts1
> >> requested-tnl-key=16711682" nb:Logical_Switch other_config name="ts1"
> >> +
> >> +OVN_CLEANUP_IC([az1])
> >> +AT_CLEANUP
> >> +])
> >> +
> >> OVN_FOR_EACH_NORTHD([
> >> AT_SETUP([ovn-ic -- port-bindings deletion upon TS deletion])
> >>
> >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >> index df646ec68..89c9d7c13 100644
> >> --- a/tests/ovn-northd.at
> >> +++ b/tests/ovn-northd.at
> >> @@ -2978,7 +2978,29 @@ OVS_WAIT_UNTIL([grep "all port tunnel ids
> >> exhausted" northd/ovn-northd.log])
> >> AT_CLEANUP
> >> ])
> >>
> >> +OVN_FOR_EACH_NORTHD_NO_HV([
> >> +AT_SETUP([check VXLAN encap in IC-mode])
> >> +ovn_start
> >> +
> >> +get_max_tunid() {
> >> + echo $(ovn-nbctl get NB_Global . options:max_tunid | sed s/":"//g |
> >> sed s/\"//g)
> >> +}
> >> +
> >> +check_uuid ovn-sbctl \
> >> + --id=@e create encap chassis_name=hv1 ip="192.168.0.1" type="vxlan" \
> >> + -- --id=@c create chassis name=hv1 encaps=@e
> >> +check ovn-nbctl --wait=sb ls-add LS
> >> +AT_CHECK([test "$(get_max_tunid)" -eq 4095])
> >> +
> >> +check ovn-nbctl --wait=sb set logical-switch LS
> >> other-config:interconn-ts=LS
> >> +check ovn-nbctl --wait=sb set logical-switch LS
> >> other-config:ic-vxlan_mode=true
> >> +AT_CHECK([test "$(get_max_tunid)" -eq 1023])
> >>
> >> +check ovn-nbctl --wait=sb clear logical-switch LS other-config
> >> +AT_CHECK([test "$(get_max_tunid)" -eq 4095])
> >> +
> >> +AT_CLEANUP
> >> +])
> >>
> >> OVN_FOR_EACH_NORTHD_NO_HV([
> >> AT_SETUP([Logical Flow Datapath Groups])
>
> --
> Regards,
> Vladislav Odintsov
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
